Add some prefs for throttling tracking timeouts

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

(Blocks 1 bug)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

I'm adding some prefs that will allow us to experiment with various strategies for throttling tracking timeouts.
Assignee: nobody → ehsan
Blocks: QuantumDOM
Attachment #8821294 - Flags: review?(bkelly)
These prefs allow us to customize the treatment for foreground and
background tabs.  By default these timeouts aren't treated differently.
Attachment #8821296 - Flags: review?(bkelly)
Depends on: 1312514
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.
Attachment #8821294 - Flags: review?(bkelly) → review+
Attachment #8821295 - Flags: review?(bkelly) → review+
Attachment #8821296 - Flags: review?(bkelly) → review+
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.
Attachment #8821298 - Flags: review?(bkelly) → review-
(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.
(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.
Attachment #8821298 - Attachment is obsolete: true
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);
Attachment #8823905 - Flags: review?(bkelly) → review-
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.
Attachment #8823905 - Attachment is obsolete: true
Pushed by eakhgari@mozilla.com:
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
Why do we have DEFAULT_MIN_TRACKING_BACKGROUND_TIMEOUT_VALUE when it is never used?
(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.
Depends on: 1332685
Depends on: 1332776
Depends on: 1338691
Blocks: 1355311
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.