Bug 203526 – Introduce WorkerEventLoop and use it in FetchBodyOwner::runNetworkTaskWhenPossible

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Introduce WorkerEventLoop class to implement the worker event loop:
https://html.spec.whatwg.org/multipage/webappapis.html#worker-event-loop-2

[Ryosuke Niwa]

Created attachment 382122 [details]
Patch

[Ryosuke Niwa]

Created attachment 382129 [details]
Fix release builds

[Ryosuke Niwa]

Created attachment 382136 [details]
GTK+ / WPE build fix

[Ryosuke Niwa]

Ping reviewers.

[Chris Dumez]

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.

[Ryosuke Niwa]

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

[Chris Dumez]

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Created attachment 382265 [details]
Updated per review comments

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #10)
> Created attachment 382265 [details]
> Updated per review comments

I don’t see the threading assertion?

[Chris Dumez]

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 382274 [details]
Added an assertion

[Chris Dumez]

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Created attachment 382281 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 382281 [details]
Patch for landing

Wait for EWS

[WebKit Commit Bot]

Comment on attachment 382281 [details]
Patch for landing

Clearing flags on attachment: 382281

Committed r251792: <https://trac.webkit.org/changeset/251792>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56754436>

[Chris Dumez]

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

[Ryosuke Niwa]

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