Closed Bug 1311425 Opened 3 years ago Closed 2 years ago

Make requestIdleCallback aware of timeouts

Categories

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

48 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: farre, Assigned: farre)

References

Details

(Whiteboard: [qf:p1])

Attachments

(8 files, 20 obsolete files)

6.29 KB, patch
farre
: review+
Details | Diff | Splinter Review
4.91 KB, patch
farre
: review+
Details | Diff | Splinter Review
1.45 KB, patch
chutten
: review+
Details | Diff | Splinter Review
1.25 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
6.11 KB, patch
smaug
: review+
Details | Diff | Splinter Review
10.09 KB, patch
smaug
: review+
Details | Diff | Splinter Review
9.53 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
1.38 KB, patch
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → afarre
Depends on: 1198381
Just threw this together as an idea of how to avoid running idle tasks when a timer is about to execute.
Attachment #8802883 - Flags: feedback?(nfroyd)
Priority: -- → P3
Comment on attachment 8802883 [details] [diff] [review]
0001-Bug-1311425-Make-idle-callbacks-avoid-timers-firing-.patch

Review of attachment 8802883 [details] [diff] [review]:
-----------------------------------------------------------------

It would be helpful to provide a little background for patches like this; I assume that we're trying to refine our guess of how soon the current idle period is over?  Why are better guesses helpful here?  Do the better guesses make things run so much better that they are worth the cost of refining the guess?

It's worth noting that just because timers are going to run on the main thread doesn't mean that their event target is necessarily going to be the main thread.  (e.g. consider something like the current work to do smart scheduling of DOM events from different tabs; those are all going to run on the main thread, but presumably there's going to be an intermediary nsIEventTarget somewhere.)  I think what you really want is nsIEventTarget::isOnCurrentThread, but that's just going to make things even slower. :(

f=me, I guess, but I am currently skeptical of the enterprise.

::: xpcom/threads/TimerThread.cpp
@@ +619,5 @@
> +{
> +  MonitorAutoLock lock(mMonitor);
> +  TimeStamp timeStamp;
> +
> +  size_t index = mTimers.IndexOf(aEventTarget, 0, EventTargetComparator());

I am not super-excited about this linear search running all the time, but I think to do anything smarter, we'd have to significantly rewrite how timers work.

::: xpcom/threads/nsTimerImpl.cpp
@@ +50,5 @@
> +Maybe<TimeStamp>
> +NS_GetTimerDeadlineHint(nsIEventTarget* aEventTarget)
> +{
> +  TimeStamp deadline = gThread->FindNextFireTime(aEventTarget);
> +  return deadline.IsNull() ? Nothing() : Some(deadline - TimeDuration::FromMilliseconds(3));

Some sort of comment about why we think this is a reasonable value would be good, or turning it into a pref or something.
Attachment #8802883 - Flags: feedback?(nfroyd) → feedback+
(In reply to Nathan Froyd [:froydnj] from comment #2)
> Comment on attachment 8802883 [details] [diff] [review]
> 0001-Bug-1311425-Make-idle-callbacks-avoid-timers-firing-.patch

Thanks, this was exactly the kind of feedback I was interested in.

> Review of attachment 8802883 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It would be helpful to provide a little background for patches like this; I
> assume that we're trying to refine our guess of how soon the current idle
> period is over?  Why are better guesses helpful here?  Do the better guesses
> make things run so much better that they are worth the cost of refining the
> guess?

I don't think we know this to be honest. It is very possible that we need to collect data before we can say how necessary this is.

> It's worth noting that just because timers are going to run on the main
> thread doesn't mean that their event target is necessarily going to be the
> main thread.  (e.g. consider something like the current work to do smart
> scheduling of DOM events from different tabs; those are all going to run on
> the main thread, but presumably there's going to be an intermediary
> nsIEventTarget somewhere.)  I think what you really want is
> nsIEventTarget::isOnCurrentThread, but that's just going to make things even
> slower. :(

So as you guess this has one of its focuses on DOM events and the main thread (really sorry for not providing more info sooner). Basically the situation we wish to avoid is this:

var epsilon = 10;
function f(d) {
  var later = Date.now() + d.timeRemaining();
  while (Date.now() < later) {}
}

function g(delay) {
  var later = Date.now() + delay;
  setTimeout(funtion() {
    if (Date.now() > later + epsilon) {
      alert("We're late!");
    }
  }, delay);
}

requestIdleCallback(f);
g(20);

where the hypothetical situation where 'f' is called during an idle period estimated to be longer than 20ms (i.e. d.timeRemaining() > 20 + epsilon) makes g happen late.

> f=me, I guess, but I am currently skeptical of the enterprise.

And this is the exact response that I wanted :) I'll just think out loud a bit on refining the idle period guess.

The above situation is maybe not really something that we should handle, it reminds us of the similar:

setTimeout(function() { 
  var later = Date.now() + 30;
  while (Date.now() < later) {}
}, 15);
g(20);

where g will be late and there is no way we could handle that.

If we want to make requestIdleCallback setTimeout-aware, we could inspect the timers in the global windows, but that is a bit more spread out. And that of course doesn't allow us to avoid other timer initiated events. 

> ::: xpcom/threads/TimerThread.cpp
> @@ +619,5 @@
> > +{
> > +  MonitorAutoLock lock(mMonitor);
> > +  TimeStamp timeStamp;
> > +
> > +  size_t index = mTimers.IndexOf(aEventTarget, 0, EventTargetComparator());
> 
> I am not super-excited about this linear search running all the time, but I
> think to do anything smarter, we'd have to significantly rewrite how timers
> work.

Not really liking that either, would it be a very bad idea to add a list of event times to event targets where that target expects to get a timer callback? That would mean more data, but perhaps not that much rewriting.

> ::: xpcom/threads/nsTimerImpl.cpp
> @@ +50,5 @@
> > +Maybe<TimeStamp>
> > +NS_GetTimerDeadlineHint(nsIEventTarget* aEventTarget)
> > +{
> > +  TimeStamp deadline = gThread->FindNextFireTime(aEventTarget);
> > +  return deadline.IsNull() ? Nothing() : Some(deadline - TimeDuration::FromMilliseconds(3));
> 
> Some sort of comment about why we think this is a reasonable value would be
> good, or turning it into a pref or something.

Yes, it's really just a guess at this point.
Blocks: 1322411
No longer blocks: 1322411
Blocks: 1353206
Attachment #8802883 - Attachment is obsolete: true
Comment on attachment 8855764 [details] [diff] [review]
0001-Bug-1311425-Prepare-for-handling-several-sources-of-.patch

Rewrote how deadline hints were handled. Now when a deadline is requested a default value is given that can be used as a fallback if no deadline is found.
Attachment #8855764 - Flags: review?(bugs)
Comment on attachment 8855765 [details] [diff] [review]
0002-Bug-1311425-Make-idle-callbacks-aware-of-nsITimers.patch

Make it possible to find the next timer that will fire on the current thread and use that as one of the deadlines when determining idleness for idle callbacks.

This is almost what you had a look at last Nathan, but with the addition of low priority timers. These are used for the case where a timer is considered to be of lower priority than an idle callback.

This still suffers from linearly searching through the list of timers. I did some testing, and it looks like that in general it will look at no more than 2 timers, but in some rare cases it can be more than 10. The good thing is that this will be done when idle at least, but I'm still following this up with the next patch (comment 6) where I add a pref for how far we should look. That seems to work out ok.
Attachment #8855765 - Flags: review?(nfroyd)
Comment on attachment 8855766 [details] [diff] [review]
0003-Bug-1311425-Add-pref-for-how-far-into-the-timer-queu.patch

Add a bound for how far we should look for timers that will execute before a idle callbacks deadline.
Attachment #8855766 - Flags: review?(nfroyd)
Comment on attachment 8855767 [details] [diff] [review]
0004-Bug-1311425-Don-t-avoid-GC-CC-timers-when-scheduling.patch

Use the lower priority timers from comment 5 to schedule CC and GC.
Attachment #8855767 - Flags: review?(bugs)
Comment on attachment 8855768 [details] [diff] [review]
0005-Bug-1311425-Don-t-avoid-telemetry-timer-when-schedul.patch

Use the lower priority timers from comment 5 when accumulating telemetry.
Attachment #8855768 - Flags: review?(chutten)
Comment on attachment 8855769 [details] [diff] [review]
0006-Bug-1311425-Don-t-avoid-expiration-timers-when-sched.patch

Use the lower priority timers from comment 5 when setting expiration trackers.
Attachment #8855769 - Flags: review?(nfroyd)
A note about the lower priority timers. So far the only meaning there is for the low priority timers is that when looking for an idle deadline for idle callbacks (from NS_IdleDispatchToCurrentThread), those timers won't be considered to be in the way of an idle callback.
Comment on attachment 8855768 [details] [diff] [review]
0005-Bug-1311425-Don-t-avoid-telemetry-timer-when-schedul.patch

Review of attachment 8855768 [details] [diff] [review]:
-----------------------------------------------------------------

The commit message doesn't explain why this is a good/necessary thing.
Attachment #8855768 - Flags: review?(chutten) → review-
Attachment #8855767 - Flags: review?(bugs) → review+
Comment on attachment 8855764 [details] [diff] [review]
0001-Bug-1311425-Prepare-for-handling-several-sources-of-.patch

>+  TimeStamp currentGuess =
>+    TimeStamp::Now() + TimeDuration::FromMilliseconds(GetLongIdlePeriod());
>+
>+  currentGuess = nsRefreshDriver::GetIdleDeadlineHint(currentGuess);
>+  // If the idle period is too small, then just return a null time
>+  // to indicate we are busy. Otherwise return the actual deadline.
>+  TimeDuration minIdlePeriod =
>+    TimeDuration::FromMilliseconds(GetMinIdlePeriod());
>+  bool busySoon = currentGuess.IsNull() ||
>+                  (TimeStamp::Now() >= (currentGuess - minIdlePeriod));
>+
Could you store TimeStamp::Now() in some local variable. ::Now() may, IIRC, show up in some profiles.
Attachment #8855764 - Flags: review?(bugs) → review+
Comment on attachment 8855766 [details] [diff] [review]
0003-Bug-1311425-Add-pref-for-how-far-into-the-timer-queu.patch

Review of attachment 8855766 [details] [diff] [review]:
-----------------------------------------------------------------

Some kind of test(s) for this, with far-away expiring timers so you ideally don't have to worry about random timers firing while the test is going on, would be great.

::: modules/libpref/init/all.js
@@ +2966,5 @@
>  
> +// How far we will look for a timer in the queue of timers on the
> +// timer thread. If this bound is exceeded we use the highest time
> +// stamp found.
> +pref("idle_queue.max_timer_thread_bound", 5);

Why are we adding a setting of the pref in all.js that's identical to the default?  Seems like we could just keep the default and ditch this pref, yes?

::: xpcom/threads/MainThreadIdlePeriod.cpp
@@ +91,5 @@
> +    Preferences::AddFloatVarCache(&sMaxTimerThreadBound, "idle_queue.max_timer_thread_bound",
> +                                  DEFAULT_MAX_TIMER_THREAD_BOUND);
> +  }
> +
> +  return sMaxTimerThreadBound;

This variable is a float, but this function returns a uint32_t.  We should make these consistent so we're not paying float->int conversion overhead every time.

Also, do we want to sanitize the value somehow?  Check for negative numbers and/or use a clamped maximum timer thread bound?  I guess if we make this uint32_t consistently, the negative numbers problem goes away...

::: xpcom/threads/nsThreadUtils.h
@@ +1337,5 @@
>  void
>  NS_SetMainThread();
>  
> +/**
> + * Return the the time of next timer to execute on the current

Nit: duplicate "the".

@@ +1341,5 @@
> + * Return the the time of next timer to execute on the current
> + * thread. If that time is greater than aDefault, then return
> + * aDefault. Don't look at more than aSearchBound timers, regardless
> + * of thread, and if no time to execute is found, return the highest
> + * seen time to execute.

Hm.  How about:

Return the expiration time of the next timer to run on the current thread.  If that expiration time is greater than aDefault, then return aDefault.  aSearchBound specifies a maximum number of timers to examine to find a timer on the current thread.  If no timer that will run on the current thread is found after examining aSearchBound timers, return the highest seen expiration time as a best effort guess.

Maybe it's worth sketching out a brief rationale why we have this function?
Attachment #8855766 - Flags: review?(nfroyd) → feedback+
Comment on attachment 8855769 [details] [diff] [review]
0006-Bug-1311425-Don-t-avoid-expiration-timers-when-sched.patch

Review of attachment 8855769 [details] [diff] [review]:
-----------------------------------------------------------------

Isn't the appropriate commit message something like "avoid expiration timers when scheduling idle runnables" ?  Or perhaps I misunderstand what these kind of timers are really trying to do...
Comment on attachment 8855769 [details] [diff] [review]
0006-Bug-1311425-Don-t-avoid-expiration-timers-when-sched.patch

Review of attachment 8855769 [details] [diff] [review]:
-----------------------------------------------------------------

Whoops, forgot to hit r+, but we should talk about the commit message before this lands.
Attachment #8855769 - Flags: review?(nfroyd) → review+
Comment on attachment 8855765 [details] [diff] [review]
0002-Bug-1311425-Make-idle-callbacks-aware-of-nsITimers.patch

Review of attachment 8855765 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/threads/TimerThread.cpp
@@ +621,5 @@
> +    }
> +
> +    bool isOnCurrentThread = false;
> +    nsresult rv = timer->mEventTarget->IsOnCurrentThread(&isOnCurrentThread);
> +    NS_WARN_IF(NS_FAILED(rv));

I think this will run into static analysis problems.  How about:

if (NS_WARN_IF(NS_FAILED(rv)) {
  continue;
}

::: xpcom/threads/nsITimer.idl
@@ +123,5 @@
> +  /**
> +   * Same as TYPE_REPEATING_SLACK with the exception that idle events won't
> +   * yield to timers with this type.
> +   */
> +  const short TYPE_REPEATING_LOW_PRIORITY = 5;

How are people going to know that these timers are the right kind to use?

We probably should put _SLACK somewhere in the name so people don't think this is a precise timer of some kind.

These additional types should also be arranged such that nsTimerImpl::IsRepeating() gives the correct answer for these kinds of timers.

::: xpcom/threads/nsTimerImpl.cpp
@@ +46,5 @@
>  
> +TimeStamp
> +NS_GetTimerDeadlineHintOnCurrentThread(TimeStamp aDefault)
> +{
> +  return gThread->FindNextFireTimeForCurrentThread(aDefault);

We're sure this isn't going to be called after the timer thread has been shut down?
Attachment #8855765 - Flags: review?(nfroyd) → feedback+
Attachment #8855764 - Attachment is obsolete: true
Attachment #8855765 - Attachment is obsolete: true
Attachment #8855766 - Attachment is obsolete: true
Attachment #8855767 - Attachment is obsolete: true
Attachment #8855768 - Attachment is obsolete: true
Attachment #8855769 - Attachment is obsolete: true
I'm very sorry, but I seem to have messed up the latest flurry of patches when I rebased them with review issues fixed. All fixes ended up in the 0006 patch. I'm going to re-submit the correct ones.
Attachment #8862034 - Attachment is obsolete: true
Attachment #8862035 - Attachment is obsolete: true
Fix iteration over timers that had broken.
Attachment #8862441 - Attachment is obsolete: true
Comment on attachment 8862440 [details] [diff] [review]
0001-Bug-1311425-Prepare-for-handling-several-sources-of-.patch

Carrying over r+ after fixing review issue.
Attachment #8862440 - Flags: review+
Comment on attachment 8862442 [details] [diff] [review]
0003-Bug-1311425-Add-pref-for-how-far-into-the-timer-queu.patch

TimerThread changed how it stored its timers so I figure a re-review is called for.
Attachment #8862442 - Flags: review?(nfroyd)
Comment on attachment 8862443 [details] [diff] [review]
0004-Bug-1311425-Avoid-GC-CC-timers-when-scheduling-idle-.patch

Carrying over r+ after fixing renaming from nsITimer::TYPE_REPEATING_LOW_PRIORITY -> nsITimer::TYPE_REPEATING_SLACK_LOW_PRIORITY
Attachment #8862443 - Flags: review+
Comment on attachment 8862444 [details] [diff] [review]
0005-Bug-1311425-Avoid-telemetry-timer-when-scheduling-id.patch

Fixed commit message.
Attachment #8862444 - Flags: review?(chutten)
Comment on attachment 8862841 [details] [diff] [review]
0002-Bug-1311425-Make-idle-callbacks-aware-of-nsITimers.-.patch

TimerThread changed how it stored its timers so I figure a re-review is called for.
Attachment #8862841 - Flags: review?(nfroyd)
Comment on attachment 8862445 [details] [diff] [review]
0006-Bug-1311425-Avoid-expiration-timers-when-scheduling-.patch

Fixed commit message.
Attachment #8862445 - Flags: review?(nfroyd)
Attachment #8862444 - Flags: review?(chutten) → review+
Attachment #8862445 - Flags: review?(nfroyd) → review+
Comment on attachment 8862442 [details] [diff] [review]
0003-Bug-1311425-Add-pref-for-how-far-into-the-timer-queu.patch

Review of attachment 8862442 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the modifications.

Please add tests (C++ gtests), as previously requested in comment 19.

::: xpcom/threads/nsTimerImpl.cpp
@@ +45,4 @@
>  }
>  
>  TimeStamp
> +NS_GetTimerDeadlineHintOnCurrentThread(TimeStamp aDefault, uint32_t aSearchBound)

This bit and the body of the function should get sorted out with whatever previous patch mismatched things.
Attachment #8862442 - Flags: review?(nfroyd) → feedback+
Comment on attachment 8862841 [details] [diff] [review]
0002-Bug-1311425-Make-idle-callbacks-aware-of-nsITimers.-.patch

Review of attachment 8862841 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/threads/nsThreadUtils.h
@@ +1353,5 @@
> + * return aDefault.  aSearchBound specifies a maximum number of timers
> + * to examine to find a timer on the current thread.  If no timer that
> + * will run on the current thread is found after examining
> + * aSearchBound timers, return the highest seen expiration time as a
> + * best effort guess.

Please add something about how we are skipping low priority timers.
Attachment #8862841 - Flags: review?(nfroyd) → review+
Fixed rebase fallout.
Attachment #8862841 - Attachment is obsolete: true
Attachment #8864066 - Flags: review?(nfroyd)
Fixed rebase fallout and added comment about skipping low priority timers.
Attachment #8862442 - Attachment is obsolete: true
Attachment #8864067 - Flags: review?(nfroyd)
Added gtest tests for finding timer execution time hints.
Attachment #8864068 - Flags: review?(nfroyd)
Attachment #8864066 - Flags: review?(nfroyd)
Attachment #8864067 - Flags: review?(nfroyd)
Attachment #8864068 - Flags: review?(nfroyd)
Blocks: 1367164
We need this to make idle dispatch more reliable when doing slow cleanups. -> qf:p1
Whiteboard: [qf:p1]
Attached patch test changes (obsolete) — Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=121fa7843b12d9e9b653c1e556718cba650a8621

Let's see if this helps. There seems to be unrelated pending timers and need to clear those.
Ah, it might be FuzzTestTimers above which leaves some timers to the timer thread.
I move the test for this to be before that.
Attachment #8871244 - Attachment is obsolete: true
Attachment #8871245 - Attachment is obsolete: true
Depends on: 1367905
Attached patch testsSplinter Review
The stuff I added is ifdef and the loop in InitTimers to ensure there are no
other timers around.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a809ffedca81c02376b6d074e6e80dec3ebfe2b6

Crossing fingers this works.
Attachment #8871501 - Flags: review?(ehsan)
We should probably try to write also some mochitests or even wpt tests for this.
I'll ask Google folks if they have anything already.
That try looks good and the test seems to be run on the platforms I want.
Attachment #8871501 - Flags: review?(ehsan) → review+
Attachment #8864067 - Flags: review?(nfroyd)
Attachment #8864068 - Flags: review?(nfroyd)
Attachment #8870399 - Flags: review?(nfroyd)
Comment on attachment 8864067 [details] [diff] [review]
0003-Bug-1311425-Add-pref-for-how-far-into-the-timer-queu.patch

I had reviewed this locally.
But, this actually has a bug.
+    // Track the currently highest timeout so that we can bail when we
+    // reach the bound or when we find a timer for the current thread.
+    timeStamp = timer->mTimeout;
Shouldn't be done if timer is low priority.

Move that to happen after low priority check.
Attachment #8864067 - Flags: review?(nfroyd) → review+
Comment on attachment 8864068 [details] [diff] [review]
0007-Bug-1311425-Add-tests-for-finding-timer-execution-ti.patch

This is the patch ehsan and I guess also I reviewed while making it work on windows.
Attachment #8864068 - Attachment is obsolete: true
Attachment #8864068 - Flags: review?(nfroyd)
Comment on attachment 8870399 [details] [diff] [review]
0002-Bug-1311425-Make-idle-callbacks-aware-of-nsITimers.-.patch

And I can r+ the change to this comparing to the one was already r+'ed.
(comment fix + IsSlack handling)
Attachment #8870399 - Flags: review?(nfroyd) → review+
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/43f38cc244b9
Prepare for handling several sources of idleness, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fb5f0d2c60b
Make idle callbacks aware of nsITimers, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/a16dd2fce0f1
Add pref for how far into the timer queue, r=smaug,f=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/60e873d0491e
Avoid GC/CC timers when scheduling idle, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/77a89d5ea06f
Avoid telemetry timer when scheduling idle runnables, r=chutten
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a74905a04ba
Avoid expiration timers when scheduling idle runnables, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/f85000b56528
gtests for NS_GetTimerDeadlineHintOnCurrentThread, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0c59bc00396
review comment fix to ensure low priority timers aren't taken into account when calling NS_GetTimerDeadlineHintOnCurrentThread, r=smaug
Does this work for things using ThrottledEventQueue like setTimeout()/setInterval()?  I don't think the ThrottledEventQueue::IsOnCurrentThread() will return true here.  See:

https://dxr.mozilla.org/mozilla-central/source/xpcom/threads/ThrottledEventQueue.cpp#398

Perhaps we could expose an nsINestedEventTarget type that has a ::GetBaseTarget().  You could then QI/call that until you find the underlying nsThread and perform the check there.
Flags: needinfo?(bugs)
Flags: needinfo?(afarre)
oh, ThrottledEventQueue.cpp looks very broken.
Flags: needinfo?(bugs)
Depends on: 1368358
Flags: needinfo?(afarre)
Sorry, but I found another bug in the patches here.  FindNextFireTimeForCurrentThread() iterates over the timeout list as if it is in a simple sorted order.  It is not.

The TimerThread uses a binary heap to store the timeout list.  See std::push_heap()/std::pop_heap().  This ensures the *first* item in the list is the soonest timeout, but otherwise does not provide any guaranteed ordering of the list.  You cannot simply iterate it and expect it to be in order.

Also, I think its very bad for perf to potentially iterate the entire timer list.  If none of them cover the given event target then you will iterate the entire list.  Given the length of this list is dictated by content, I think this could be potentially bad news.

I would recommend just looking at the first item in the list for now.  For the main thread it will most likely match.  If it doesn't match its likely the refresh frame budget will be a limiting factor on the idle case anyway.
Flags: needinfo?(bugs)
Flags: needinfo?(afarre)
huh, our timers code is bizarre.

First item might be low priority timer, so we definitely want to skip that.
Depends on: 1368493
(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #65)
> Sorry, but I found another bug in the patches here. 
> FindNextFireTimeForCurrentThread() iterates over the timeout list as if it
> is in a simple sorted order.  It is not.
> 
> The TimerThread uses a binary heap to store the timeout list.  See
> std::push_heap()/std::pop_heap().  This ensures the *first* item in the list
> is the soonest timeout, but otherwise does not provide any guaranteed
> ordering of the list.  You cannot simply iterate it and expect it to be in
> order.
> 
> Also, I think its very bad for perf to potentially iterate the entire timer
> list.  If none of them cover the given event target then you will iterate
> the entire list.  Given the length of this list is dictated by content, I
> think this could be potentially bad news.
> 
> I would recommend just looking at the first item in the list for now.  For
> the main thread it will most likely match.  If it doesn't match its likely
> the refresh frame budget will be a limiting factor on the idle case anyway.

That's what the preffed bound is meant to handle, but an unordered list is indeed troublesome.
Flags: needinfo?(afarre)
Setting dev-doc-needed on this, just in case this needs some kind of mention in the docs.
Keywords: dev-doc-needed
I don’t think this needs any docs work; anyone think of any reason it might? Andreas?
Flags: needinfo?(afarre)
(In reply to Eric Shepherd [:sheppy] from comment #69)
> I don’t think this needs any docs work; anyone think of any reason it might?
> Andreas?

I don't think so. The documentation for requestIdleCallback mentions that callbacks will execute in idle periods and that should be good enough. We don't mention inter-frame idle periods either.
Flags: needinfo?(afarre)
Depends on: 1375503
OK, clearing DDN on this then. Thank you!
Keywords: dev-doc-needed
Flags: needinfo?(bugs)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.