Bug 62164

Summary: Remove "multi-threaded" logic in V8 DOMData, DOMDataStore and friends
Product: WebKit Reporter: Dmitry Lomov <dslomov>
Component: WebCore JavaScriptAssignee: 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 Flags
Removing multithreaded logic
dimich: review-
CR feedback - more context in ChangeLog
dimich: review-
More cleanup + renames
dslomov: review-, dslomov: commit-queue-
Updated threading comment in DOMDataStore.cpp
none
Fixing the chromium tests and chromium debug build issues
none
Rebased patch
none
Removes one more WTF::isMainThread assertion none

Description Dmitry Lomov 2011-06-06 16:05:09 PDT
This functionality is untested and unused.
Comment 1 Dmitry Lomov 2011-06-06 16:18:46 PDT
Created attachment 96147 [details]
Removing multithreaded logic
Comment 2 David Levin 2011-06-06 16:30:22 PDT
Adam, I'm hoping you'll review this!
Comment 3 Dmitry Titov 2011-06-06 17:03:23 PDT
I can totally review this, I know this code.
Comment 4 Dmitry Titov 2011-06-06 17:21:13 PDT
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...
Comment 5 Dmitry Lomov 2011-06-06 17:27:04 PDT
Created attachment 96159 [details]
CR feedback - more context in ChangeLog
Comment 6 Adam Barth 2011-06-06 17:35:24 PDT
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?
Comment 7 Adam Barth 2011-06-06 17:35:44 PDT
I'll let dimich review the updated patch.
Comment 8 Dmitry Lomov 2011-06-06 17:40:37 PDT
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 9 Dmitry Titov 2011-06-06 17:53:31 PDT
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.
Comment 10 Dmitry Lomov 2011-06-06 20:31:31 PDT
Created attachment 96188 [details]
More cleanup + renames
Comment 11 WebKit Review Bot 2011-06-06 20:33:40 PDT
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.
Comment 12 Dmitry Lomov 2011-06-06 20:40:07 PDT
Created attachment 96189 [details]
Updated threading comment in DOMDataStore.cpp

This patch generates bogus style error (comparison with 0 inside UNLIKELY)
Comment 13 WebKit Review Bot 2011-06-06 20:41:31 PDT
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.
Comment 14 David Levin 2011-06-06 22:05:39 PDT
(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 15 Adam Barth 2011-06-06 22:21:46 PDT
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 16 WebKit Commit Bot 2011-06-06 23:19:22 PDT
Comment on attachment 96189 [details]
Updated threading comment in DOMDataStore.cpp

Clearing flags on attachment: 96189

Committed r88221: <http://trac.webkit.org/changeset/88221>
Comment 17 WebKit Commit Bot 2011-06-06 23:19:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Andras Becsi 2011-06-07 01:28:33 PDT
Build fix landed in http://trac.webkit.org/changeset/88224.
Comment 19 anton muhin 2011-06-07 06:44:21 PDT
Comment on attachment 96189 [details]
Updated threading comment in DOMDataStore.cpp

Nice!
Comment 20 Dmitry Lomov 2011-06-07 08:02:43 PDT
(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.
Comment 21 Dmitry Lomov 2011-06-07 08:03:42 PDT
(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.
Comment 22 Andras Becsi 2011-06-07 08:07:26 PDT
(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
Comment 23 Eric Seidel (no email) 2011-06-07 08:42:05 PDT
(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.
Comment 24 Dmitry Lomov 2011-06-07 15:39:26 PDT
Patch has broken chromium debug tests
Comment 25 Dmitry Lomov 2011-06-07 17:39:43 PDT
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
Comment 26 Dmitry Lomov 2011-06-07 19:51:17 PDT
Created attachment 96363 [details]
Rebased patch
Comment 27 Dmitry Lomov 2011-06-07 19:51:48 PDT
Comment on attachment 96363 [details]
Rebased patch

This patch passes chromium trybots
Comment 28 Dmitry Lomov 2011-06-07 20:30:55 PDT
Created attachment 96367 [details]
Removes one more WTF::isMainThread assertion
Comment 29 WebKit Review Bot 2011-06-08 10:31:15 PDT
Comment on attachment 96367 [details]
Removes one more WTF::isMainThread assertion

Clearing flags on attachment: 96367

Committed r88357: <http://trac.webkit.org/changeset/88357>
Comment 30 WebKit Review Bot 2011-06-08 10:31:22 PDT
All reviewed patches have been landed.  Closing bug.