Bug 62164

Created attachment 96147 [details] Removing multithreaded logic

Adam, I'm hoping you'll review this!

I can totally review this, I know this code.

Comment on attachment 96147 [details] Removing multithreaded logic Great patch, since you want to use commit-queue, lets make it perfect: could you please add a sentence to ChangeLog explaining that this is an old code from Lockers-based implementation of WebWorkers in V8 bindings, to make sure that DOM objects are released on the right thread even though GC could have happened on any thread. It is currently unused (since current model is one worker per process) and is being removed because new implementation will be using V8 isolates and does not need the DOMStores per thread. Just to give it more context...

Created attachment 96159 [details] CR feedback - more context in ChangeLog

Comment on attachment 96159 [details] CR feedback - more context in ChangeLog View in context: https://bugs.webkit.org/attachment.cgi?id=96159&action=review Remove this lock probably removes the need for the visitor pattern, but maybe it's worth keeping anyway. > Source/WebCore/bindings/v8/DOMData.cpp:-55 > - DEFINE_STATIC_LOCAL(WTF::ThreadSpecific<ChildThreadDOMData>, childThreadDOMData, ()); Can we just remove ChildThreadDOMData at this point?

I'll let dimich review the updated patch.

Comment on attachment 96159 [details] CR feedback - more context in ChangeLog View in context: https://bugs.webkit.org/attachment.cgi?id=96159&action=review >> Source/WebCore/bindings/v8/DOMData.cpp:-55 >> - DEFINE_STATIC_LOCAL(WTF::ThreadSpecific<ChildThreadDOMData>, childThreadDOMData, ()); > > Can we just remove ChildThreadDOMData at this point? Good point.

Comment on attachment 96159 [details] CR feedback - more context in ChangeLog Adam has inspired me to do a better review :-) Lets indeed remove all pieces that are not needed anymore: - Comment in DOMDataStore.cpp, that explains threading considerations should be updated. - in V8DOMMap.*, lets rename methods like 'visitActiveDOMObjectsInCurrentThread' to 'visitActiveDOMObjects', since they are not looking at thread anymore. - As Adam noted, ChildThreadDOMData can go away - but at the same time MainThreadDOMData can go away as well, so there is only single DOMData class.

Created attachment 96188 [details] More cleanup + renames

Attachment 96188 [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/DOMData.cpp:63: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 96189 [details] Updated threading comment in DOMDataStore.cpp This patch generates bogus style error (comparison with 0 inside UNLIKELY)

Attachment 96189 [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/DOMData.cpp:63: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #12) > Created an attachment (id=96189) [details] > Updated threading comment in DOMDataStore.cpp > > This patch generates bogus style error (comparison with 0 inside UNLIKELY) 1. You could use !!a instead of a != 0. 2. More importantly, UNLIKELY only appears to be defined for GCC. I think you can get the same result by reversing the if. if (!context) return m_defaultStore; return *context->world()->domDataStore(); (You could still add LIKELY here if it was useful.)

Comment on attachment 96189 [details] Updated threading comment in DOMDataStore.cpp View in context: https://bugs.webkit.org/attachment.cgi?id=96189&action=review This looks great. There's probably another round of cleanup we can do here, but we can do that in a subsequent patch. > Source/WebCore/bindings/v8/DOMData.cpp:74 > + > + This diff seem unneeded. > Source/WebCore/bindings/v8/DOMData.h:54 > - virtual DOMDataStore& getStore() = 0; > + DOMDataStore& getStore() { return getMainThreadStore(); } Now that this function isn't virtual, it seems like we don't need to separate these calls, right? > Source/WebCore/bindings/v8/DOMData.h:62 > - ThreadIdentifier owningThread() const { return m_owningThread; } > + static DOMDataStore& getCurrentMainThreadStore() { return getCurrent()->getMainThreadStore(); } Do we still need the concept of "MainThreadStore" or can we remove all mentions of that too now?

Comment on attachment 96189 [details] Updated threading comment in DOMDataStore.cpp Clearing flags on attachment: 96189 Committed r88221: <http://trac.webkit.org/changeset/88221>

All reviewed patches have been landed. Closing bug.

Build fix landed in http://trac.webkit.org/changeset/88224.

Comment on attachment 96189 [details] Updated threading comment in DOMDataStore.cpp Nice!

(In reply to comment #15) > (From update of attachment 96189 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96189&action=review > > This looks great. There's probably another round of cleanup we can do here, but we can do that in a subsequent patch. > > > Source/WebCore/bindings/v8/DOMData.cpp:74 > > + > > + > > This diff seem unneeded. > > > Source/WebCore/bindings/v8/DOMData.h:54 > > - virtual DOMDataStore& getStore() = 0; > > + DOMDataStore& getStore() { return getMainThreadStore(); } > > Now that this function isn't virtual, it seems like we don't need to separate these calls, right? > > > Source/WebCore/bindings/v8/DOMData.h:62 > > - ThreadIdentifier owningThread() const { return m_owningThread; } > > + static DOMDataStore& getCurrentMainThreadStore() { return getCurrent()->getMainThreadStore(); } > > Do we still need the concept of "MainThreadStore" or can we remove all mentions of that too now? All this is going to change soon when we will have one isolate per V8IsolatedWorld.

(In reply to comment #14) > (In reply to comment #12) > > Created an attachment (id=96189) [details] [details] > > Updated threading comment in DOMDataStore.cpp > > > > This patch generates bogus style error (comparison with 0 inside UNLIKELY) > > 1. You could use !!a instead of a != 0. > 2. More importantly, UNLIKELY only appears to be defined for GCC. > > I think you can get the same result by reversing the if. > > if (!context) > return m_defaultStore; > return *context->world()->domDataStore(); > > (You could still add LIKELY here if it was useful.) I think we should change the style tool to be aware of LIKELY and UNLIKELY.

(In reply to comment #18) > Build fix landed in http://trac.webkit.org/changeset/88224. Another build fix for the debug build landed in: http://trac.webkit.org/changeset/88231

(In reply to comment #21) > > (You could still add LIKELY here if it was useful.) > > I think we should change the style tool to be aware of LIKELY and UNLIKELY. Please file a bug and CC dave levin. Hacking cpp_style.py is quite easy.

Patch has broken chromium debug tests

Created attachment 96342 [details] Fixing the chromium tests and chromium debug build issues This patch removes all ASSERT(WTF::isMainThread()), since chromium worker processes do not execute worker scripts on the WTF main thread. I am not asking for commit queue because I want to push this patch through Chromium trybots as well, but will appreciate the review

Created attachment 96363 [details] Rebased patch

Comment on attachment 96363 [details] Rebased patch This patch passes chromium trybots

Created attachment 96367 [details] Removes one more WTF::isMainThread assertion

Comment on attachment 96367 [details] Removes one more WTF::isMainThread assertion Clearing flags on attachment: 96367 Committed r88357: <http://trac.webkit.org/changeset/88357>

All reviewed patches have been landed. Closing bug.