Bug 63041 - [Chromium][V8] Make DOMDataStore per-isolate
Summary: [Chromium][V8] Make DOMDataStore per-isolate
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dmitry Lomov
URL:
Keywords:
Depends on:
Blocks: 61016
  Show dependency treegraph
 
Reported: 2011-06-20 20:18 PDT by Dmitry Lomov
Modified: 2011-07-13 11:59 PDT (History)
5 users (show)

See Also:


Attachments
The fix (10.79 KB, patch)
2011-07-12 14:08 PDT, Dmitry Lomov
no flags Details | Formatted Diff | Diff
Added field initialization to V8BindingPerIsolateData constructor (11.25 KB, patch)
2011-07-12 16:19 PDT, Dmitry Lomov
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Lomov 2011-06-20 20:18:08 PDT
Another prerequisite to making WebWorkers per-isolate: make sure that DOMDataStore is accessible from multiple threads
Comment 1 Dmitry Lomov 2011-06-20 20:26:41 PDT
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();
Comment 2 Dmitry Lomov 2011-06-20 20:29:48 PDT
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)
Comment 3 anton muhin 2011-06-21 05:11:12 PDT
(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.
Comment 4 Dmitry Lomov 2011-07-12 14:08:52 PDT
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
Comment 5 Dmitry Lomov 2011-07-12 14:09:50 PDT
Comment on attachment 100559 [details]
The fix

(no cq yet - waiting for chromium trybots)
Comment 6 Dmitry Lomov 2011-07-12 15:02:10 PDT
Comment on attachment 100559 [details]
The fix

Errors on trybots.
Comment 7 Dmitry Lomov 2011-07-12 16:19:54 PDT
Created attachment 100582 [details]
Added field initialization to V8BindingPerIsolateData constructor
Comment 8 Dmitry Lomov 2011-07-12 19:40:10 PDT
Comment on attachment 100582 [details]
Added field initialization to V8BindingPerIsolateData constructor

Chromium trybots are happy.
Comment 9 Adam Barth 2011-07-12 19:45:02 PDT
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.
Comment 10 Dmitry Lomov 2011-07-12 20:21:53 PDT
(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.
Comment 11 Adam Barth 2011-07-12 20:26:45 PDT
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?
Comment 12 Dmitry Lomov 2011-07-13 11:59:00 PDT
Commited in http://trac.webkit.org/changeset/90931.
Closing bug.