1461957 - requestIdleCallback might fail to fire during idle periods

Status: RESOLVED INVALID

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Kartikaya Gupta (email:[e-mail])]

While investigating bug 1451305 and trying to understand how the requestIdleCallback code works, I think I spotted a problem in the code. Consider the case where the main thread event queue just has a bunch of idle-priority events in it, but just finished a refresh driver tick. The thread loop tries to get the next event - since the idle queue is non-empty, but we just finished a refresh driver tick, we end up return null from [1]. At this point, let's say the main thread actually becomes idle - no new events are scheduled and the refresh driver doesn't tick.

What is the mechanism by which the idle-priority events in the queue actually fire? In theory after a some milliseconds of doing nothing the conditions for "idleness" should be satisfied, but as far as I can tell, nothing will actually wake up the thread loop to try and process those idle events. It will remain stuck at [2] until another event is queued at which point it will wake up and try to do things.

I could be misunderstanding the code here, and it's possible this scenario cannot occur for some reason that I missed. If so please let me know what that is.

[1] https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/xpcom/threads/PrioritizedEventQueue.cpp#233
[2] https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/xpcom/threads/ThreadEventQueue.cpp#155

Comment 1

[Andreas Farre [:farre]]

What determines if we should start run an idle callback is if the higher prioritized queues are empty, and it is as you say, if GetIdleDeadline returns null when there are events in only the idle queue, then we might wait indefinitely. 

But that this should happen _after_ we've run a refresh driver tick shouldn't happen. If nsRefreshDriver::GetIdleDeadlineHint says that it expects to run again in less than kMinIdlePeriodMS, but that is false (i.e. it has stopped and won't tick), then there is a bug in nsRefreshDriver::GetIdleDeadlineHint. Otherwise actually just finishing a refresh driver tick will mean that nsRefreshDriver::GetIdleDeadlineHint will return something that actually is sufficiently large enough that we would determine that we aren't busy.

That is, it isn't the refresh driver tick that drives the idle periods, it is the event queue. The refresh driver participates by answering the question about how long it thinks until it will tick again.

You don't happen to have a test that exhibits a hanging event queue?

Comment 2

[Kartikaya Gupta (email:[e-mail])]

(In reply to Andreas Farre [:farre] from comment #1)
> But that this should happen _after_ we've run a refresh driver tick
> shouldn't happen. If nsRefreshDriver::GetIdleDeadlineHint says that it
> expects to run again in less than kMinIdlePeriodMS, but that is false (i.e.
> it has stopped and won't tick), then there is a bug in
> nsRefreshDriver::GetIdleDeadlineHint. Otherwise actually just finishing a
> refresh driver tick will mean that nsRefreshDriver::GetIdleDeadlineHint will
> return something that actually is sufficiently large enough that we would
> determine that we aren't busy.
> 
> That is, it isn't the refresh driver tick that drives the idle periods, it
> is the event queue. The refresh driver participates by answering the
> question about how long it thinks until it will tick again.

Ah, so if I'm understanding this correctly, this has a built-in assumption about the refresh interval being larger than kMinIdlePeriodMS. So in the case of ASAP mode (where layout.frame_rate = 0) the refresh interval is actually set to 0.01ms so as to drive refreshes as fast as possible. That's the scenario I was testing under, which might explain some of what I was seeing. That being said, with ASAP mode the refresh driver never actually stops ticking, so I guess it's fair to not actually run the rIC callback in that case.

If that's correct, then the behaviour I was seeing was correct, but the test itself is flawed. Also then there's a problem with the non-WebRender codepath because that one was actually still triggering the rIC callback to run even in ASAP mode.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

s/0.01/0.1ms/ in my previous comment.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> That being said, with ASAP mode the refresh driver never actually
> stops ticking, so I guess it's fair to not actually run the rIC callback in
> that case.

Actually this isn't true. Even with ASAP mode the refresh driver will stop ticking in an idle state. But that wasn't happening for me, which might point to another problem. This is all so confusing :/

Comment 5

[Andreas Farre [:farre]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> Ah, so if I'm understanding this correctly, this has a built-in assumption
> about the refresh interval being larger than kMinIdlePeriodMS. So in the
> case of ASAP mode (where layout.frame_rate = 0) the refresh interval is
> actually set to 0.01ms so as to drive refreshes as fast as possible. That's
> the scenario I was testing under, which might explain some of what I was
> seeing. That being said, with ASAP mode the refresh driver never actually
> stops ticking, so I guess it's fair to not actually run the rIC callback in
> that case.
> 
> If that's correct, then the behaviour I was seeing was correct, but the test
> itself is flawed. Also then there's a problem with the non-WebRender
> codepath because that one was actually still triggering the rIC callback to
> run even in ASAP mode.

Yes, that's a consequence of kMinIdlePeriodMS. If you run the refresh driver faster than that there will be no idle periods. There is an intended way around this, and that is by setting a timeout on the idle callback. If it's the webidl exposed requestIdleCallback that's doable with the API. If it's a dispatched runnable, the dispatch should be done by NS_IdleDispatchToCurrentThread(already_AddRefed<nsIRunnable>&& aEvent, uint32_t aTimeout), which sets up a timer that triggers the callback to ensure that it fires.

In the end, the fact is that it is entirely possible that idle callback may wait forever to run, but it's important to know that a thread won't wait for incoming events when there are events on the idle queue. If you actually do see this somehow I am very interested in fixing that bug.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Thanks, that makes sense.

(In reply to Andreas Farre [:farre] from comment #5)
> it's important to know that a thread won't wait for
> incoming events when there are events on the idle queue. If you actually do
> see this somehow I am very interested in fixing that bug.

I'll let you know if I manage to isolate this behaviour.

One more question, though. Say we're ticking the refresh driver regularly every 16ms, and then we run into a paint that takes 100ms for example. After the 100ms paint finishes, the nsRefreshDriver::GetIdleDeadlineHint call happens. At this point the "most recent refresh" time would be 100ms ago and so the function would go down the "quiescent frames" codepath at [1], and the rIC callback would trigger. This might happen even if we're ticking continuously every 16ms. i.e. a long paint might get mistaken for quiescent frames, and trigger the idle callback when maybe it shouldn't. Is this a possibility, or is there something preventing this from happening? I noticed bug 1332634 and thought this might explain that behaviour.

[1] https://searchfox.org/mozilla-central/rev/d4b9e50875ad7e5d20f2fee6a53418315f6dfcc0/layout/base/nsRefreshDriver.cpp#252

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Ok, I haven't been able to isolate any specific scenario where this actually happens, and I discovered a different problem (specific to WebRender) which was causing the issues I was seeing. Given that I can fix that problem, I'm going to stop my investigation into the rIC behaviour, and will close this bug for now.