Bug 62345

Summary: [V8][Chromium] Use per-isolate embedder data instead of statics for caches in bindings
Product: WebKit Reporter: Dmitry Lomov <dslomov>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, antonm, dglazkov, dimich, dslomov, hans, levin, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 61016    
Attachments:
Description Flags
Proposed fix
none
CR feedback + fixes for chromium tests
webkit.review.bot: commit-queue-
Rebased patch
webkit.review.bot: commit-queue-
Release mode compilation fixed.
abarth: review+, abarth: commit-queue-
CR feedback fixed
none
Oops wrong patch uploaded none

Description Dmitry Lomov 2011-06-08 18:12:14 PDT
Change caches for FunctionTemplates in v8 bindings to use v8 per-isolate embedder data
Comment 1 Dmitry Lomov 2011-06-08 18:16:57 PDT
Created attachment 96525 [details]
Proposed fix
Comment 2 Dmitry Lomov 2011-06-08 18:22:29 PDT
Comment on attachment 96525 [details]
Proposed fix

Not asking for cq, need trybots results - but would appreciate a review
Comment 3 Dmitry Lomov 2011-06-08 21:14:09 PDT
Comment on attachment 96525 [details]
Proposed fix

Patch broke chromium unit-tests; I'll be back.
Comment 4 Adam Barth 2011-06-09 00:46:21 PDT
Comment on attachment 96525 [details]
Proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=96525&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2206
> +    V8BindingPerIsolateData::TemplateMap::iterator result = 
> +        data->rawTemplateMap().find(&info);

One line pls.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2207
> +    if (result != data->rawTemplateMap().end()) {

No { pls

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2210
> +    v8::HandleScope handle_scope;

handle_scope => handleScope

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2223
> +    V8BindingPerIsolateData::TemplateMap::iterator result = 
> +        data->templateMap().find(&info);
> +    if (result != data->templateMap().end()) {
> +        return result->second;
> +    }

ditto

> Source/WebCore/bindings/v8/V8Binding.cpp:72
> +    void* data = isolate->GetData();
> +    if (data)
> +        delete static_cast<V8BindingPerIsolateData*>(data);

This pattern is really ugly.

> Source/WebCore/bindings/v8/V8Binding.cpp:595
> +    v8::Persistent<v8::FunctionTemplate>& toStringTemplate = V8BindingPerIsolateData::current()->toStringTemplate();

Using a reference here is very subtle.  Maybe use a pointer instead?

> Source/WebCore/bindings/v8/V8Binding.h:76
> +        V8BindingPerIsolateData(v8::Isolate*);

explicit
Comment 5 Dmitry Lomov 2011-06-09 12:08:54 PDT
Comment on attachment 96525 [details]
Proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=96525&action=review

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2206
>> +        data->rawTemplateMap().find(&info);
> 
> One line pls.

Thanks - will fix. I should also update example generation results in bindings/scripts/test - then the style checker would have caught me.

>> Source/WebCore/bindings/v8/V8Binding.cpp:72
>> +        delete static_cast<V8BindingPerIsolateData*>(data);
> 
> This pattern is really ugly.

What is ugly? How would you write this instead?

>> Source/WebCore/bindings/v8/V8Binding.cpp:595
>> +    v8::Persistent<v8::FunctionTemplate>& toStringTemplate = V8BindingPerIsolateData::current()->toStringTemplate();
> 
> Using a reference here is very subtle.  Maybe use a pointer instead?

I tend to use references when I want to enforce that pointers are not NULL. Tell me If this rubs strongly against webkit tradition - I'll change to pointers.
Comment 6 Dmitry Lomov 2011-06-10 09:14:14 PDT
Created attachment 96747 [details]
CR feedback + fixes for chromium tests

Chromium trybots successful
Comment 7 WebKit Review Bot 2011-06-10 09:31:40 PDT
Comment on attachment 96747 [details]
CR feedback + fixes for chromium tests

Attachment 96747 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8826430
Comment 8 Dmitry Lomov 2011-06-10 11:33:56 PDT
Created attachment 96765 [details]
Rebased patch
Comment 9 WebKit Review Bot 2011-06-10 12:46:08 PDT
Comment on attachment 96765 [details]
Rebased patch

Attachment 96765 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8825447
Comment 10 Dmitry Lomov 2011-06-10 15:25:29 PDT
Created attachment 96803 [details]
Release mode compilation fixed.
Comment 11 Adam Barth 2011-06-13 13:15:21 PDT
Comment on attachment 96803 [details]
Release mode compilation fixed.

View in context: https://bugs.webkit.org/attachment.cgi?id=96803&action=review

I'm not in love with the raw memory management, but I don't see any other choice.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2206
> +    if (result != data->rawTemplateMap().end())  return result->second;

One statement per line, plz.

> Source/WebCore/bindings/v8/V8Binding.cpp:79
> +    void* data = isolate->GetData();
> +    if (data)
> +        delete static_cast<V8BindingPerIsolateData*>(data);

This code is still ugly, but I guess that's what we get for using void* in the V8 API.  This branch is probably not needed.  delete 0 is supposed to be safe.

> Source/WebCore/bindings/v8/V8Utilities.h:73
> +        v8::Persistent<v8::Context> m_context;

Please use OwnHandle.
Comment 12 Dmitry Lomov 2011-06-13 14:11:40 PDT
Created attachment 97008 [details]
CR feedback fixed
Comment 13 Dmitry Lomov 2011-06-13 14:21:09 PDT
Created attachment 97010 [details]
Oops wrong patch uploaded
Comment 14 WebKit Review Bot 2011-06-13 14:35:03 PDT
Attachment 97010 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/bindings/v8/V8Utilities.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 WebKit Review Bot 2011-06-13 16:07:07 PDT
Comment on attachment 97010 [details]
Oops wrong patch uploaded

Clearing flags on attachment: 97010

Committed r88729: <http://trac.webkit.org/changeset/88729>
Comment 16 WebKit Review Bot 2011-06-13 16:07:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 anton muhin 2011-06-14 04:40:04 PDT
Dmitry, may you provide more details why it is needed and what's the performance impact of this change?

(In reply to comment #16)
> All reviewed patches have been landed.  Closing bug.
Comment 18 Dmitry Lomov 2011-06-14 10:12:38 PDT
(In reply to comment #17)
> Dmitry, may you provide more details why it is needed and what's the performance impact of this change?

This is a needed part of moving webworkers in-process. I have not seen the perf impact from this particular change.
Comment 19 anton muhin 2011-06-14 12:41:46 PDT
(In reply to comment #18)
> (In reply to comment #17)
> > Dmitry, may you provide more details why it is needed and what's the performance impact of this change?
> 
> This is a needed part of moving webworkers in-process. I have not seen the perf impact from this particular change.

That part I understand.  I'd be glad to know more why it's needed for in-process webworkers.
Comment 20 Dmitry Lomov 2011-06-14 13:37:24 PDT
(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > Dmitry, may you provide more details why it is needed and what's the performance impact of this change?
> > 
> > This is a needed part of moving webworkers in-process. I have not seen the perf impact from this particular change.
> 
> That part I understand.  I'd be glad to know more why it's needed for in-process webworkers.

Workers access V8 bindings, starting with V8DedicatedWorkerContext. Therefore they need a per-isolate cache for function templates.
Comment 21 Hans Wennborg 2011-07-04 08:19:54 PDT
Hi Dmitry,

This patch causes problems for Indexed DB. It seems that when creating and destroying many V8LocalContext objects, memory is not free'd properly, and we eventually run out of it.

This diff adds a test to webkit_unit_tests that exposes the problem (make sure to build in Release mode, this is pretty slow otherwise):

diff --git a/Source/WebKit/chromium/tests/IDBBindingUtilitiesTest.cpp b/Source/WebKit/chromium/tests/IDBBindingUtilitiesTest.cpp
index 3647ce6..aad9f02 100644
--- a/Source/WebKit/chromium/tests/IDBBindingUtilitiesTest.cpp
+++ b/Source/WebKit/chromium/tests/IDBBindingUtilitiesTest.cpp
@@ -106,6 +106,14 @@ TEST(IDBKeyFromValueAndKeyPathTest, TopLevelPropertyStringValue)
     checkKeyPathNullValue(serializedScriptValue.get(), "[3]");
 }
 
+TEST(IDBKeyFromValueAndKeyPathTest, ManyV8LocalContexts)
+{
+    for (int i = 0; i < 100000; ++i) {
+        printf("%d\n", i);
+        V8LocalContext v8context;
+    }
+}
+


There seems to be something subtle going wrong with the use of OwnHandle in V8LocalContext. If I apply the following patch, the problem goes away, but I'm not sure why; can you take a look?


diff --git a/Source/WebCore/bindings/v8/V8Utilities.cpp b/Source/WebCore/bindings/v8/V8Utilities.cpp
index 490cb74..4f783b6 100644
--- a/Source/WebCore/bindings/v8/V8Utilities.cpp
+++ b/Source/WebCore/bindings/v8/V8Utilities.cpp
@@ -49,16 +49,17 @@
 namespace WebCore {
 
 V8LocalContext::V8LocalContext()
+    : m_context(v8::Context::New())
 {
     V8BindingPerIsolateData::ensureInitialized(v8::Isolate::GetCurrent());
-    m_context.set(v8::Context::New());
-    m_context.get()->Enter();
+    m_context->Enter();
 }
 
 
-V8LocalContext::~V8LocalContext() 
+V8LocalContext::~V8LocalContext()
 {
-    m_context.get()->Exit();
+    m_context->Exit();
+    m_context.Dispose();
 }
 
 // Use an array to hold dependents. It works like a ref-counted scheme.
diff --git a/Source/WebCore/bindings/v8/V8Utilities.h b/Source/WebCore/bindings/v8/V8Utilities.h
index 6f48907..c217964 100644
--- a/Source/WebCore/bindings/v8/V8Utilities.h
+++ b/Source/WebCore/bindings/v8/V8Utilities.h
@@ -71,7 +71,7 @@ namespace WebCore {
         virtual ~V8LocalContext();
     private:
         v8::HandleScope m_handleScope;
-        OwnHandle<v8::Context> m_context;
+        v8::Persistent<v8::Context> m_context;
Comment 22 Adam Barth 2011-07-04 11:05:16 PDT
Please file a new bug rather than posting to this closed bug.

The problem is that OwnHandle is responsible for calling New and Disposed in a balanced way.  Because this code calls New, that New is never balanced with a Dispose, which leads to the leak.
Comment 23 Dmitry Lomov 2011-07-04 14:34:26 PDT
Hans, thanks for tracking this down - I guess it wasn't easy...

(In reply to comment #22)
> Please file a new bug rather than posting to this closed bug.
> 
> The problem is that OwnHandle is responsible for calling New and Disposed in a balanced way.  Because this code calls New, that New is never balanced with a Dispose, which leads to the leak.

I do not think you are right: I am not sure what is the problem with the code yet, but OwnHandle only calls New and Dispose on Persistent, and this code calls New on Context. I feel this usage should be OK, but there might be a subtle reason for it not to - maybe I somehow misunderstand OwnHandle?

Anyway, it looks like OwnHandle leads to too much subtlety in this code, and direct use of Persistent (as in  https://bug-62345-attachments.webkit.org/attachment.cgi?id=96803 or Hans' patch) is much easier to reason about.
Comment 24 Adam Barth 2011-07-04 20:35:31 PDT
OK.  The goal of OwnHandle is to make the code less subtle.  I guess that means ownhandle needs more work.  :(

In any case, please feel free to remove the use of ownhandle if its not helpful.