Closed Bug 1362322 Opened 7 years ago Closed 7 years ago

Implement budget based background timeout throttling

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: farre, Assigned: farre)

References

(Blocks 3 open bugs)

Details

Attachments

(1 file, 15 obsolete files)

32.61 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
      No description provided.
Note to self: is it a good idea to have a dynamic regeneration rate, e.g. if we're animating the foreground tab do we regenerate budgets more slowly? Could use some similar heuristic as rIC to guess if we'll paint again soon.
IMO would be good to get some basic budgeting landed and enabled first, and then add some more heuristics etc.
Priority: -- → P2
(In reply to Olli Pettay [:smaug] from comment #2)
> IMO would be good to get some basic budgeting landed and enabled first, and
> then add some more heuristics etc.

I agree, FWIW!
Assignee: nobody → afarre
What's missing in this patch:

* No grace period before beginning throttling
* No comments
* No foreground throttling for testing
(In reply to Andreas Farre [:farre] from comment #4)
> Created attachment 8866419 [details] [diff] [review]
> WIP-Bug-1362322-Throttle-background-timeouts-based-on-a-.patch
> 
> What's missing in this patch:
> 
> * No grace period before beginning throttling
> * No comments
> * No foreground throttling for testing

* No check for IndexedDB transactions, open WebSockets, etc.
Depends on: 1363829
Depends on: 1367173
Blocks: 1367173
No longer depends on: 1367173
Attachment #8866419 - Attachment is obsolete: true
Still work in progress. Needs bug 1363829 to work, as well as bug 1352401.

What's missing in these patches:

* No grace period before beginning throttling
* More comments
* No foreground throttling for testing
* No check for IndexedDB transactions, open WebSockets, etc.
* No flag for running with budget bg throttling turned off for try
Still work in progress. Needs bug 1363829 to work, as well as bug 1352401.

What's missing in these patches:

* More comments
* No check for IndexedDB transactions, open WebSockets, etc.
* No flag for running with budget bg throttling turned off for try

Now has hidden prefs for budgets and fore foreground throttling.

Also uses the tracking timer delay before throttling.
Attachment #8872620 - Attachment is obsolete: true
Attachment #8872621 - Attachment is obsolete: true
Depends on: 1352401
Attachment #8872619 - Attachment is obsolete: true
Attachment #8872686 - Attachment is obsolete: true
Blocks: 1369418
Attachment #8873461 - Attachment is obsolete: true
Attachment #8878055 - Flags: feedback?(ehsan)
Attachment #8878055 - Flags: feedback?(bkelly)
Attachment #8873462 - Attachment is obsolete: true
Attachment #8878056 - Flags: feedback?(mcmanus)
Attachment #8873463 - Attachment is obsolete: true
Attachment #8878057 - Flags: feedback?(ehsan)
Attachment #8878057 - Flags: feedback?(bkelly)
Attachment #8873464 - Attachment is obsolete: true
Attachment #8878058 - Flags: feedback?(mconley)
Attachment #8878058 - Flags: feedback?(jmaher)
I'm flying a bit blind since I can't see treeherder currently so I don't know how much errors there are. I knew that there used to be a handful of permafail timeout intermittents that I need to fix, but I also need some feedback to ease the review process.

Thankful for any help I can get :)
Comment on attachment 8878055 [details] [diff] [review]
0001-Bug-1362322-Unify-execution-measurements.-r-ehsan.patch

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

::: dom/base/TimeoutManager.cpp
@@ +23,4 @@
>  // Time between sampling timeout execution time.
>  const uint32_t kTelemetryPeriodMS = 1000;
>  
> +class TimeoutBudgetManager

I think it would be nice to move this to a separate set of h/cpp files.

::: dom/base/TimeoutManager.h
@@ +133,4 @@
>  
>    TimeDuration
>    MinSchedulingDelay() const;
> +  

nit: trailing whitespace
Attachment #8878055 - Flags: feedback?(bkelly) → feedback+
Comment on attachment 8878058 [details] [diff] [review]
0004-Bug-1362322-Add-command-line-flag-to-disable-timeout.patch

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

outside of automation.py.in I am a bit lost here- I assume my comment will help get this enabled in the right places going forward.

::: build/automation.py.in
@@ +475,4 @@
>      if self.IS_MAC:
>        args.append("-foreground")
>  
> +    args.append("-disable-background-timer-throttling")

do you want this on all platforms and all configurations (debug, opt, pgo, asan)?

Will we need this in all harnesses (mochitest, reftest, xpcshell, marionette, web-platform-tests, talos, awsy, autophone, etc.)  If so, then we should edit more files.  automation,py is for mochitest/reftest.
Attachment #8878058 - Flags: feedback?(jmaher)
Comment on attachment 8878056 [details] [diff] [review]
0002-Bug-1362322-Add-hasListenerFor-to-nsIWebSocketEventS.patch

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

no issues..
Attachment #8878056 - Flags: feedback?(mcmanus) → feedback+
Comment on attachment 8878057 [details] [diff] [review]
0003-Bug-1362322-Throttle-background-timeouts-using-budge.patch

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

Looks like a reasonable approach, but I think there are some problems with the math being used.

In addition to the changes I suggest below, it might be useful to have a big comment block that lays out an example of the math like:

// If you have a budget of 10ms
//   and we execute for 2ms
//   and its been 15ms since our last update
//   then you end up with X ms

// If you have a budget of 10ms
//    out of a budget of 50ms
//    we must delay by at least X ms

Also, I think maybe we should consider adding some gtests to validate our calcuation methods.  They are getting hard to reason about.

::: dom/base/TimeoutManager.cpp
@@ +147,4 @@
>  static int32_t gMinBackgroundTimeoutValue = 0;
>  static int32_t gMinTrackingTimeoutValue = 0;
>  static int32_t gMinTrackingBackgroundTimeoutValue = 0;
> +static int32_t gTimeoutThrottlingDelay = 0;

We are not throttling tracking timeouts any more?

@@ +164,5 @@
>  // static
>  const uint32_t TimeoutManager::InvalidFiringId = 0;
>  
>  bool
> +TimeoutManager::IsActive() const

Can we keep an IsBackground() that is simply an alias to `!IsActive() && mWindow.IsBackgroundInternal()`?

It might also be worth adding a comment describing the states here:

1. Active means a tab doing one of these things.
2. Both foreground and background tabs can be Active or Inactive.
3. We treat Active background tabs as if they were foreground.

@@ +209,3 @@
>  }
>  
> +

nit: extra line

@@ +237,5 @@
>    return !IsInvalidFiringId(aFiringId);
>  }
>  
> +static int32_t
> +GetRegenerationFactor(bool aIsBackground)

nit: I think it would be better to put this in an anonymous namespace and drop the 'static' keyword.

A comment about what this returns might be nice.  Is int32_t really the best type here?  I think a double would be better.

@@ +249,5 @@
>  TimeoutManager::MinSchedulingDelay() const
>  {
> +  if (!IsActive()) {
> +    bool isBackground = mWindow.IsBackgroundInternal();
> +    int32_t delay = isBackground ? gMinBackgroundTimeoutValue : gMinTimeoutValue;

This seems wrong.  gMinTimeoutValue should only be used for clamping.

@@ +253,5 @@
> +    int32_t delay = isBackground ? gMinBackgroundTimeoutValue : gMinTimeoutValue;
> +
> +    if (mBudgetThrottleTimeouts) {
> +      int32_t factor = GetRegenerationFactor(isBackground);
> +      int32_t budget = - mExecutionBudget.ToMilliseconds() * factor;

This doesn't make sense to me at all.  We multiply the negative mExecutionBudget by the factor?  So this is always a negative number?  Or is it supposed to be subtracted from something.

In general I would expect our MinSchedulingDelay() to increase as the execution budget is decreased.

Should this be:

  MaxBudget() - mExecutionBudget

?  Why the factor multiplication here.

@@ +254,5 @@
> +
> +    if (mBudgetThrottleTimeouts) {
> +      int32_t factor = GetRegenerationFactor(isBackground);
> +      int32_t budget = - mExecutionBudget.ToMilliseconds() * factor;
> +      delay = std::max(delay, budget);

Since mExecutionBudget is a TimeoutDuration I think we should avoid integer math here.  Conversions to and from integers have caused a lot of problems for me in this code before.  So I would write this as:

  double factor = GetRegenerationFactor(isBackground);
  TimeDuration budget(mExecutionBudget.MultDouble(factor));
  delay = TimeDuration::Max(delay, budget);

@@ +315,5 @@
>      // If we're running a timeout callback, record any execution until
>      // now.
> +    bool isBackground = mWindow.IsBackgroundInternal();
> +    TimeDuration duration = TimeoutBudgetManager::Get().RecordExecution(
> +      now, aRunningTimeout, isBackground);

This is changing the meaning of this call.  It used to use IsBackground() which took audio into account, but now its using IsBackgroundInternal().  Is that intentional?  It seems we should continue to use IsBackground() everywhere we did before.

@@ +330,5 @@
>    }
>  }
>  
> +static TimeDuration
> +GetMaxBudget(bool aIsBackground)

nit: Please put in an anonymous namespace and drop static keyword.

@@ +349,5 @@
> +  // The budget is adjusted by increasing it with the time since the
> +  // last budget update factored with the regeneration rate. If a
> +  // runnable has executed, subtract that duration from the budget.
> +  if (mBudgetThrottleTimeouts) {
> +    bool isBackground = mWindow.IsBackgroundInternal();

Why doesn't this code have to take into account IsActive()?  It seems like that state could change and we would need to adjust the budget accordingly.

@@ +351,5 @@
> +  // runnable has executed, subtract that duration from the budget.
> +  if (mBudgetThrottleTimeouts) {
> +    bool isBackground = mWindow.IsBackgroundInternal();
> +    int32_t factor = GetRegenerationFactor(isBackground);
> +    int32_t regenerated = (aNow - mLastBudgetUpdate).ToMilliseconds() / factor;

Again, please keep this in TimeDuration instead of using integer math.  See TimeDuration::DivideDouble().

@@ +356,5 @@
> +    // Clamp the budget to the maximum allowed budget.
> +    mExecutionBudget =
> +      TimeDuration::Min(GetMaxBudget(isBackground),
> +                        mExecutionBudget - aDuration +
> +                          TimeDuration::FromMilliseconds(regenerated));

If a timeout executes for a really long time I think you can end up with a negative value here.

::: modules/libpref/init/all.js
@@ +1246,5 @@
> +// values greater than 0.
> +pref("dom.timeout.background_throttling_max_budget", 50);
> +// Maximum value (in ms) for the background budget. Only valid for
> +// values greater than 0.
> +pref("dom.timeout.foreground_throttling_max_budget", -1);

Maybe group these by foreground/background and add a comment that foreground throttling is currently disabled.
Attachment #8878057 - Flags: feedback?(bkelly) → feedback-
Comment on attachment 8878058 [details] [diff] [review]
0004-Bug-1362322-Add-command-line-flag-to-disable-timeout.patch

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

::: browser/components/nsBrowserContentHandler.js
@@ +442,5 @@
>          }
>        }
>      }
> +
> +    if (cmdLine.handleFlag("disable-background-timer-throttling", false)) {

I'm a little confused... maybe I missed something, but why do we want this to be a command line argument?

I understand that there might be some tests or some automation where we want to disable those things, but I'm pretty sure in those cases we can flip prefs in other ways (like making sure that the testing profile has a particular pref flipped).

Or did I miss something?
Attachment #8878058 - Flags: feedback?(mconley)
(In reply to Joel Maher ( :jmaher) from comment #22)
> Comment on attachment 8878058 [details] [diff] [review]
> 0004-Bug-1362322-Add-command-line-flag-to-disable-timeout.patch
> 
> Review of attachment 8878058 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> outside of automation.py.in I am a bit lost here- I assume my comment will
> help get this enabled in the right places going forward.

That was what I intended, thanks.

> Will we need this in all harnesses (mochitest, reftest, xpcshell,
> marionette, web-platform-tests, talos, awsy, autophone, etc.)  If so, then
> we should edit more files.  automation,py is for mochitest/reftest.

Right, and this is a probable reason for the permafails. Needs to be added everywhere!
(In reply to Mike Conley (:mconley) from comment #25)
> Comment on attachment 8878058 [details] [diff] [review]
> 0004-Bug-1362322-Add-command-line-flag-to-disable-timeout.patch
> 
> Review of attachment 8878058 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/nsBrowserContentHandler.js
> @@ +442,5 @@
> >          }
> >        }
> >      }
> > +
> > +    if (cmdLine.handleFlag("disable-background-timer-throttling", false)) {
> 
> I'm a little confused... maybe I missed something, but why do we want this
> to be a command line argument?
> 
> I understand that there might be some tests or some automation where we want
> to disable those things, but I'm pretty sure in those cases we can flip
> prefs in other ways (like making sure that the testing profile has a
> particular pref flipped).
> 
> Or did I miss something?

Nope, you're correct. This is mostly to match the same flag in Chrome.
(In reply to Andreas Farre [:farre] from comment #27)
> Nope, you're correct. This is mostly to match the same flag in Chrome.

I see - and is matching the command line flag an explicit requirement of this feature?
Flags: needinfo?(afarre)
(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #24)
> Comment on attachment 8878057 [details] [diff] [review]
> 0003-Bug-1362322-Throttle-background-timeouts-using-budge.patch
> 
> Review of attachment 8878057 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks like a reasonable approach, but I think there are some problems with
> the math being used.
> 
> In addition to the changes I suggest below, it might be useful to have a big
> comment block that lays out an example of the math like:
> 
> // If you have a budget of 10ms
> //   and we execute for 2ms
> //   and its been 15ms since our last update
> //   then you end up with X ms
> 
> // If you have a budget of 10ms
> //    out of a budget of 50ms
> //    we must delay by at least X ms
> 
> Also, I think maybe we should consider adding some gtests to validate our
> calcuation methods.  They are getting hard to reason about.

Sounds like a plan.

> ::: dom/base/TimeoutManager.cpp
> @@ +147,4 @@
> >  static int32_t gMinBackgroundTimeoutValue = 0;
> >  static int32_t gMinTrackingTimeoutValue = 0;
> >  static int32_t gMinTrackingBackgroundTimeoutValue = 0;
> > +static int32_t gTimeoutThrottlingDelay = 0;
> 
> We are not throttling tracking timeouts any more?

We are, but we use the same timer to start throttling.

> @@ +164,5 @@
> >  // static
> >  const uint32_t TimeoutManager::InvalidFiringId = 0;
> >  
> >  bool
> > +TimeoutManager::IsActive() const
> 
> Can we keep an IsBackground() that is simply an alias to `!IsActive() &&
> mWindow.IsBackgroundInternal()`?

Yes, will do.

> It might also be worth adding a comment describing the states here:
> 
> 1. Active means a tab doing one of these things.
> 2. Both foreground and background tabs can be Active or Inactive.
> 3. We treat Active background tabs as if they were foreground.

Will do.

> @@ +209,3 @@
> >  }
> >  
> > +
> 
> nit: extra line
> 
> @@ +237,5 @@
> >    return !IsInvalidFiringId(aFiringId);
> >  }
> >  
> > +static int32_t
> > +GetRegenerationFactor(bool aIsBackground)
> 
> nit: I think it would be better to put this in an anonymous namespace and
> drop the 'static' keyword.

Good point.

> A comment about what this returns might be nice.  Is int32_t really the best
> type here?  I think a double would be better.
> 
> @@ +249,5 @@
> >  TimeoutManager::MinSchedulingDelay() const
> >  {
> > +  if (!IsActive()) {
> > +    bool isBackground = mWindow.IsBackgroundInternal();
> > +    int32_t delay = isBackground ? gMinBackgroundTimeoutValue : gMinTimeoutValue;
> 
> This seems wrong.  gMinTimeoutValue should only be used for clamping.

This is to be able to throttle foreground tabs. Essential for testing.

> @@ +253,5 @@
> > +    int32_t delay = isBackground ? gMinBackgroundTimeoutValue : gMinTimeoutValue;
> > +
> > +    if (mBudgetThrottleTimeouts) {
> > +      int32_t factor = GetRegenerationFactor(isBackground);
> > +      int32_t budget = - mExecutionBudget.ToMilliseconds() * factor;
> 
> This doesn't make sense to me at all.  We multiply the negative
> mExecutionBudget by the factor?  So this is always a negative number?  Or is
> it supposed to be subtracted from something.

This is actually the way that we punish long running timeouts. When a timeout exceeds the execution budget, it goes negative.

> > +      int32_t budget = - mExecutionBudget.ToMilliseconds() * factor;
----------------------------^

That sign flips the potential negative sign in that case, and the std::max picks that one. If we have budget left, that will be negative and isn't picked by std::max. I concede that int32_t budget could be June's worst variable name though.

> In general I would expect our MinSchedulingDelay() to increase as the
> execution budget is decreased.
> 
> Should this be:
> 
>   MaxBudget() - mExecutionBudget
> 
> ?  Why the factor multiplication here.
> 
> @@ +254,5 @@
> > +
> > +    if (mBudgetThrottleTimeouts) {
> > +      int32_t factor = GetRegenerationFactor(isBackground);
> > +      int32_t budget = - mExecutionBudget.ToMilliseconds() * factor;
> > +      delay = std::max(delay, budget);
> 
> Since mExecutionBudget is a TimeoutDuration I think we should avoid integer
> math here.  Conversions to and from integers have caused a lot of problems
> for me in this code before.  So I would write this as:
>
>   double factor = GetRegenerationFactor(isBackground);
>   TimeDuration budget(mExecutionBudget.MultDouble(factor));
>   delay = TimeDuration::Max(delay, budget);

Ok, can do.
 
> @@ +315,5 @@
> >      // If we're running a timeout callback, record any execution until
> >      // now.
> > +    bool isBackground = mWindow.IsBackgroundInternal();
> > +    TimeDuration duration = TimeoutBudgetManager::Get().RecordExecution(
> > +      now, aRunningTimeout, isBackground);
> 
> This is changing the meaning of this call.  It used to use IsBackground()
> which took audio into account, but now its using IsBackgroundInternal().  Is
> that intentional?  It seems we should continue to use IsBackground()
> everywhere we did before.

It is intentional. I think that budget should be calculated based on actual backgroundedness. If there is audio we won't throttle anyway, due to the use of IsActive.

> @@ +330,5 @@
> >    }
> >  }
> >  
> > +static TimeDuration
> > +GetMaxBudget(bool aIsBackground)
> 
> nit: Please put in an anonymous namespace and drop static keyword.

Right.

> @@ +349,5 @@
> > +  // The budget is adjusted by increasing it with the time since the
> > +  // last budget update factored with the regeneration rate. If a
> > +  // runnable has executed, subtract that duration from the budget.
> > +  if (mBudgetThrottleTimeouts) {
> > +    bool isBackground = mWindow.IsBackgroundInternal();
> 
> Why doesn't this code have to take into account IsActive()?  It seems like
> that state could change and we would need to adjust the budget accordingly.

Because if the TimeoutManager::IsActive we don't throttle anyway and budget becomes a moot point, throttling wise. For the telemetry side of it, I think actual backgroundedness is more accurate.

> @@ +351,5 @@
> > +  // runnable has executed, subtract that duration from the budget.
> > +  if (mBudgetThrottleTimeouts) {
> > +    bool isBackground = mWindow.IsBackgroundInternal();
> > +    int32_t factor = GetRegenerationFactor(isBackground);
> > +    int32_t regenerated = (aNow - mLastBudgetUpdate).ToMilliseconds() / factor;
> 
> Again, please keep this in TimeDuration instead of using integer math.  See
> TimeDuration::DivideDouble().
> 
> @@ +356,5 @@
> > +    // Clamp the budget to the maximum allowed budget.
> > +    mExecutionBudget =
> > +      TimeDuration::Min(GetMaxBudget(isBackground),
> > +                        mExecutionBudget - aDuration +
> > +                          TimeDuration::FromMilliseconds(regenerated));
> 
> If a timeout executes for a really long time I think you can end up with a
> negative value here.

And that is the point. It happens fairly fast with long running timeouts, and it is those we wish to punish. See comment above.

> ::: modules/libpref/init/all.js
> @@ +1246,5 @@
> > +// values greater than 0.
> > +pref("dom.timeout.background_throttling_max_budget", 50);
> > +// Maximum value (in ms) for the background budget. Only valid for
> > +// values greater than 0.
> > +pref("dom.timeout.foreground_throttling_max_budget", -1);
> 
> Maybe group these by foreground/background and add a comment that foreground
> throttling is currently disabled.

Sure thing.

Thanks for the feedback.
(In reply to Mike Conley (:mconley) from comment #28)
> (In reply to Andreas Farre [:farre] from comment #27)
> > Nope, you're correct. This is mostly to match the same flag in Chrome.
> 
> I see - and is matching the command line flag an explicit requirement of
> this feature?

Not really. Just handy.
Flags: needinfo?(afarre)
So what I'm saying is that the math is correct, except for being in internet, but that I'll write comments to prove it :)
Being in int! Stupid phone, and stupid me for working after hours.
(In reply to Andreas Farre [:farre] from comment #29)
> > @@ +249,5 @@
> > >  TimeoutManager::MinSchedulingDelay() const
> > >  {
> > > +  if (!IsActive()) {
> > > +    bool isBackground = mWindow.IsBackgroundInternal();
> > > +    int32_t delay = isBackground ? gMinBackgroundTimeoutValue : gMinTimeoutValue;
> > 
> > This seems wrong.  gMinTimeoutValue should only be used for clamping.
> 
> This is to be able to throttle foreground tabs. Essential for testing.

You need to add a different pref then.  We don't want to force all setTimeout() calls to a minimum of 4ms if they are not using one of the "active" APIs.  Think about setTimeout(f, 0) in a normal foreground tab.

> > @@ +253,5 @@
> > > +    int32_t delay = isBackground ? gMinBackgroundTimeoutValue : gMinTimeoutValue;
> > > +
> > > +    if (mBudgetThrottleTimeouts) {
> > > +      int32_t factor = GetRegenerationFactor(isBackground);
> > > +      int32_t budget = - mExecutionBudget.ToMilliseconds() * factor;
> > 
> > This doesn't make sense to me at all.  We multiply the negative
> > mExecutionBudget by the factor?  So this is always a negative number?  Or is
> > it supposed to be subtracted from something.
> 
> This is actually the way that we punish long running timeouts. When a
> timeout exceeds the execution budget, it goes negative.
> 
> > > +      int32_t budget = - mExecutionBudget.ToMilliseconds() * factor;
> ----------------------------^
> 
> That sign flips the potential negative sign in that case, and the std::max
> picks that one. If we have budget left, that will be negative and isn't
> picked by std::max. I concede that int32_t budget could be June's worst
> variable name though.

It probably goes without saying, but a huge comment to this would be great.

Also, it might be a bit more explicit to do something like this:

  if (mBudgetThrottleTimeouts && mExecutionBudget < TimeoutDuration(0)) {
    // apply throttling only if mExecutionBudget is negative
  }

Is it ok that content script can trigger a throttle period of potentially unbounded length?  Should we cap this at some large, but reasonable value?
Also, I think you should probably make this take mExecutionBudget into account:

http://searchfox.org/mozilla-central/source/dom/base/TimeoutManager.cpp#562

So it does something like:

  const TimeDuration totalTimeLimit = TimeDuration::Min(TimeDuration::FromMilliseconds(totalTimeLimitMS),
                                                        mExecutionBudget);
  totalTimeLimit = TimeDuration::Max(TimeDuration(), totalTimeLimit);

And then short circuit the first loop immediately if totalTimeLimit is zero:

http://searchfox.org/mozilla-central/source/dom/base/TimeoutManager.cpp#562

With something like:

  if (!timeout || initialTimeLimit.IsZero() || timeout->When() > deadline) {
    if (timeout) {
      nextDeadline = timeout->When();
    }
    break;
  }

This should then just update the budget and call MaybeSchedule() again with the new min scheduling delay.

I think we want to do this in order to handle the cases where TimeExecutor() fires a duplicate or something.  If it does this when the budget is negative we could end up running a really expensive timeout callback right away again.
Comment on attachment 8878055 [details] [diff] [review]
0001-Bug-1362322-Unify-execution-measurements.-r-ehsan.patch

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

::: dom/base/TimeoutManager.cpp
@@ +23,4 @@
>  // Time between sampling timeout execution time.
>  const uint32_t kTelemetryPeriodMS = 1000;
>  
> +class TimeoutBudgetManager

But as a separate patch please (preferably separate bug even!)
Attachment #8878055 - Flags: feedback?(ehsan) → feedback+
Depends on: 1373536
(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #33)
>
> Is it ok that content script can trigger a throttle period of potentially
> unbounded length?  Should we cap this at some large, but reasonable value?

This is a really interesting question, and I actually think that we shouldn't cap it. Google's documentation[1] for their version of this feature say:

"Finally, remember that if you are using long tasks in the background, your application can be throttled for a very long period of time (up to 100 times the duration of your task)"

and I think that we should behave the same. Since switching to the tab will reset timers and start running immediately I think we're all good.

[1] https://developers.google.com/web/updates/2017/03/background_tabs
No longer depends on: 1373536
(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #34)
> Also, I think you should probably make this take mExecutionBudget into
> account:
> 
> http://searchfox.org/mozilla-central/source/dom/base/TimeoutManager.cpp#562
> 
> So it does something like:
> 
>   const TimeDuration totalTimeLimit =
> TimeDuration::Min(TimeDuration::FromMilliseconds(totalTimeLimitMS),
>                                                         mExecutionBudget);
>   totalTimeLimit = TimeDuration::Max(TimeDuration(), totalTimeLimit);
> 
> And then short circuit the first loop immediately if totalTimeLimit is zero:
> 
> http://searchfox.org/mozilla-central/source/dom/base/TimeoutManager.cpp#562
> 
> With something like:
> 
>   if (!timeout || initialTimeLimit.IsZero() || timeout->When() > deadline) {
>     if (timeout) {
>       nextDeadline = timeout->When();
>     }
>     break;
>   }
> 
> This should then just update the budget and call MaybeSchedule() again with
> the new min scheduling delay.
> 
> I think we want to do this in order to handle the cases where TimeExecutor()
> fires a duplicate or something.  If it does this when the budget is negative
> we could end up running a really expensive timeout callback right away again.

Ooohh, I like this. Adding it.
Depends on: 1373536
(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #33)
> (In reply to Andreas Farre [:farre] from comment #29)
> > > @@ +249,5 @@
> > > >  TimeoutManager::MinSchedulingDelay() const
> > > >  {
> > > > +  if (!IsActive()) {
> > > > +    bool isBackground = mWindow.IsBackgroundInternal();
> > > > +    int32_t delay = isBackground ? gMinBackgroundTimeoutValue : gMinTimeoutValue;
> > > 
> > > This seems wrong.  gMinTimeoutValue should only be used for clamping.
> > 
> > This is to be able to throttle foreground tabs. Essential for testing.
> 
> You need to add a different pref then.  We don't want to force all
> setTimeout() calls to a minimum of 4ms if they are not using one of the
> "active" APIs.  Think about setTimeout(f, 0) in a normal foreground tab.

Actually, I would like to understand this more.  It seems that your budget throttling will still take effect even if we replace gMinTimeoutValue with 0 here.

Also, I just renamed gMinTimeoutValue to gMinClampTimeoutValue to clarify its not a global minimum, but only applies to clamped timeouts.
Depends on: 1373675
Addressed feedback issues. Note that the suggestion from Comment 34 isn't implemented, since that didn't work out that well. I think it's ok to skip that though, because doing that only really delays the budget throttling. And that is something that is happening already, since a currently running timeout doesn't contribute in execution time to the budget until it has finished. The important thing is that the _average_ execution is throttled.
Attachment #8878057 - Attachment is obsolete: true
Attachment #8878057 - Flags: feedback?(ehsan)
Attachment #8878541 - Flags: feedback?(bkelly)
Comment on attachment 8878541 [details] [diff] [review]
0003-Bug-1362322-Throttle-background-timeouts-using-budge.patch

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

::: dom/base/TimeoutManager.cpp
@@ +938,5 @@
>      return false;
>    }
>  
> +  TimeStamp currentNow = TimeStamp::Now();
> +

This move is silly, I'll remove it.
Comment on attachment 8878541 [details] [diff] [review]
0003-Bug-1362322-Throttle-background-timeouts-using-budge.patch

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

I didn't read it completely in detail, but looks a lot better.  Thanks!

It might improve readability to make the rename of mThrottleTrackingTimeoutsTimer/etc a separate patch that comes before this.

::: dom/base/TimeoutManager.cpp
@@ +238,5 @@
> +  if (mBudgetThrottleTimeouts && mExecutionBudget < TimeDuration()) {
> +    // Only throttle if execution budget is less than 0
> +    double factor = 1.0 / GetRegenerationFactor(mWindow.IsBackgroundInternal());
> +    return TimeDuration::Max(TimeDuration(),
> +                             -mExecutionBudget.MultDouble(factor));

I'm not sure returning is quite right here.  What if this calculated value is less than gMinBackgroundTimeoutValue?  This should probably be "maxed" with the background value.
Attachment #8878541 - Flags: feedback?(bkelly) → feedback+
(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #41)
> Comment on attachment 8878541 [details] [diff] [review]
> 0003-Bug-1362322-Throttle-background-timeouts-using-budge.patch
> 
> Review of attachment 8878541 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I didn't read it completely in detail, but looks a lot better.  Thanks!
> 
> It might improve readability to make the rename of
> mThrottleTrackingTimeoutsTimer/etc a separate patch that comes before this.
> 
> ::: dom/base/TimeoutManager.cpp
> @@ +238,5 @@
> > +  if (mBudgetThrottleTimeouts && mExecutionBudget < TimeDuration()) {
> > +    // Only throttle if execution budget is less than 0
> > +    double factor = 1.0 / GetRegenerationFactor(mWindow.IsBackgroundInternal());
> > +    return TimeDuration::Max(TimeDuration(),
> > +                             -mExecutionBudget.MultDouble(factor));
> 
> I'm not sure returning is quite right here.  What if this calculated value
> is less than gMinBackgroundTimeoutValue?  This should probably be "maxed"
> with the background value.

Sure thing!
Comment on attachment 8878056 [details] [diff] [review]
0002-Bug-1362322-Add-hasListenerFor-to-nsIWebSocketEventS.patch

Landed in Bug 1373675.
Attachment #8878056 - Attachment is obsolete: true
Comment on attachment 8878055 [details] [diff] [review]
0001-Bug-1362322-Unify-execution-measurements.-r-ehsan.patch

Landed in Bug 1373536.
Attachment #8878055 - Attachment is obsolete: true
Attachment #8878541 - Attachment is obsolete: true
Caveat: there is some permaorange indirectly (I hope) cause by this that needs fixing as well, see https://treeherder.mozilla.org/#/jobs?repo=try&revision=88ac6abcc1cc2c5d10c90b93a349faf4f62f82ea
Attachment #8879583 - Attachment is obsolete: true
Attachment #8882008 - Flags: review?(bkelly)
Comment on attachment 8882008 [details] [diff] [review]
0001-Bug-1362322-Throttle-background-timeouts-using-budge.patch

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

Overall looks good.  We need to fix the perma-orange, though, of course.  Its concerning that there is any effect with this disabled by default.

::: dom/base/TimeoutManager.cpp
@@ +73,5 @@
> +  double denominator =
> +    std::max(aIsBackground ? gBackgroundBudgetRegenerationFactor
> +                           : gForegroundBudgetRegenerationFactor,
> +             1);
> +  return 1.0 / denominator;

Is there any reason we don't just make the pref a double?  We support floating point prefs.

@@ +134,5 @@
> +      MediaManager::Get()->IsWindowStillActive(mWindow.WindowID())) {
> +    return true;
> +  }
> +
> +  bool active;

Please initialize this for sanity.

@@ +149,5 @@
> +#endif // MOZ_WEBRTC
> +
> +  // Check if we have web sockets
> +  RefPtr<WebSocketEventService> eventService =
> +    WebSocketEventService::GetOrCreate();

Is there just a Get() instead of GetOrCreate()?  It would be nice not to create this in every window using setTimeout().

@@ +212,5 @@
> +  // that if we're setting a timeout while running a timeout we won't
> +  // consider the budget deduction from the currently running
> +  // timeout. That is, we're always one timeout behind when
> +  // considering budget throttling, but this is fine, since the
> +  // average contribution of the budget throttling is the same.

Is this paragraph still true?

@@ +215,5 @@
> +  // considering budget throttling, but this is fine, since the
> +  // average contribution of the budget throttling is the same.
> +  //
> +  // The value that we compute is the time in the future when we again
> +  // have a postive execution budget. We do this by taking the

nit: positive

@@ +222,5 @@
> +  // we should throttle it until the budget again is positive. The
> +  // factor used is the rate of budget regeneration.
> +  //
> +  // We clamp the delay to be less than or equal to
> +  // gBudgetThrottlingMaxDelay to not entirely starve the timeouts.

This also dictates how fast we can respond to changes in the IsActive() state as well.  It would be nice if some of those things could trigger a direct call like the background state change so we could respond faster.

@@ +378,5 @@
> +  // still be counted against the minimum delay.
> +  if (mBudgetThrottleTimeouts) {
> +    bool isBackground = mWindow.IsBackgroundInternal();
> +    double factor = GetRegenerationFactor(isBackground);
> +    TimeDuration regenerated = (aNow - mLastBudgetUpdate).MultDouble(factor);

Can we assert that the regenerated value is not negative?

@@ +430,5 @@
> +    mThrottleTimeouts(false),
> +    mThrottleTrackingTimeouts(false),
> +    mBudgetThrottleTimeouts(false),
> +    mLastBudgetUpdate(TimeStamp::Now()),
> +    mExecutionBudget(GetMaxBudget(true))

Shouldn't we check the window background state instead of forcing true here?

@@ +958,5 @@
> +            // If we ran out of execution budget we need to force a
> +            // reschedule. By cancelling the executor we will not run
> +            // immediately, but instead reschedule to the minimum
> +            // scheduling delay.
> +            if (mExecutionBudget < TimeDuration()) {

Should this be less-or-equal?  The budget has been exhausted, but not quite negative.

::: dom/base/TimeoutManager.h
@@ +234,5 @@
>    bool                        mThrottleTrackingTimeouts;
> +  bool                        mBudgetThrottleTimeouts;
> +
> +  mozilla::TimeStamp          mLastBudgetUpdate;
> +  mozilla::TimeDuration       mExecutionBudget;

Please put these before the bool members to get better memory packing.
Attachment #8882008 - Flags: review?(bkelly) → review+
(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #47)
> Comment on attachment 8882008 [details] [diff] [review]
> 0001-Bug-1362322-Throttle-background-timeouts-using-budge.patch
> 
> Review of attachment 8882008 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall looks good.  We need to fix the perma-orange, though, of course. 
> Its concerning that there is any effect with this disabled by default.
> 
> ::: dom/base/TimeoutManager.cpp
> @@ +73,5 @@E3
> > +  double denominator =
> > +    std::max(aIsBackground ? gBackgroundBudgetRegenerationFactor
> > +                           : gForegroundBudgetRegenerationFactor,
> > +             1);
> > +  return 1.0 / denominator;
> 
> Is there any reason we don't just make the pref a double?  We support
> floating point prefs.

Nope, just an oversight. Will fix.

> @@ +134,5 @@
> > +      MediaManager::Get()->IsWindowStillActive(mWindow.WindowID())) {
> > +    return true;
> > +  }
> > +
> > +  bool active;
> 
> Please initialize this for sanity.
> 
> @@ +149,5 @@
> > +#endif // MOZ_WEBRTC
> > +
> > +  // Check if we have web sockets
> > +  RefPtr<WebSocketEventService> eventService =
> > +    WebSocketEventService::GetOrCreate();
> 
> Is there just a Get() instead of GetOrCreate()?  It would be nice not to
> create this in every window using setTimeout().

I'll have a look.
> 
> @@ +212,5 @@
> > +  // that if we're setting a timeout while running a timeout we won't
> > +  // consider the budget deduction from the currently running
> > +  // timeout. That is, we're always one timeout behind when
> > +  // considering budget throttling, but this is fine, since the
> > +  // average contribution of the budget throttling is the same.
> 
> Is this paragraph still true?

Nope, will change it.

> @@ +215,5 @@
> > +  // considering budget throttling, but this is fine, since the
> > +  // average contribution of the budget throttling is the same.
> > +  //
> > +  // The value that we compute is the time in the future when we again
> > +  // have a postive execution budget. We do this by taking the
> 
> nit: positive
> 
> @@ +222,5 @@
> > +  // we should throttle it until the budget again is positive. The
> > +  // factor used is the rate of budget regeneration.
> > +  //
> > +  // We clamp the delay to be less than or equal to
> > +  // gBudgetThrottlingMaxDelay to not entirely starve the timeouts.
> 
> This also dictates how fast we can respond to changes in the IsActive()
> state as well.  It would be nice if some of those things could trigger a
> direct call like the background state change so we could respond faster.

Right, I'll have a look at that as well.

> @@ +378,5 @@
> > +  // still be counted against the minimum delay.
> > +  if (mBudgetThrottleTimeouts) {
> > +    bool isBackground = mWindow.IsBackgroundInternal();
> > +    double factor = GetRegenerationFactor(isBackground);
> > +    TimeDuration regenerated = (aNow - mLastBudgetUpdate).MultDouble(factor);
> 
> Can we assert that the regenerated value is not negative?

Yep.

> @@ +430,5 @@
> > +    mThrottleTimeouts(false),
> > +    mThrottleTrackingTimeouts(false),
> > +    mBudgetThrottleTimeouts(false),
> > +    mLastBudgetUpdate(TimeStamp::Now()),
> > +    mExecutionBudget(GetMaxBudget(true))
> 
> Shouldn't we check the window background state instead of forcing true here?

Sure, we should.

> @@ +958,5 @@
> > +            // If we ran out of execution budget we need to force a
> > +            // reschedule. By cancelling the executor we will not run
> > +            // immediately, but instead reschedule to the minimum
> > +            // scheduling delay.
> > +            if (mExecutionBudget < TimeDuration()) {
> 
> Should this be less-or-equal?  The budget has been exhausted, but not quite
> negative.

I think less than is better. 0 in a way also means that a timeout can/should run immediately.

> ::: dom/base/TimeoutManager.h
> @@ +234,5 @@
> >    bool                        mThrottleTrackingTimeouts;
> > +  bool                        mBudgetThrottleTimeouts;
> > +
> > +  mozilla::TimeStamp          mLastBudgetUpdate;
> > +  mozilla::TimeDuration       mExecutionBudget;
> 
> Please put these before the bool members to get better memory packing.

Right.
(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #47)
> Comment on attachment 8882008 [details] [diff] [review]
> 0001-Bug-1362322-Throttle-background-timeouts-using-budge.patch
> 
> Review of attachment 8882008 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall looks good.  We need to fix the perma-orange, though, of course. 
> Its concerning that there is any effect with this disabled by default.

At least one of the permas is diagnosed, ShowModalDialog is misbehaving. Trouble is that HasActivePeerConnection is implemented in js, and a failing ShowModalDialog leaves an exception on the context when it calls LeaveModalStatw.

The devtools issue is unresolved though.
Depends on: 1377215
Blocks: 1377766
Attachment #8878058 - Attachment is obsolete: true
Pushed by afarre@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ee4714539c2
Throttle background timeouts using budget. r=bkelly
https://hg.mozilla.org/mozilla-central/rev/0ee4714539c2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.