1311425 - Make requestIdleCallback aware of timeouts

Status: RESOLVED FIXED

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

Comment 1

[Andreas Farre [:farre]]

Attachment: 0001-Bug-1311425-Make-idle-callbacks-avoid-timers-firing-.patch

Just threw this together as an idea of how to avoid running idle tasks when a timer is about to execute.

Comment 2

[Nathan Froyd [:froydnj]]

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.

Comment 3

[Andreas Farre [:farre]]

(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.

Comment 4

[Andreas Farre [:farre]]

Attachment: 0001-Bug-1311425-Prepare-for-handling-several-sources-of-.patch

Comment 5

[Andreas Farre [:farre]]

Attachment: 0002-Bug-1311425-Make-idle-callbacks-aware-of-nsITimers.patch

Comment 6

[Andreas Farre [:farre]]

Attachment: 0003-Bug-1311425-Add-pref-for-how-far-into-the-timer-queu.patch

Comment 7

[Andreas Farre [:farre]]

Attachment: 0004-Bug-1311425-Don-t-avoid-GC-CC-timers-when-scheduling.patch

Comment 8

[Andreas Farre [:farre]]

Attachment: 0005-Bug-1311425-Don-t-avoid-telemetry-timer-when-schedul.patch

Comment 9

[Andreas Farre [:farre]]

Attachment: 0006-Bug-1311425-Don-t-avoid-expiration-timers-when-sched.patch

Comment 10

[Andreas Farre [:farre]]

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.

Comment 11

[Andreas Farre [:farre]]

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.

Comment 12

[Andreas Farre [:farre]]

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.

Comment 13

[Andreas Farre [:farre]]

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.

Comment 14

[Andreas Farre [:farre]]

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.

Comment 15

[Andreas Farre [:farre]]

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.

Comment 16

[Andreas Farre [:farre]]

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 17

[Chris H-C :chutten]

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.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 19

[Nathan Froyd [:froydnj]]

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?

Comment 20

[Nathan Froyd [:froydnj]]

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 21

[Nathan Froyd [:froydnj]]

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.

Comment 22

[Nathan Froyd [:froydnj]]

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?

Comment 23

[Andreas Farre [:farre]]

Attachment: 0001-Bug-1311425-Prepare-for-handling-several-sources-of-.patch

Comment 24

[Andreas Farre [:farre]]

Attachment: 0002-Bug-1311425-Make-idle-callbacks-aware-of-nsITimers.-.patch

Comment 25

[Andreas Farre [:farre]]

Attachment: 0003-Bug-1311425-Add-pref-for-how-far-into-the-timer-queu.patch

Comment 26

[Andreas Farre [:farre]]

Attachment: 0004-Bug-1311425-Avoid-GC-CC-timers-when-scheduling-idle-.patch

Comment 27

[Andreas Farre [:farre]]

Attachment: 0005-Bug-1311425-Avoid-telemetry-timer-when-scheduling-id.patch

Comment 28

[Andreas Farre [:farre]]

Attachment: 0006-Bug-1311425-Avoid-expiration-timers-when-scheduling-.patch

Comment 29

[Andreas Farre [:farre]]

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.

Comment 30

[Andreas Farre [:farre]]

Attachment: 0001-Bug-1311425-Prepare-for-handling-several-sources-of-.patch

Comment 31

[Andreas Farre [:farre]]

Attachment: 0002-Bug-1311425-Make-idle-callbacks-aware-of-nsITimers.-.patch

Comment 32

[Andreas Farre [:farre]]

Attachment: 0003-Bug-1311425-Add-pref-for-how-far-into-the-timer-queu.patch

Comment 33

[Andreas Farre [:farre]]

Attachment: 0004-Bug-1311425-Avoid-GC-CC-timers-when-scheduling-idle-.patch

Comment 34

[Andreas Farre [:farre]]

Attachment: 0005-Bug-1311425-Avoid-telemetry-timer-when-scheduling-id.patch

Comment 35

[Andreas Farre [:farre]]

Attachment: 0006-Bug-1311425-Avoid-expiration-timers-when-scheduling-.patch

Comment 36

[Andreas Farre [:farre]]

Attachment: 0002-Bug-1311425-Make-idle-callbacks-aware-of-nsITimers.-.patch

Fix iteration over timers that had broken.

Comment 37

[Andreas Farre [:farre]]

Comment on attachment 8862440 [details] [diff] [review]
0001-Bug-1311425-Prepare-for-handling-several-sources-of-.patch

Carrying over r+ after fixing review issue.

Comment 38

[Andreas Farre [:farre]]

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.

Comment 39

[Andreas Farre [:farre]]

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

Comment 40

[Andreas Farre [:farre]]

Comment on attachment 8862444 [details] [diff] [review]
0005-Bug-1311425-Avoid-telemetry-timer-when-scheduling-id.patch

Fixed commit message.

Comment 41

[Andreas Farre [:farre]]

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.

Comment 42

[Andreas Farre [:farre]]

Comment on attachment 8862445 [details] [diff] [review]
0006-Bug-1311425-Avoid-expiration-timers-when-scheduling-.patch

Fixed commit message.

Comment 43

[Nathan Froyd [:froydnj]]

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.

Comment 44

[Nathan Froyd [:froydnj]]

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.

Comment 45

[Andreas Farre [:farre]]

Attachment: 0002-Bug-1311425-Make-idle-callbacks-aware-of-nsITimers.-.patch

Fixed rebase fallout.

Comment 46

[Andreas Farre [:farre]]

Attachment: 0003-Bug-1311425-Add-pref-for-how-far-into-the-timer-queu.patch

Fixed rebase fallout and added comment about skipping low priority timers.

Comment 47

[Andreas Farre [:farre]]

Attachment: 0007-Bug-1311425-Add-tests-for-finding-timer-execution-ti.patch

Added gtest tests for finding timer execution time hints.

Comment 48

[Andreas Farre [:farre]]

Attachment: 0002-Bug-1311425-Make-idle-callbacks-aware-of-nsITimers.-.patch

Comment 49

[Olli Pettay [:smaug][[e-mail]]]

We need this to make idle dispatch more reliable when doing slow cleanups. -> qf:p1

Comment 50

[Olli Pettay [:smaug][[e-mail]]]

Attachment: test changes

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.

Comment 51

[Olli Pettay [:smaug][[e-mail]]]

Ah, it might be FuzzTestTimers above which leaves some timers to the timer thread.
I move the test for this to be before that.

Comment 52

[Olli Pettay [:smaug][[e-mail]]]

Attachment: test changes, v2

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

Comment 53

[Olli Pettay [:smaug][[e-mail]]]

Attachment: tests

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.

Comment 54

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 55

[Olli Pettay [:smaug][[e-mail]]]

That try looks good and the test seems to be run on the platforms I want.

Comment 56

[Olli Pettay [:smaug][[e-mail]]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bac2e7764823b2035f256a946431d9e50dcb0f54
try: -all basically.

Comment 57

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 58

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 59

[Olli Pettay [:smaug][[e-mail]]]

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)

Comment 60

[Olli Pettay [:smaug][[e-mail]]]

Attachment: comment 57 addressed

Comment 61

[Pulsebot]

Pushed by [e-mail]:
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

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/43f38cc244b9
https://hg.mozilla.org/mozilla-central/rev/5fb5f0d2c60b
https://hg.mozilla.org/mozilla-central/rev/a16dd2fce0f1
https://hg.mozilla.org/mozilla-central/rev/60e873d0491e
https://hg.mozilla.org/mozilla-central/rev/77a89d5ea06f
https://hg.mozilla.org/mozilla-central/rev/9a74905a04ba
https://hg.mozilla.org/mozilla-central/rev/f85000b56528
https://hg.mozilla.org/mozilla-central/rev/f0c59bc00396

Comment 63

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 64

[Olli Pettay [:smaug][[e-mail]]]

oh, ThrottledEventQueue.cpp looks very broken.

Comment 65

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 66

[Olli Pettay [:smaug][[e-mail]]]

huh, our timers code is bizarre.

First item might be low priority timer, so we definitely want to skip that.

Comment 67

[Andreas Farre [:farre]]

(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.

Comment 68

[Chris Mills [:cmills]]

Setting dev-doc-needed on this, just in case this needs some kind of mention in the docs.

Comment 69

[Eric Shepherd [:sheppy]]

I don’t think this needs any docs work; anyone think of any reason it might? Andreas?

Comment 70

[Andreas Farre [:farre]]

(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.

Comment 71

[Eric Shepherd [:sheppy]]

OK, clearing DDN on this then. Thank you!