Bug 63041 – [Chromium][V8] Make DOMDataStore per-isolate

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Dmitry Lomov]

Another prerequisite to making WebWorkers per-isolate: make sure that DOMDataStore is accessible from multiple threads

[Dmitry Lomov]

The hot spot for this work is getting current DOMDataStore.
I have been experimenting with various implementation, and the following strategy recovers all perf on Dromaeo/?dom on Linux:


Index: WebCore/bindings/v8/DOMData.cpp
===================================================================
--- WebCore/bindings/v8/DOMData.cpp     (revision 89232)
+++ WebCore/bindings/v8/DOMData.cpp     (working copy)
@@ -53,12 +54,12 @@
 
 DOMDataStore& DOMData::getMainThreadStore()
 {
    // This is broken out as a separate non-virtual method from getStore()
    // so that it can be inlined by getCurrentMainThreadStore, which is
    // a hot spot in Dromaeo DOM tests.
-    V8IsolatedContext* context = V8IsolatedContext::getEntered();
-    if (UNLIKELY(context != 0))
-        return *context->world()->domDataStore();
+    DOMDataStore* currentDOMDataStore = V8BindingPerIsolateData::current()->getDOMDataStore();
+    if (UNLIKELY(currentDOMDataStore != 0))
+        return *currentDOMDataStore;
     return m_defaultStore;
 }

I spare you all the gory details, but the idea is that:
1) current active DOMDataStore is saved in V8BindingPerIsolateData unless we are in main world
2) required changes include updating current DOMDataStore in per-isolate data on entering and exiting isolated worlds.
3) I keep m_defaultStore so that I do not have to reason about initialization order, but with the appearance of single V8 intitialization point in WebScriptController::inititialize, I might as well remove it too, and the whole function will then be  
      return *V8BindingPerIsolateData::current()->getDOMDataStore();

[Dmitry Lomov]

Here is some prelim data on Linux (before/after):
http://dromaeo.com/?id=142768,142775
there is even some minor speed-up (although dromaeo is crazy volatile, so don't believe it much)

[anton muhin]

(In reply to comment #2)
> Here is some prelim data on Linux (before/after):
> http://dromaeo.com/?id=142768,142775
> there is even some minor speed-up (although dromaeo is crazy volatile, so don't believe it much)

Nice!

Just FYI: this path should be hottest on DOM Query (we return the same element in the tight loop).  So that definitely suppors your point that there is no perf regression.

[Dmitry Lomov]

Created attachment 100559 [details]
The fix

The patch:
- makes DOMData a static utility class
- adds per-isolate DOMDataStore to V8BindingPerIsolateData 

dromaeo.com/?dom is not affected on Linux (Linux is a slow-TLS platform for V8):
http://dromaeo.com/?id=144456,144540

[Dmitry Lomov]

Comment on attachment 100559 [details]
The fix

(no cq yet - waiting for chromium trybots)

[Dmitry Lomov]

Comment on attachment 100559 [details]
The fix

Errors on trybots.

[Dmitry Lomov]

Created attachment 100582 [details]
Added field initialization to V8BindingPerIsolateData constructor

[Dmitry Lomov]

Comment on attachment 100582 [details]
Added field initialization to V8BindingPerIsolateData constructor

Chromium trybots are happy.

[Adam Barth]

Comment on attachment 100582 [details]
Added field initialization to V8BindingPerIsolateData constructor

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

> Source/WebCore/bindings/v8/DOMData.h:56
> +        static DOMDataStore& getCurrentStore();

Sorry to pick nits, but we usually skip the "get" prefix on these function names.

> Source/WebCore/bindings/v8/V8Binding.h:114
> +        DOMDataStore* getDOMDataStore() { return m_DOMDataStore; }
> +        void setDOMDataStore(DOMDataStore* store) { m_DOMDataStore = store; }

We keep the "set" prefix though.

I don't see where setDOMDataStore is ever called.  How does m_DOMDataStore become non-null ?

> Source/WebCore/bindings/v8/V8Binding.h:127
> +        DOMDataStore* m_DOMDataStore;

It's unclear to me what the ownership model for this pointer is, probably because I can't find anyone calling the setter.

[Dmitry Lomov]

(In reply to comment #9)
> (From update of attachment 100582 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100582&action=review
> 
> > Source/WebCore/bindings/v8/DOMData.h:56
> > +        static DOMDataStore& getCurrentStore();
> 
> Sorry to pick nits, but we usually skip the "get" prefix on these function names.

Sure, will fix.

> 
> > Source/WebCore/bindings/v8/V8Binding.h:114
> > +        DOMDataStore* getDOMDataStore() { return m_DOMDataStore; }
> > +        void setDOMDataStore(DOMDataStore* store) { m_DOMDataStore = store; }
> 
> We keep the "set" prefix though.
> 
> I don't see where setDOMDataStore is ever called.  How does m_DOMDataStore become non-null ?

This is the next patch :) The intent is that workers will do this.
> 
> > Source/WebCore/bindings/v8/V8Binding.h:127
> > +        DOMDataStore* m_DOMDataStore;
> 
> It's unclear to me what the ownership model for this pointer is, probably because I can't find anyone calling the setter.

This pointer is owned by someone outside V8BindingPerIsolateData. DOMDataStore is not a refcounted object, so RefPtr and such do not apply.

[Adam Barth]

Comment on attachment 100582 [details]
Added field initialization to V8BindingPerIsolateData constructor

Ok.  Looks like this pointer is going to have a complicated ownership model.  Maybe we should add a comment explaining the ownership model?

[Dmitry Lomov]

Commited in http://trac.webkit.org/changeset/90931.
Closing bug.