Make requestIdleCallback aware of timeouts

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM
P3
normal
RESOLVED FIXED
8 months ago
7 days ago

People

(Reporter: farre, Assigned: farre, NeedInfo)

Tracking

(Depends on: 1 bug, {dev-doc-needed})

48 Branch
mozilla55
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(8 attachments, 20 obsolete attachments)

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
Comment hidden (empty)
(Assignee)

Updated

8 months ago
Assignee: nobody → afarre
Depends on: 1198381
(Assignee)

Comment 1

8 months ago
Created attachment 8802883 [details] [diff] [review]
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.
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+
(Assignee)

Comment 3

8 months ago
(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.

Updated

6 months ago
Blocks: 1322411

Updated

6 months ago
No longer blocks: 1322411
(Assignee)

Updated

3 months ago
Blocks: 1353206
(Assignee)

Comment 4

3 months ago
Created attachment 8855764 [details] [diff] [review]
0001-Bug-1311425-Prepare-for-handling-several-sources-of-.patch
(Assignee)

Comment 5

3 months ago
Created attachment 8855765 [details] [diff] [review]
0002-Bug-1311425-Make-idle-callbacks-aware-of-nsITimers.patch
Attachment #8802883 - Attachment is obsolete: true
(Assignee)

Comment 6

3 months ago
Created attachment 8855766 [details] [diff] [review]
0003-Bug-1311425-Add-pref-for-how-far-into-the-timer-queu.patch
(Assignee)

Comment 7

3 months ago
Created attachment 8855767 [details] [diff] [review]
0004-Bug-1311425-Don-t-avoid-GC-CC-timers-when-scheduling.patch
(Assignee)

Comment 8

3 months ago
Created attachment 8855768 [details] [diff] [review]
0005-Bug-1311425-Don-t-avoid-telemetry-timer-when-schedul.patch
(Assignee)

Comment 9

3 months ago
Created attachment 8855769 [details] [diff] [review]
0006-Bug-1311425-Don-t-avoid-expiration-timers-when-sched.patch
(Assignee)

Comment 10

3 months ago
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)
(Assignee)

Comment 11

3 months ago
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)
(Assignee)

Comment 12

3 months ago
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)
(Assignee)

Comment 13

3 months ago
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)
(Assignee)

Comment 14

3 months ago
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)
(Assignee)

Comment 15

3 months ago
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)
(Assignee)

Comment 16

3 months ago
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

3 months ago
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-

Updated

3 months ago
Attachment #8855767 - Flags: review?(bugs) → review+

Comment 18

3 months ago
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+
(Assignee)

Comment 23

2 months ago
Created attachment 8862033 [details] [diff] [review]
0001-Bug-1311425-Prepare-for-handling-several-sources-of-.patch
Attachment #8855764 - Attachment is obsolete: true
(Assignee)

Comment 24

2 months ago
Created attachment 8862034 [details] [diff] [review]
0002-Bug-1311425-Make-idle-callbacks-aware-of-nsITimers.-.patch
Attachment #8855765 - Attachment is obsolete: true
(Assignee)

Comment 25

2 months ago
Created attachment 8862035 [details] [diff] [review]
0003-Bug-1311425-Add-pref-for-how-far-into-the-timer-queu.patch
Attachment #8855766 - Attachment is obsolete: true
(Assignee)

Comment 26

2 months ago
Created attachment 8862036 [details] [diff] [review]
0004-Bug-1311425-Avoid-GC-CC-timers-when-scheduling-idle-.patch
Attachment #8855767 - Attachment is obsolete: true
(Assignee)

Comment 27

2 months ago
Created attachment 8862037 [details] [diff] [review]
0005-Bug-1311425-Avoid-telemetry-timer-when-scheduling-id.patch
Attachment #8855768 - Attachment is obsolete: true
(Assignee)

Comment 28

2 months ago
Created attachment 8862038 [details] [diff] [review]
0006-Bug-1311425-Avoid-expiration-timers-when-scheduling-.patch
Attachment #8855769 - Attachment is obsolete: true
(Assignee)

Comment 29

2 months ago
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.
(Assignee)

Comment 30

2 months ago
Created attachment 8862440 [details] [diff] [review]
0001-Bug-1311425-Prepare-for-handling-several-sources-of-.patch
Attachment #8862033 - Attachment is obsolete: true
(Assignee)

Comment 31

2 months ago
Created attachment 8862441 [details] [diff] [review]
0002-Bug-1311425-Make-idle-callbacks-aware-of-nsITimers.-.patch
Attachment #8862034 - Attachment is obsolete: true
(Assignee)

Comment 32

2 months ago
Created attachment 8862442 [details] [diff] [review]
0003-Bug-1311425-Add-pref-for-how-far-into-the-timer-queu.patch
Attachment #8862035 - Attachment is obsolete: true
(Assignee)

Comment 33

2 months ago
Created attachment 8862443 [details] [diff] [review]
0004-Bug-1311425-Avoid-GC-CC-timers-when-scheduling-idle-.patch
Attachment #8862036 - Attachment is obsolete: true
(Assignee)

Comment 34

2 months ago
Created attachment 8862444 [details] [diff] [review]
0005-Bug-1311425-Avoid-telemetry-timer-when-scheduling-id.patch
Attachment #8862037 - Attachment is obsolete: true
(Assignee)

Comment 35

2 months ago
Created attachment 8862445 [details] [diff] [review]
0006-Bug-1311425-Avoid-expiration-timers-when-scheduling-.patch
Attachment #8862038 - Attachment is obsolete: true
(Assignee)

Comment 36

2 months ago
Created attachment 8862841 [details] [diff] [review]
0002-Bug-1311425-Make-idle-callbacks-aware-of-nsITimers.-.patch

Fix iteration over timers that had broken.
Attachment #8862441 - Attachment is obsolete: true
(Assignee)

Comment 37

2 months ago
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+
(Assignee)

Comment 38

2 months ago
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)
(Assignee)

Comment 39

2 months ago
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+
(Assignee)

Comment 40

2 months ago
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)
(Assignee)

Comment 41

2 months ago
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)
(Assignee)

Comment 42

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

Fixed commit message.
Attachment #8862445 - Flags: review?(nfroyd)

Updated

2 months ago
Attachment #8862444 - Flags: review?(chutten) → review+

Updated

2 months ago
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+
(Assignee)

Comment 45

2 months ago
Created attachment 8864066 [details] [diff] [review]
0002-Bug-1311425-Make-idle-callbacks-aware-of-nsITimers.-.patch

Fixed rebase fallout.
Attachment #8862841 - Attachment is obsolete: true
Attachment #8864066 - Flags: review?(nfroyd)
(Assignee)

Comment 46

2 months ago
Created attachment 8864067 [details] [diff] [review]
0003-Bug-1311425-Add-pref-for-how-far-into-the-timer-queu.patch

Fixed rebase fallout and added comment about skipping low priority timers.
Attachment #8862442 - Attachment is obsolete: true
Attachment #8864067 - Flags: review?(nfroyd)
(Assignee)

Comment 47

2 months ago
Created attachment 8864068 [details] [diff] [review]
0007-Bug-1311425-Add-tests-for-finding-timer-execution-ti.patch

Added gtest tests for finding timer execution time hints.
Attachment #8864068 - Flags: review?(nfroyd)
(Assignee)

Updated

2 months ago
Attachment #8864066 - Flags: review?(nfroyd)
(Assignee)

Updated

2 months ago
Attachment #8864067 - Flags: review?(nfroyd)
(Assignee)

Updated

2 months ago
Attachment #8864068 - Flags: review?(nfroyd)
(Assignee)

Comment 48

a month ago
Created attachment 8870399 [details] [diff] [review]
0002-Bug-1311425-Make-idle-callbacks-aware-of-nsITimers.-.patch
Attachment #8864066 - Attachment is obsolete: true

Updated

a month ago
Blocks: 1367164
We need this to make idle dispatch more reliable when doing slow cleanups. -> qf:p1
Whiteboard: [qf:p1]
Created attachment 8871244 [details] [diff] [review]
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.
Ah, it might be FuzzTestTimers above which leaves some timers to the timer thread.
I move the test for this to be before that.
Created attachment 8871245 [details] [diff] [review]
test changes, v2

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

Updated

a month ago
Attachment #8871244 - Attachment is obsolete: true

Updated

a month ago
Attachment #8871245 - Attachment is obsolete: true

Updated

a month ago
Depends on: 1367905
Created attachment 8871501 [details] [diff] [review]
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.
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.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bac2e7764823b2035f256a946431d9e50dcb0f54
try: -all basically.
Attachment #8871501 - Flags: review?(ehsan) → review+
(Assignee)

Updated

a month ago
Attachment #8864067 - Flags: review?(nfroyd)
(Assignee)

Updated

a month ago
Attachment #8864068 - Flags: review?(nfroyd)
(Assignee)

Updated

a month ago
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+
Created attachment 8871708 [details] [diff] [review]
comment 57 addressed

Comment 61

a month ago
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

Comment 62

a month ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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)

Updated

a month ago
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.

Updated

a month ago
Depends on: 1368493
(Assignee)

Comment 67

a month ago
(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)
(Assignee)

Comment 70

20 days ago
(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)

Updated

7 days ago
Depends on: 1375503
You need to log in before you can comment on or make changes to this bug.