Closed Bug 1314314 Opened 7 years ago Closed 7 years ago

requestIdleCallback can fire with close to 0ms timeRemaining()

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: bkelly, Assigned: farre)

References

Details

Attachments

(1 file)

Bug 1314314 - Restrict when idle callbacks are fired.
58 bytes, text/x-review-board-request
bkelly
: review+
Details 
Currently requestIdleCallback can fire callbacks with close to 0ms time remaining.  This seems problematic because script doesn't really have anything they can reasonable do.  They can either blow the budget or just re-queue their request.

What is actually happening is we subtract the value of layout.idle_period.time_limit from the actual amount of time we think we have.  If this is greater than zero we fire the callback.  We also expose this adjusted time as the time remaining value.

By default the pref is set to 3ms.  So we will fire callbacks at least 3ms before the next frame.  But the callback will see a time remaining possibly close to 0ms.

Solutions here could be:

1) Don't subtract the pref value from the timeRemaining() exposed to script.  So they would see 3ms with our current pref default.
2) Have a secondary pref that restricts when we fire idle callbacks.  Only fire if the adjusted timeRemaining() is 2ms or greater, for example.  With our current adjustment of 3ms that means we would only fire idle callbacks at least 5ms before the end of the frame.
Assignee: nobody → afarre
Blocks: 1314959
(In reply to Ben Kelly [:bkelly] from comment #0)
> Solutions here could be:
> 
> 1) Don't subtract the pref value from the timeRemaining() exposed to script.
> So they would see 3ms with our current pref default.
> 2) Have a secondary pref that restricts when we fire idle callbacks.  Only
> fire if the adjusted timeRemaining() is 2ms or greater, for example.  With
> our current adjustment of 3ms that means we would only fire idle callbacks
> at least 5ms before the end of the frame.

I think that solution 2 is what we want, but perhaps with some tweaked values. Maybe lower the value that we adjust with and have a larger cutoff before firing rIC? Something like 3 + 1? (Where the 1 is the currently present pref and 3 would be the new one).
That would work for me.  What was the layout pref before rIC?
Status: NEW → ASSIGNED
Comment on attachment 8807233 [details]
Bug 1314314 - Restrict when idle callbacks are fired.

https://reviewboard.mozilla.org/r/90462/#review90176

Thanks.  I think this solves the issue, but I have to admit I found it hard to understand.  r- for right now because I feel like we should consolidate the threshold checking in a single place.

Also, can you write a test for this?  Not sure how easy tests are for this stuff given timing is somewhat racy.

::: modules/libpref/init/all.js:2749
(Diff revision 1)
>  pref("dom.idle_period.throttled_length", 10000);
>  
>  // The amount of idle time (milliseconds) reserved for a long idle period
>  pref("idle_queue.long_period", 50);
>  
> +// The minimum amount of time (milliseconds) required for an idle period

Can you expand this comment to describe that this is in addition to the time required by layout.idle_period.time_limit?

::: xpcom/threads/MainThreadIdlePeriod.cpp:28
(Diff revision 1)
>    Maybe<TimeStamp> deadline = nsRefreshDriver::GetIdleDeadlineHint();
>  
>    if (deadline.isSome()) {
> -    *aIdleDeadline = deadline.value();
> +    *aIdleDeadline =
> +      TimeStamp::Now() <
> +          deadline.value() - TimeDuration::FromMilliseconds(GetMinIdlePeriod())

I think its really confusing to do this check here while the other half of the condition is in nsThread.cpp.

Can you either:

1. Move this check to nsThread.cpp
2. Or move the checks into a helper method on MainThreadIdlePeriod that nsThread can call?

::: xpcom/threads/MainThreadIdlePeriod.cpp:57
(Diff revision 1)
>  }
>  
> +/* static */ float
> +MainThreadIdlePeriod::GetMinIdlePeriod()
> +{
> +  static float sMinIdlePeriod = DEFAULT_MIN_IDLE_PERIOD;

MOZ_ASSERT(NS_IsMainThread())
Attachment #8807233 - Flags: review?(bkelly) → review-
(In reply to Ben Kelly [:bkelly] from comment #4)
> Comment on attachment 8807233 [details]
> Bug 1314314 - Restrict when idle callbacks are fired.
> 
> https://reviewboard.mozilla.org/r/90462/#review90176
> 
> Thanks.  I think this solves the issue, but I have to admit I found it hard
> to understand.  r- for right now because I feel like we should consolidate
> the threshold checking in a single place.
> 
> Also, can you write a test for this?  Not sure how easy tests are for this
> stuff given timing is somewhat racy.

I think that's hard without knowing the frame rate. If we did, then we could do a bunch of rAF that always took up the frame budget - 2.9ms, and then assert that no rIC callbacks ran. But that feels rather fragile even if it was possible.

> ::: modules/libpref/init/all.js:2749
> (Diff revision 1)
> >  pref("dom.idle_period.throttled_length", 10000);
> >  
> >  // The amount of idle time (milliseconds) reserved for a long idle period
> >  pref("idle_queue.long_period", 50);
> >  
> > +// The minimum amount of time (milliseconds) required for an idle period
> 
> Can you expand this comment to describe that this is in addition to the time
> required by layout.idle_period.time_limit?

Will do.

> ::: xpcom/threads/MainThreadIdlePeriod.cpp:28
> (Diff revision 1)
> >    Maybe<TimeStamp> deadline = nsRefreshDriver::GetIdleDeadlineHint();
> >  
> >    if (deadline.isSome()) {
> > -    *aIdleDeadline = deadline.value();
> > +    *aIdleDeadline =
> > +      TimeStamp::Now() <
> > +          deadline.value() - TimeDuration::FromMilliseconds(GetMinIdlePeriod())
> 
> I think its really confusing to do this check here while the other half of
> the condition is in nsThread.cpp.
> 
> Can you either:
> 
> 1. Move this check to nsThread.cpp
> 2. Or move the checks into a helper method on MainThreadIdlePeriod that
> nsThread can call?

I have a couple of reasons for having it in MainThreadIdlePeriod.

One is that MainThreadIdlePeriod captures how we imagine that we handle deadlines on the main thread. It might be that on another type of thread, where we want to do idle processing, jank isn't an issue (or something similarly hypothetical. I'm reaching a bit for it her, but this was the idea of the nsIIdlePeriod interface to begin with). 

Another is that this also makes the prefs live outside of nsThread, keeping nsThread code simpler. Having a helper in MainThreadIdlePeriod also solves this, but then I would probably need to expose a method on nsIIdlePeriod that allows us calling that helper without knowledge of the type of idle period.

Also, the documentation in nsIIdlePeriod.idl mentions that it is expected of getIdlePeriodHint to return TimeStamp() (i.e. the null time) if it is expected that the thread will become busy soonish. So in a way, the threshold checking is consolidated to the degree that the expected behaviour of the main thread's threshold is consolidated to MainThreadIdlePeriod::GetIdlePeriodHint.

Would it be OK to keep it as it is if I also add a documentation snippet about that as well? Something like:

"restrict idle periods to be longer than idle_queue.min_period by requiring that deadline is more than idle_queue.min_period ms into the future"

> ::: xpcom/threads/MainThreadIdlePeriod.cpp:57
> (Diff revision 1)
> >  }
> >  
> > +/* static */ float
> > +MainThreadIdlePeriod::GetMinIdlePeriod()
> > +{
> > +  static float sMinIdlePeriod = DEFAULT_MIN_IDLE_PERIOD;
> 
> MOZ_ASSERT(NS_IsMainThread())

Right.
(In reply to Andreas Farre [:farre] from comment #5)
> Also, the documentation in nsIIdlePeriod.idl mentions that it is expected of
> getIdlePeriodHint to return TimeStamp() (i.e. the null time) if it is
> expected that the thread will become busy soonish. So in a way, the
> threshold checking is consolidated to the degree that the expected behaviour
> of the main thread's threshold is consolidated to
> MainThreadIdlePeriod::GetIdlePeriodHint.

Hmm, ok.  I guess I didn't see the nsIIdlePeriod API docs.

But can you maybe change the code in MainThreadIdlePeriod to something like:

  // If the idle period is too small, then just return a null time to indicate we
  // we are busy.  Otherwise return the actual deadline.
  TimeDuration minIdlePeriod = TimeDuration::FromMilliseconds(GetMinIdlePeriod());
  bool busySoon = TimeStamp::Now() >= deadline.value() - minIdlePeriod;
  *aIdleDeadline = busySoon ? TimeStamp() : deadline.value();

I think it makes it a bit clearer what the calculation means.
Comment on attachment 8807233 [details]
Bug 1314314 - Restrict when idle callbacks are fired.

https://reviewboard.mozilla.org/r/90462/#review90208

Thanks!

::: xpcom/threads/MainThreadIdlePeriod.cpp:28
(Diff revisions 1 - 2)
>  
>    Maybe<TimeStamp> deadline = nsRefreshDriver::GetIdleDeadlineHint();
>  
>    if (deadline.isSome()) {
> -    *aIdleDeadline =
> -      TimeStamp::Now() <
> +    // If the idle period is too small, then just return a null time
> +    // to indicate we we are busy. Otherwise return the actual

s/we we/we/g
Attachment #8807233 - Flags: review?(bkelly) → review+
Try found a bug for me. Missing TimeStamp.IsNull check in busySoon calculation added.
Pushed by afarre@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3e2ce6e549e3
Restrict when idle callbacks are fired. r=bkelly
https://hg.mozilla.org/mozilla-central/rev/3e2ce6e549e3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.