Another prerequisite to making WebWorkers per-isolate: make sure that DOMDataStore is accessible from multiple threads
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();
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)
(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.
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 on attachment 100559 [details] The fix (no cq yet - waiting for chromium trybots)
Comment on attachment 100559 [details] The fix Errors on trybots.
Created attachment 100582 [details] Added field initialization to V8BindingPerIsolateData constructor
Comment on attachment 100582 [details] Added field initialization to V8BindingPerIsolateData constructor Chromium trybots are happy.
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.
(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 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?
Commited in http://trac.webkit.org/changeset/90931. Closing bug.