Bug 62164 – Remove "multi-threaded" logic in V8 DOMData, DOMDataStore and friends

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Dmitry Lomov]

This functionality is untested and unused.

[Dmitry Lomov]

Created attachment 96147 [details]
Removing multithreaded logic

[David Levin]

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

[Dmitry Titov]

I can totally review this, I know this code.

[Dmitry Titov]

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...

[Dmitry Lomov]

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

[Adam Barth]

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?

[Adam Barth]

I'll let dimich review the updated patch.

[Dmitry Lomov]

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.

[Dmitry Titov]

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.

[Dmitry Lomov]

Created attachment 96188 [details]
More cleanup + renames

[WebKit Review Bot]

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.

[Dmitry Lomov]

Created attachment 96189 [details]
Updated threading comment in DOMDataStore.cpp

This patch generates bogus style error (comparison with 0 inside UNLIKELY)

[WebKit Review Bot]

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.

[David Levin]

(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.)

[Adam Barth]

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?

[WebKit Commit Bot]

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

Clearing flags on attachment: 96189

Committed r88221: <http://trac.webkit.org/changeset/88221>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Andras Becsi]

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

[anton muhin]

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

Nice!

[Dmitry Lomov]

(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.

[Dmitry Lomov]

(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.

[Andras Becsi]

(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

[Eric Seidel (no email)]

(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.

[Dmitry Lomov]

Patch has broken chromium debug tests

[Dmitry Lomov]

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

[Dmitry Lomov]

Created attachment 96363 [details]
Rebased patch

[Dmitry Lomov]

Comment on attachment 96363 [details]
Rebased patch

This patch passes chromium trybots

[Dmitry Lomov]

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

[WebKit Review Bot]

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

Clearing flags on attachment: 96367

Committed r88357: <http://trac.webkit.org/changeset/88357>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.