Bug 62345 – [V8][Chromium] Use per-isolate embedder data instead of statics for caches in bindings

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Dmitry Lomov]

Change caches for FunctionTemplates in v8 bindings to use v8 per-isolate embedder data

[Dmitry Lomov]

Created attachment 96525 [details]
Proposed fix

[Dmitry Lomov]

Comment on attachment 96525 [details]
Proposed fix

Not asking for cq, need trybots results - but would appreciate a review

[Dmitry Lomov]

Comment on attachment 96525 [details]
Proposed fix

Patch broke chromium unit-tests; I'll be back.

[Adam Barth]

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

[Dmitry Lomov]

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.

[Dmitry Lomov]

Created attachment 96747 [details]
CR feedback + fixes for chromium tests

Chromium trybots successful

[WebKit Review Bot]

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

[Dmitry Lomov]

Created attachment 96765 [details]
Rebased patch

[WebKit Review Bot]

Comment on attachment 96765 [details]
Rebased patch

Attachment 96765 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8825447

[Dmitry Lomov]

Created attachment 96803 [details]
Release mode compilation fixed.

[Adam Barth]

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.

[Dmitry Lomov]

Created attachment 97008 [details]
CR feedback fixed

[Dmitry Lomov]

Created attachment 97010 [details]
Oops wrong patch uploaded

[WebKit Review Bot]

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.

[WebKit Review Bot]

Comment on attachment 97010 [details]
Oops wrong patch uploaded

Clearing flags on attachment: 97010

Committed r88729: <http://trac.webkit.org/changeset/88729>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[anton muhin]

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.

[Dmitry Lomov]

(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.

[anton muhin]

(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.

[Dmitry Lomov]

(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.

[Hans Wennborg]

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;

[Adam Barth]

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.

[Dmitry Lomov]

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.

[Adam Barth]

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.