Summary: | Remove "multi-threaded" logic in V8 DOMData, DOMDataStore and friends | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dmitry Lomov <dslomov> | ||||||||||||||||
Component: | WebCore JavaScript | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | abarth, abecsi, commit-queue, dimich, dslomov, eric, levin, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | 62197, 62233 | ||||||||||||||||||
Bug Blocks: | 61016 | ||||||||||||||||||
Attachments: |
|
Description
Dmitry Lomov
2011-06-06 16:05:09 PDT
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. |