Avoid processing async IPC until rendering update happens.
Created attachment 391149 [details] wip
Created attachment 391154 [details] patch
Created attachment 391156 [details] patch
Created attachment 391158 [details] patch
The patch doesn't affect WK1, the failure is a flake.
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?
> 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.
(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.
> 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.
Created attachment 391183 [details] patch
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.
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.
> 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_
> 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.
> > 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.
Yeah, that would work. I’ll do that.
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.
Created attachment 391267 [details] patch
Now with less state.
Created attachment 391275 [details] patch
Created attachment 391276 [details] patch
Created attachment 391279 [details] patch
Created attachment 391281 [details] patch
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.
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.
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.
Comment on attachment 391281 [details] patch Clearing flags on attachment: 391281 Committed r257072: <https://trac.webkit.org/changeset/257072>
All reviewed patches have been landed. Closing bug.
<rdar://problem/59636696>
Comment fix https://trac.webkit.org/r257076