Bug 204263 – Share more code between WindowEventLoop and WorkerEventLoop by introducing EventLoopTaskGroup

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

WindowEventLoop is currently aware of multiple Document owning a group of tasks.
This logic is useful for grouping a set of tasks and canceling or suspending them all at once.

Extract this grouping logic out of WindowEventLoop, and share most of event loop logic with WorkerEventLoop.

[Ryosuke Niwa]

Created attachment 383674 [details]
Does refactoring

[Ryosuke Niwa]

Created attachment 383679 [details]
Fixed worker tests

[Ryosuke Niwa]

Created attachment 383692 [details]
Fixed the tests

[Ryosuke Niwa]

Ping reviewers.

[Antti Koivisto]

Comment on attachment 383692 [details]
Fixed the tests

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

> Source/WebCore/ChangeLog:15
> +        Each task is now represented as an instance of a concrete subclass of EventLoopTask, which has
> +        a pure virtual execute() member function. Its primary purpose is to know EventLoopTaskGroup to which
> +        each task belongs. One of subclasss, EventLoopFunctionDispatchTask, is used to store a callback function
> +        and another subclass, ActiveDOMObjectEventDispatchTask, is used to dispatch an async event.

Why all this subclassing? Are there likely be many more subclasses in the future? What is wrong with 'if'/'switch'/Variant<>?

> Source/WebCore/dom/AbstractEventLoop.h:66
> +class AbstractEventLoop : public RefCounted<AbstractEventLoop>, public CanMakeWeakPtr<AbstractEventLoop> {

AbstractEventLoop is not very abstract at all anymore. Could it be named into something more descriptive (like just EventLoop)? Or is this a spec name?

> Source/WebCore/dom/AbstractEventLoop.h:73
> +    void suspendGroup(EventLoopTaskGroup&);

This appears to have no callers.

> Source/WebCore/dom/AbstractEventLoop.h:88
> +    // Use a global queue instead of multiple task queues since HTML5 spec allows UA to pick arbitrary queue.
> +    Vector<std::unique_ptr<EventLoopTask>> m_tasks;

I don't understand how the comment relates to the line below. Are you describing some possible alternative implementation strategy?

> Source/WebCore/dom/AbstractEventLoop.h:120
> +        ASSERT(m_state != State::Stopped);
> +        m_state = State::Suspended;
> +        // EventLoop doesn't do anything upon suspension.

Might be better to describe what this does (suspends when trying to run) rather than what it (obviously) doesn't do.

> Source/WebCore/dom/WindowEventLoop.cpp:81
> +void AbstractEventLoop::resumeGroup(EventLoopTaskGroup& group)
>  {
> -    ASSERT(isMainThread());
> -    if (!m_documentIdentifiersForSuspendedTasks.contains(document.identifier()))
> +    ASSERT(isContextThread());
> +    if (!m_groupsWithSuspenedTasks.contains(group))
>          return;
>      scheduleToRunIfNeeded();
>  }

I'm wondering if m_groupsWithSuspenedTasks optimization is even needed. Would it really hurt to scheduleToRunIfNeeded() unconditionally? Would simplify the logic...

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

Bit weird that eventLoop() doesn't return an event loop, but ok.

[Antti Koivisto]

Comment on attachment 383692 [details]
Fixed the tests

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

> Source/WebCore/dom/AbstractEventLoop.h:141
> +    void queueTask(std::unique_ptr<EventLoopTask>&& task)
> +    {
> +        if (m_state == State::Stopped || !m_eventLoop)
> +            return;
> +        ASSERT(!task->m_group);
> +        task->m_group = makeWeakPtr(*this);
> +        m_eventLoop->queueTask(WTFMove(task));
> +    }

There is an alternative factoring where group would be provided to EventLoopTask as a constructor parameter instead of being set here. This might lead to cleaner separation of functionality between AbstractEventLoop and EventLoopTaskGroup. AbstractEventLoop would offer the queuing interface while suspend/resume would be in EventLoopTaskGroup.

Not sure if it would end up looking better but I find the approach taken in this patch slightly confusing.

[Ryosuke Niwa]

Thanks for the review!

(In reply to Antti Koivisto from comment #5)
> Comment on attachment 383692 [details]
> Fixed the tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383692&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        Each task is now represented as an instance of a concrete subclass of EventLoopTask, which has
> > +        a pure virtual execute() member function. Its primary purpose is to know EventLoopTaskGroup to which
> > +        each task belongs. One of subclasss, EventLoopFunctionDispatchTask, is used to store a callback function
> > +        and another subclass, ActiveDOMObjectEventDispatchTask, is used to dispatch an async event.
> 
> Why all this subclassing? Are there likely be many more subclasses in the
> future? What is wrong with 'if'/'switch'/Variant<>?

We want to make it possible for JSC to hook into this so we don't want AbstractEventLoop or EventLoopTaskGroup to depend on things like Event or ActiveDOMObjet, which are WebCore concepts.

> 
> > Source/WebCore/dom/AbstractEventLoop.h:66
> > +class AbstractEventLoop : public RefCounted<AbstractEventLoop>, public CanMakeWeakPtr<AbstractEventLoop> {
> 
> AbstractEventLoop is not very abstract at all anymore. Could it be named
> into something more descriptive (like just EventLoop)? Or is this a spec
> name?

Yes, I'm going to rename it to EventLoop in a follow up patch. I didn't want to make the patch unreadable by doing all the renames & refactoring at once.

> 
> > Source/WebCore/dom/AbstractEventLoop.h:73
> > +    void suspendGroup(EventLoopTaskGroup&);
> 
> This appears to have no callers.

Removed.

> > Source/WebCore/dom/AbstractEventLoop.h:88
> > +    // Use a global queue instead of multiple task queues since HTML5 spec allows UA to pick arbitrary queue.
> > +    Vector<std::unique_ptr<EventLoopTask>> m_tasks;
> 
> I don't understand how the comment relates to the line below. Are you
> describing some possible alternative implementation strategy?

Right, if you very faithfully implement what the spec says, we're going to have multiple queues (i.e. multiple vectors) from which we'd pick in an arbitrary order.
> > Source/WebCore/dom/AbstractEventLoop.h:120
> > +        ASSERT(m_state != State::Stopped);
> > +        m_state = State::Suspended;
> > +        // EventLoop doesn't do anything upon suspension.
> 
> Might be better to describe what this does (suspends when trying to run)
> rather than what it (obviously) doesn't do.

Good point. This is a useless "what" comment. Revised it to:
// We don't remove suspended tasks to preserve the ordering.
// AbstractEventLoop::run checks whether each task's group is suspended or not.

> > Source/WebCore/dom/WindowEventLoop.cpp:81
> > +void AbstractEventLoop::resumeGroup(EventLoopTaskGroup& group)
> >  {
> > -    ASSERT(isMainThread());
> > -    if (!m_documentIdentifiersForSuspendedTasks.contains(document.identifier()))
> > +    ASSERT(isContextThread());
> > +    if (!m_groupsWithSuspenedTasks.contains(group))
> >          return;
> >      scheduleToRunIfNeeded();
> >  }
> 
> I'm wondering if m_groupsWithSuspenedTasks optimization is even needed.
> Would it really hurt to scheduleToRunIfNeeded() unconditionally? Would
> simplify the logic...

Media elements, etc... are going to use this mechanism so I'd like to keep this optimization.
One important optimization I really have to implement is to put the suspended tasks
that AbstractEventLoop::run had already processed to a different queue until they're resumed.

Otherwise, it could result in a very pathological scenario when there are multiple windows' involved.
e.g. a.com in window 1 opens another page in a.com in window 2. Then window 1 navigates to b.com.
In this scenario, a.com in window 1 is suspended but a.com in window 2 is not suspended.
window 1 had hundreds of tasks suspended, then we'd have to go through every one of them each time any task is scheduled.

This whole suspended task check in WindowEventLoop was added to implement that optimization
but I haven't had a chance to implement it since I couldn't come up with a way to test it well.
I think I have a decent testing strategy now so I'd go write a patch after this.

> > Source/WebCore/workers/WorkerGlobalScope.h:70
> > -    AbstractEventLoop& eventLoop() final;
> > +    EventLoopTaskGroup& eventLoop() final;
> 
> Bit weird that eventLoop() doesn't return an event loop, but ok.

Yeah, I thought about renaming this to eventLoopTestGroup but I didn't want everyone using eventLoop() to think about EventLoopTestGroup.

[Ryosuke Niwa]

(In reply to Antti Koivisto from comment #6)
> Comment on attachment 383692 [details]
> Fixed the tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383692&action=review
> 
> > Source/WebCore/dom/AbstractEventLoop.h:141
> > +    void queueTask(std::unique_ptr<EventLoopTask>&& task)
> > +    {
> > +        if (m_state == State::Stopped || !m_eventLoop)
> > +            return;
> > +        ASSERT(!task->m_group);
> > +        task->m_group = makeWeakPtr(*this);
> > +        m_eventLoop->queueTask(WTFMove(task));
> > +    }
> 
> There is an alternative factoring where group would be provided to
> EventLoopTask as a constructor parameter instead of being set here. This
> might lead to cleaner separation of functionality between AbstractEventLoop
> and EventLoopTaskGroup. AbstractEventLoop would offer the queuing interface
> while suspend/resume would be in EventLoopTaskGroup.

Oh, that's a good idea. EventLoopTaskGroup::queueTask which takes a function will end up using `*this` as an argument to EventLoopFunctionDispatchTask's constructor but this is still way cleaner. EventLoopTask no longer has to have EventLoopTaskGroup as a friend class. Not sure why I didn't think of this.

FYI, you can also see other variants on https://bugs.webkit.org/show_bug.cgi?id=203667 where I considered using EventTarget or more broadly void* as a grouping mechanism.

[Ryosuke Niwa]

Created attachment 383788 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 383788 [details]
Patch for landing

Wait for EWS.

[Ryosuke Niwa]

Committed r252607: <https://trac.webkit.org/changeset/252607>

[Radar WebKit Bug Importer]

<rdar://problem/57304899>