Bug 203526 - Introduce WorkerEventLoop and use it in FetchBodyOwner::runNetworkTaskWhenPossible
Summary: Introduce WorkerEventLoop and use it in FetchBodyOwner::runNetworkTaskWhenPos...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks: 203137
  Show dependency treegraph
 
Reported: 2019-10-28 15:28 PDT by Ryosuke Niwa
Modified: 2019-11-05 15:31 PST (History)
14 users (show)

See Also:


Attachments
Patch (23.13 KB, patch)
2019-10-28 15:40 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fix release builds (23.14 KB, patch)
2019-10-28 16:20 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
GTK+ / WPE build fix (23.22 KB, patch)
2019-10-28 16:42 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Updated per review comments (27.15 KB, patch)
2019-10-29 19:11 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Added an assertion (27.21 KB, patch)
2019-10-29 20:47 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (27.15 KB, patch)
2019-10-29 22:20 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2019-10-28 15:28:27 PDT
Introduce WorkerEventLoop class to implement the worker event loop:
https://html.spec.whatwg.org/multipage/webappapis.html#worker-event-loop-2
Comment 1 Ryosuke Niwa 2019-10-28 15:40:06 PDT
Created attachment 382122 [details]
Patch
Comment 2 Ryosuke Niwa 2019-10-28 16:20:03 PDT
Created attachment 382129 [details]
Fix release builds
Comment 3 Ryosuke Niwa 2019-10-28 16:42:15 PDT
Created attachment 382136 [details]
GTK+ / WPE build fix
Comment 4 Ryosuke Niwa 2019-10-28 19:35:01 PDT
Ping reviewers.
Comment 5 Chris Dumez 2019-10-29 09:02:52 PDT
Comment on attachment 382136 [details]
GTK+ / WPE build fix

View in context: https://bugs.webkit.org/attachment.cgi?id=382136&action=review

> Source/WebCore/dom/Document.h:1065
> +    AbstractEventLoop& eventLoop();

It looks like this is an override? If so, why isn't is marked as final? Also, using the tighter WindowEventLoop typing in the override should work and would actually be beneficial.

> Source/WebCore/workers/WorkerGlobalScope.cpp:109
> +{

We should likely assert that this is only called from the context thread.

> Source/WebCore/workers/WorkerGlobalScope.h:69
> +    AbstractEventLoop& eventLoop() final;

Can use WorkerEventLoop return type.

> Source/WebCore/worklets/WorkletGlobalScope.h:61
> +    AbstractEventLoop& eventLoop() final;

Can use tighter type.
Comment 6 Ryosuke Niwa 2019-10-29 16:56:49 PDT
(In reply to Chris Dumez from comment #5)
> Comment on attachment 382136 [details]
> GTK+ / WPE build fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382136&action=review
> 
> > Source/WebCore/dom/Document.h:1065
> > +    AbstractEventLoop& eventLoop();
> 
> It looks like this is an override? If so, why isn't is marked as final?

Good point.

> Also, using the tighter WindowEventLoop typing in the override should work
> and would actually be beneficial.

There is no need for that now but I guess we can do that.

> 
> > Source/WebCore/workers/WorkerGlobalScope.cpp:109
> > +{
> 
> We should likely assert that this is only called from the context thread.

Added.

> > Source/WebCore/workers/WorkerGlobalScope.h:69
> > +    AbstractEventLoop& eventLoop() final;
> 
> Can use WorkerEventLoop return type.

Sure. Again, there isn't any need for that. Specific type isn't necessarily better if it's not useful. In fact, I'd argue that it's bad in this case because it would encourage people to add new functions to WorkerEventLoop and not to AbstractEventLoop.

> > Source/WebCore/worklets/WorkletGlobalScope.h:61
> > +    AbstractEventLoop& eventLoop() final;
> 
> Can use tighter type.

Fixed for now but again, I don't think this is a good idea.
Comment 7 Chris Dumez 2019-10-29 17:00:23 PDT
(In reply to Ryosuke Niwa from comment #6)
> (In reply to Chris Dumez from comment #5)
> > Comment on attachment 382136 [details]
> > GTK+ / WPE build fix
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=382136&action=review
> > 
> > > Source/WebCore/dom/Document.h:1065
> > > +    AbstractEventLoop& eventLoop();
> > 
> > It looks like this is an override? If so, why isn't is marked as final?
> 
> Good point.
> 
> > Also, using the tighter WindowEventLoop typing in the override should work
> > and would actually be beneficial.
> 
> There is no need for that now but I guess we can do that.
> 
> > 
> > > Source/WebCore/workers/WorkerGlobalScope.cpp:109
> > > +{
> > 
> > We should likely assert that this is only called from the context thread.
> 
> Added.
> 
> > > Source/WebCore/workers/WorkerGlobalScope.h:69
> > > +    AbstractEventLoop& eventLoop() final;
> > 
> > Can use WorkerEventLoop return type.
> 
> Sure. Again, there isn't any need for that. Specific type isn't necessarily
> better if it's not useful. In fact, I'd argue that it's bad in this case
> because it would encourage people to add new functions to WorkerEventLoop
> and not to AbstractEventLoop.
> 
> > > Source/WebCore/worklets/WorkletGlobalScope.h:61
> > > +    AbstractEventLoop& eventLoop() final;
> > 
> > Can use tighter type.
> 
> Fixed for now but again, I don't think this is a good idea.

One reason I think it is beneficial is perf. If you do document().eventLoop().queueTask() then no virtual table lookup since you get a WindowEventLoop and WindowEventLoop::queueTask() is marked as final.
Comment 8 Ryosuke Niwa 2019-10-29 17:49:26 PDT
(In reply to Chris Dumez from comment #7)
>
> One reason I think it is beneficial is perf. If you do
> document().eventLoop().queueTask() then no virtual table lookup since you
> get a WindowEventLoop and WindowEventLoop::queueTask() is marked as final.

I guess that's a good point although I don't think it really matters.

Ugh... now I remember why I went with AbstractEventLoop&. It's because if I used WindowEventLoop then I'd have to include WindowEventLoop.h in Document.h which isn't great (since otherwise the complier can't tell WindowEventLoop is covariant with AbstractEventLoop).
Comment 9 Ryosuke Niwa 2019-10-29 18:35:26 PDT
(In reply to Ryosuke Niwa from comment #8)
> (In reply to Chris Dumez from comment #7)
> >
> > One reason I think it is beneficial is perf. If you do
> > document().eventLoop().queueTask() then no virtual table lookup since you
> > get a WindowEventLoop and WindowEventLoop::queueTask() is marked as final.
> 
> I guess that's a good point although I don't think it really matters.
> 
> Ugh... now I remember why I went with AbstractEventLoop&. It's because if I
> used WindowEventLoop then I'd have to include WindowEventLoop.h in
> Document.h which isn't great (since otherwise the complier can't tell
> WindowEventLoop is covariant with AbstractEventLoop).

This would also mean we'd have to export WindowEventLoop.h to WebKit layer as well. I don't think we want to do that. I'm revering back to AbstractEventLoop. It's not worth the trouble & slower compilation time.
Comment 10 Ryosuke Niwa 2019-10-29 19:11:54 PDT
Created attachment 382265 [details]
Updated per review comments
Comment 11 Chris Dumez 2019-10-29 19:40:27 PDT
(In reply to Ryosuke Niwa from comment #10)
> Created attachment 382265 [details]
> Updated per review comments

I don’t see the threading assertion?
Comment 12 Chris Dumez 2019-10-29 19:41:40 PDT
(In reply to Chris Dumez from comment #11)
> (In reply to Ryosuke Niwa from comment #10)
> > Created attachment 382265 [details]
> > Updated per review comments
> 
> I don’t see the threading assertion?

The isContextThread() one I asked for in queueTask(). Or did I miss it?
Comment 13 Ryosuke Niwa 2019-10-29 20:43:24 PDT
(In reply to Chris Dumez from comment #12)
> (In reply to Chris Dumez from comment #11)
> > (In reply to Ryosuke Niwa from comment #10)
> > > Created attachment 382265 [details]
> > > Updated per review comments
> > 
> > I don’t see the threading assertion?
> 
> The isContextThread() one I asked for in queueTask(). Or did I miss it?

Oh, sorry, I misunderstood what you were asking.
Comment 14 Ryosuke Niwa 2019-10-29 20:46:37 PDT
I'm gonna keep the assertion in WorkerGlobalScope::eventLoop too though since it's not safe to call that member function in a wrong thread.
Comment 15 Ryosuke Niwa 2019-10-29 20:47:35 PDT
Created attachment 382274 [details]
Added an assertion
Comment 16 Chris Dumez 2019-10-29 21:03:32 PDT
Comment on attachment 382274 [details]
Added an assertion

View in context: https://bugs.webkit.org/attachment.cgi?id=382274&action=review

r=me with comments

> Source/WebCore/dom/Document.cpp:6132
>  {

We likely want a isMainThread() assertion here too.

> Source/WebCore/workers/WorkerEventLoop.cpp:61
> +bool WorkerEventLoop::hasPendingActivity() const

Why do we need to override this. Presumably, the default implementation already returns false if you don’t use makePendingActivity / setPendingActivity.

> Source/WebCore/workers/WorkerEventLoop.cpp:71
> +void WorkerEventLoop::suspend(ReasonForSuspension)

We don’t suspend / resume ActiveDOMObjects on workers. Instead, we suspend the whole thread. As a result, I don’t think the suspend / resume methods do anything here.

> Source/WebCore/workers/WorkerEventLoop.h:48
> +    WorkerEventLoop(ScriptExecutionContext&);

Explicit
Comment 17 Ryosuke Niwa 2019-10-29 22:16:09 PDT
(In reply to Chris Dumez from comment #16)
> Comment on attachment 382274 [details]
> Added an assertion
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382274&action=review
> 
> r=me with comments
> 
> > Source/WebCore/dom/Document.cpp:6132
> >  {
> 
> We likely want a isMainThread() assertion here too.

Good catch. Added.

> > Source/WebCore/workers/WorkerEventLoop.cpp:61
> > +bool WorkerEventLoop::hasPendingActivity() const
> 
> Why do we need to override this. Presumably, the default implementation
> already returns false if you don’t use makePendingActivity /
> setPendingActivity.

Yeah, I don't recall why I added there. Removed.

> > Source/WebCore/workers/WorkerEventLoop.cpp:71
> > +void WorkerEventLoop::suspend(ReasonForSuspension)
> 
> We don’t suspend / resume ActiveDOMObjects on workers. Instead, we suspend
> the whole thread. As a result, I don’t think the suspend / resume methods do
> anything here.

Oh, sure. I wrote this patch before your patch was landed so I forgot to reflect that. I'd replace the implementation with ASSERT_NOT_REACHED().

> > Source/WebCore/workers/WorkerEventLoop.h:48
> > +    WorkerEventLoop(ScriptExecutionContext&);
> 
> Explicit

Done.
Comment 18 Ryosuke Niwa 2019-10-29 22:20:08 PDT
Created attachment 382281 [details]
Patch for landing
Comment 19 Ryosuke Niwa 2019-10-29 22:20:34 PDT
Comment on attachment 382281 [details]
Patch for landing

Wait for EWS
Comment 20 WebKit Commit Bot 2019-10-30 12:38:02 PDT
Comment on attachment 382281 [details]
Patch for landing

Clearing flags on attachment: 382281

Committed r251792: <https://trac.webkit.org/changeset/251792>
Comment 21 WebKit Commit Bot 2019-10-30 12:38:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Radar WebKit Bug Importer 2019-10-30 12:39:15 PDT
<rdar://problem/56754436>
Comment 23 Chris Dumez 2019-10-31 10:49:09 PDT
Comment on attachment 382281 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=382281&action=review

> Source/WebCore/workers/WorkerEventLoop.cpp:36
> +    return adoptRef(*new WorkerEventLoop(context));

This fails to call suspendIfNeeded().

> Source/WebCore/workers/WorkerEventLoop.cpp:42
> +    return adoptRef(*new WorkerEventLoop(context));

This fails to call suspendIfNeeded().
Comment 24 Ryosuke Niwa 2019-11-05 15:31:37 PST
(In reply to Chris Dumez from comment #23)
> Comment on attachment 382281 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382281&action=review
> 
> > Source/WebCore/workers/WorkerEventLoop.cpp:36
> > +    return adoptRef(*new WorkerEventLoop(context));
> 
> This fails to call suspendIfNeeded().
> 
> > Source/WebCore/workers/WorkerEventLoop.cpp:42
> > +    return adoptRef(*new WorkerEventLoop(context));
> 
> This fails to call suspendIfNeeded().

This got fixed in https://trac.webkit.org/changeset/251934.