Bug 207931 – [macOS] Disable RunLoop function dispatch when there is a pending rendering update

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Antti Koivisto]

Avoid processing async IPC until rendering update happens.

[Antti Koivisto]

Created attachment 391149 [details]
wip

[Antti Koivisto]

Created attachment 391154 [details]
patch

[Antti Koivisto]

Created attachment 391156 [details]
patch

[Antti Koivisto]

Created attachment 391158 [details]
patch

[Antti Koivisto]

The patch doesn't affect WK1, the failure is a flake.

[Ben Nham]

Comment on attachment 391158 [details]
patch

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

> Source/WebCore/dom/WindowEventLoop.h:85
> +    static bool m_hasPendingRenderingUpdate;

Should this be a member variable rather than a static variable?

[Antti Koivisto]

> Should this be a member variable rather than a static variable?

Maybe later. For now this is simplest and waiting-for-flush really is a process global state.

[Ben Nham]

(In reply to Antti Koivisto from comment #7)
> > Should this be a member variable rather than a static variable?
> 
> Maybe later. For now this is simplest and waiting-for-flush really is a
> process global state.

I see. Maybe make it use the s_ prefix rather than m_ prefix then? It seems like we follow that pattern for static variables in a lot of places.

[Antti Koivisto]

> I see. Maybe make it use the s_ prefix rather than m_ prefix then? It seems
> like we follow that pattern for static variables in a lot of places.

Ah right. Good point.

[Antti Koivisto]

Created attachment 391183 [details]
patch

[Simon Fraser (smfr)]

Comment on attachment 391158 [details]
patch

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

> Source/WTF/wtf/RunLoop.h:187
> +    bool m_isFunctionDispatchSuspended { false };

Should this be separate from a 'has pending rendering update?

> Source/WTF/wtf/cf/RunLoopCF.cpp:67
>  RunLoop::CycleResult RunLoop::cycle(RunLoopMode mode)
>  {
> +    main().setFunctionDispatchSuspended(false);

Can this be called on any runloop? Why would, say, the scrolling thread runloop care about whether function dispatch is enabled on the main runloop?

> Source/WebCore/dom/WindowEventLoop.cpp:173
> +    if (m_hasPendingRenderingUpdate) {
> +        // FIXME: Also bail out from the task loop in EventLoop::run().
> +        threadGlobalData().threadTimers().breakFireLoopForRenderingUpdate();
> +    }
>  
> +    RunLoop::main().setFunctionDispatchSuspended(m_hasPendingRenderingUpdate);

awkward that these are different.

>> Source/WebCore/dom/WindowEventLoop.h:85
>> +    static bool m_hasPendingRenderingUpdate;
> 
> Should this be a member variable rather than a static variable?

Agreed; using the m_ prefix seems like it will lead to coding errors in future.

[Geoffrey Garen]

Comment on attachment 391183 [details]
patch

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

> Source/WTF/wtf/RunLoop.cpp:105
> +            if (m_isFunctionDispatchSuspended)
> +                return;

It's kind of scary to allow ourselves to get into a permanently disabled state (if someone forgets to make the balancing call to setFunctionDispatchSuspended()).

Instead of a persistent setting, can we make this a one-shot behavior that automatically clears itself and then re-schedules a wakeUp()? I believe that would be sufficient to allow any scheduled rendering update to complete in the current runloop iteration.

We could call this deferring the next runloop iteration, rather than suspending it entirely.

[Antti Koivisto]

> Should this be separate from a 'has pending rendering update?

I didn't want to push the concept of rendering update to WTF.

> Can this be called on any runloop? Why would, say, the scrolling thread
> runloop care about whether function dispatch is enabled on the main runloop?

It is only used (and probably useful) for JS debugger.

> awkward that these are different.

Different abstraction levels. I don't find that particularly awkward.

> Agreed; using the m_ prefix seems like it will lead to coding errors in
> future.

Fixed with s_

[Antti Koivisto]

> It's kind of scary to allow ourselves to get into a permanently disabled
> state (if someone forgets to make the balancing call to
> setFunctionDispatchSuspended()).
> 
> Instead of a persistent setting, can we make this a one-shot behavior that
> automatically clears itself and then re-schedules a wakeUp()? I believe that
> would be sufficient to allow any scheduled rendering update to complete in
> the current runloop iteration.
> 
> We could call this deferring the next runloop iteration, rather than
> suspending it entirely.

I thought about it. The cost is that we may unnecessarily skip the cycle after the rendering update has already happened (since we won't know if the current one will in fact end up waking up). I'm not sure if that is a problem in practice. Maybe not.

[Geoffrey Garen]

> > We could call this deferring the next runloop iteration, rather than
> > suspending it entirely.
> 
> I thought about it. The cost is that we may unnecessarily skip the cycle
> after the rendering update has already happened (since we won't know if the
> current one will in fact end up waking up). I'm not sure if that is a
> problem in practice. Maybe not.

Maybe the act of deferring could also schedule a wakeup? That would eagerly clear the deferred state at the end of the runloop, so that future wakeups would not be delayed.

[Antti Koivisto]

Yeah, that would work. I’ll do that.

[Simon Fraser (smfr)]

Comment on attachment 391183 [details]
patch

I'm a bit concerned that we're repeating the mistake of layer tree freezing and all the timers, and putting throttling/event loop logic in too many places.

Can we move the calls into WebCore::WindowEventLoop into WebCore (maybe via something on Page)? Maybe start to put all the throttling logic in one place.

[Antti Koivisto]

Created attachment 391267 [details]
patch

[Antti Koivisto]

Now with less state.

[Antti Koivisto]

Created attachment 391275 [details]
patch

[Antti Koivisto]

Created attachment 391276 [details]
patch

[Antti Koivisto]

Created attachment 391279 [details]
patch

[Antti Koivisto]

Created attachment 391281 [details]
patch

[Geoffrey Garen]

Comment on attachment 391281 [details]
patch

r=me

I do think it would be nice to establish a central controller that manages all layout/paint "throttling". That said, I think this behavior is a little lower level, and probably something that central controller will call into. This behavior is about acknowledging that some system feature needs to run in the runloop without being preempted by other expensive WebKit runloop work.

[Simon Fraser (smfr)]

Comment on attachment 391281 [details]
patch

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

> Source/WTF/wtf/RunLoop.cpp:145
> +    // Wake up even if there is nothing to do to disable suspension.

"to do to" Maybe Wake up (even if there is nothing to do) to disable suspension.

[Antti Koivisto]

For this sort of things I think WindowEventLoop (that is the HTML event loop) is the right central place as we'll eventually want to handle all task scheduling activities there (including IPC, networking, timers,...).

Throttling is a mostly unrelated concept.

[WebKit Commit Bot]

Comment on attachment 391281 [details]
patch

Clearing flags on attachment: 391281

Committed r257072: <https://trac.webkit.org/changeset/257072>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59636696>

[Antti Koivisto]

Comment fix https://trac.webkit.org/r257076