1325467 - Add some prefs for throttling tracking timeouts

Status: RESOLVED FIXED

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

Description

[(no longer active)]

I'm adding some prefs that will allow us to experiment with various strategies for throttling tracking timeouts.

Comment 1

[(no longer active)]

Attachment: Part 1: Add Timeout::mIsTracking

Comment 2

[(no longer active)]

Attachment: Part 2: Let TimeoutManager::DOMMinTimeoutValue know whethe the timeout being scheduled is tracking

Comment 3

[(no longer active)]

Attachment: Part 3: Add a pref for adjusting the minimum timeout for tracking timeouts

These prefs allow us to customize the treatment for foreground and
background tabs.  By default these timeouts aren't treated differently.

Comment 4

[(no longer active)]

Attachment: Part 4: Add a pref for stretching the time for tracking timeouts

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8821294 [details] [diff] [review]
Part 1: Add Timeout::mIsTracking

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

::: dom/base/TimeoutManager.cpp
@@ +177,5 @@
> +    break;
> +  }
> +  case ALL_NORMAL_TIMEOUT_BUCKETING_STRATEGY:
> +    // timeout->mIsTracking is already false!
> +    MOZ_DIAGNOSTIC_ASSERT(!timeout->mIsTracking);

Is this guaranteed for a timeout that is rescheduled exactly when the pref is changed from a different strategy to this one?  Perhaps the mIsTracking flag should be cleared when the timeout is executed or canceled.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8821298 [details] [diff] [review]
Part 4: Add a pref for stretching the time for tracking timeouts

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

::: dom/base/TimeoutManager.cpp
@@ +45,5 @@
> +  // the input delay and the back pressure delay.
> +  auto factor = aIsTracking ? (isBackground ? gTrackingBackgroundTimeoutStretchFactor
> +                                            : gTrackingTimeoutStretchFactor)
> +                            : 1.0f;
> +  value = T(float(value + aInputDelay) * factor);

Why do you add the input delay to the backpressure delay?  It should really be a std::max() operation per the existing behavior.

Comment 7

[(no longer active)]

(In reply to Ben Kelly [Food Coma / PTO / back Jan 3 :bkelly] from comment #5)
> Comment on attachment 8821294 [details] [diff] [review]
> Part 1: Add Timeout::mIsTracking
> 
> Review of attachment 8821294 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/TimeoutManager.cpp
> @@ +177,5 @@
> > +    break;
> > +  }
> > +  case ALL_NORMAL_TIMEOUT_BUCKETING_STRATEGY:
> > +    // timeout->mIsTracking is already false!
> > +    MOZ_DIAGNOSTIC_ASSERT(!timeout->mIsTracking);
> 
> Is this guaranteed for a timeout that is rescheduled exactly when the pref
> is changed from a different strategy to this one? Perhaps the mIsTracking
> flag should be cleared when the timeout is executed or canceled.

This code isn't involved in the rescheduling case at all, this is only invoked from setTimeout, setInterval and IdleRequest.

Comment 8

[(no longer active)]

(In reply to Ben Kelly [Food Coma / PTO / back Jan 3 :bkelly] from comment #6)
> Comment on attachment 8821298 [details] [diff] [review]
> Part 4: Add a pref for stretching the time for tracking timeouts
> 
> Review of attachment 8821298 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/TimeoutManager.cpp
> @@ +45,5 @@
> > +  // the input delay and the back pressure delay.
> > +  auto factor = aIsTracking ? (isBackground ? gTrackingBackgroundTimeoutStretchFactor
> > +                                            : gTrackingTimeoutStretchFactor)
> > +                            : 1.0f;
> > +  value = T(float(value + aInputDelay) * factor);
> 
> Why do you add the input delay to the backpressure delay?  It should really
> be a std::max() operation per the existing behavior.

You're right...  I don't know why I thought this makes sense before.

Comment 9

[(no longer active)]

Attachment: Part 4: Add a pref for stretching the time for tracking timeouts

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8823905 [details] [diff] [review]
Part 4: Add a pref for stretching the time for tracking timeouts

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

This is an improvement, but I still don't think it does what you want.  You are only getting your stretching for setInterval, nested timers, or background windows.

::: dom/base/TimeoutManager.cpp
@@ +225,5 @@
>    uint32_t nestingLevel = sNestingLevel + 1;
>    uint32_t realInterval = interval;
>    if (aIsInterval || nestingLevel >= DOM_CLAMP_TIMEOUT_NESTING_LEVEL ||
>        mBackPressureDelayMS > 0 || mWindow.IsBackgroundInternal()) {
> +    realInterval = ComputeDelay(realInterval, timeout->mIsTracking);

I think you should move the conditional into ComputeDelay().  For example, you don't actually use your tracking stretch here because the conditional does not take that into account.  Lets just keep all the logic together in one function.

The main complication with doing this is getting the clamping setting in the function.  I suppose you could do something like:

  enum class Clamp {
    Disabled,
    Enabled
  }

  Clamp clamp = (aIsInterval || nestingLevel >= DOM_CLAMP_TIMEOUT_NESTING_LEVEL)
              ? Clamp::Enabled : Clamp::Disabled;

And then at other callsites do:

  ComputeDelay(timeout->mInterval, Clamp::Enabled, timeout->mIsTracking);

Or you to improve things, you could track the clamping state flag on the timeout.  So you could just do:

  ComputeDelay(timeout->mInterval, timeout->mClamping, timeout->mIsTracking);

Comment 11

[(no longer active)]

Comment on attachment 8823905 [details] [diff] [review]
Part 4: Add a pref for stretching the time for tracking timeouts

Looks like I forgot to land this...

So after a fair amount of local testing, I think there is a good chance that we won't need the prefs for stretching the time at all, we should be able to get what we want only with the pref added in part 3, so I'll land the first 3 parts here.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/00c84bfb5005
Part 1: Add Timeout::mIsTracking; r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae1b2492b2fc
Part 2: Let TimeoutManager::DOMMinTimeoutValue know whethe the timeout being scheduled is tracking; r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/eca0ecd94ddc
Part 3: Add a pref for adjusting the minimum timeout for tracking timeouts; r=bkelly

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/00c84bfb5005
https://hg.mozilla.org/mozilla-central/rev/ae1b2492b2fc
https://hg.mozilla.org/mozilla-central/rev/eca0ecd94ddc

Comment 14

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

Why do we have DEFAULT_MIN_TRACKING_BACKGROUND_TIMEOUT_VALUE when it is never used?

Comment 15

[(no longer active)]

(In reply to Olli Pettay [:smaug] (review request backlog because of a work week) from comment #14)
> Why do we have DEFAULT_MIN_TRACKING_BACKGROUND_TIMEOUT_VALUE when it is
> never used?

That's a typo, fixed in bug 1332685.