Closed Bug 1445243 Opened 4 years ago Closed 4 years ago

Disable privacy.reduceTimerPrecision for Talos/AWFY

Categories

(Testing :: Talos, enhancement, P1)

Version 3
enhancement

Tracking

(firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

Details

Attachments

(1 file)

We clamp time exposed by the performance APIs (and other browser APIs accessible to web content) to 2ms; this might throw the results off a bit.

Is this something that is necessary?
if we do a try push to see the effect that would help make a decision.  It is good to be careful with prefs and new features.
I see talos differences, but not AWSY differences.  One benefit looks to be that the noise is reduced (except for windows 7) on all platforms slightly:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=d20c9d41e473&newProject=try&newRevision=806bba662f5e&framework=1&showOnlyNoise=1

that might be worth a shift in perf as this is considering a change to a pref for our internal measurements- if it makes our numbers more reliable I would vote for that :)
Flags: needinfo?(jmaher)
Okay, thanks! I'm interpreting that as WONTFIX; if you disagree, feel free to reopen.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
I'm not sure I see how we get from comment 3 to comment 4. Given that some of our key metrics (like tabpaint) are in the tens of millisecond, 2ms of extra variance is quite significant, and reducing that noise seems very much worthwhile.
Status: RESOLVED → REOPENED
Flags: needinfo?(tom)
Resolution: WONTFIX → ---
I'll do whatever Joel/others think we should do. :)
Flags: needinfo?(tom) → needinfo?(jmaher)
I really don't understand the options here.  I looked at the data from comment 2 and would like to see whatever the 'new' push has landed as it reduces noise.
Flags: needinfo?(jmaher)
I think fixing this is as simple as setting the appropriate prefs in the talos profile.
(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #7)
> I really don't understand the options here.

Right now, timer resolution is clamped to 2ms to prevent timing attacks. The question is whether we should disable that clamping (and reenable high-precision timers) on Talos tests.

> I looked at the data from
> comment 2 and would like to see whatever the 'new' push has landed as it
> reduces noise.

The "new" revision is the one that reenables high-precision timers.
Assignee: nobody → tom
Flags: needinfo?(tom)
Yes, 'fixing' this (or reverting to old behavior) is trivial, just setting a pref.


(In reply to Bobby Holley (:bholley) from comment #9)
> (In reply to Joel Maher ( :jmaher) (UTC-5) from comment #7)
> > I really don't understand the options here.
> 
> Right now, timer resolution is clamped to 2ms to prevent timing attacks. The
> question is whether we should disable that clamping (and reenable
> high-precision timers) on Talos tests.

My understanding from Joel's response was that because the timer precision reduction reduced noise, and it was how users would actually be experiencing the browser in release, he wanted to leave it enabled. 

BUT the timer precision reduction only affects reported times; not how things are rendered (unless you're talking high fps game calculations) so it's not _really_ how users experience the browser so maybe that is not the best approach.



> > I looked at the data from
> > comment 2 and would like to see whatever the 'new' push has landed as it
> > reduces noise.
> 
> The "new" revision is the one that re-enables high-precision timers.

Bug 1435296 is the 2ms bump, and it landed in central in https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=f1a4536966c4 about a month ago.
Flags: needinfo?(tom)
(In reply to Tom Ritter [:tjr] from comment #10)
> Yes, 'fixing' this (or reverting to old behavior) is trivial, just setting a
> pref.
> 
> 
> (In reply to Bobby Holley (:bholley) from comment #9)
> > (In reply to Joel Maher ( :jmaher) (UTC-5) from comment #7)
> > > I really don't understand the options here.
> > 
> > Right now, timer resolution is clamped to 2ms to prevent timing attacks. The
> > question is whether we should disable that clamping (and reenable
> > high-precision timers) on Talos tests.
> 
> My understanding from Joel's response was that because the timer precision
> reduction reduced noise, and it was how users would actually be experiencing
> the browser in release, he wanted to leave it enabled. 
> 
> BUT the timer precision reduction only affects reported times; not how
> things are rendered (unless you're talking high fps game calculations) so
> it's not _really_ how users experience the browser so maybe that is not the
> best approach.

Yes, this is entirely about measurement precision. None of the actual test content uses Date.now()/performance.now() in any meaningful way.

> > > I looked at the data from
> > > comment 2 and would like to see whatever the 'new' push has landed as it
> > > reduces noise.
> > 
> > The "new" revision is the one that re-enables high-precision timers.
> 
> Bug 1435296 is the 2ms bump, and it landed in central in
> https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=f1a4536966c4
> about a month ago.

That's not what Joel is asking. He's asking about the try push in comment 3, which compares two revision, labeled "old" and "new". He wants the revision labeled "new", which is the one where we prefed off timer fuzzing.
Flags: needinfo?(tom)
(In reply to Bobby Holley (:bholley) from comment #11)
> > > > I looked at the data from
> > > > comment 2 and would like to see whatever the 'new' push has landed as it
> > > > reduces noise.
> > > 
> > > The "new" revision is the one that re-enables high-precision timers.
> > 
> > Bug 1435296 is the 2ms bump, and it landed in central in
> > https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=f1a4536966c4
> > about a month ago.
> 
> That's not what Joel is asking. He's asking about the try push in comment 3,
> which compares two revision, labeled "old" and "new". He wants the revision
> labeled "new", which is the one where we prefed off timer fuzzing.

Ah okay; I'll go spelunking and figure out how to flip the pref in Talos. It sounded like AWFY doesn't need anything though?
Flags: needinfo?(tom)
Priority: -- → P1
I think we should fix both. There's no reason to be intentionally adding noise to our performance measurements.
Comment on attachment 8959663 [details]
Bug 1445243 Disable privacy.reduceTimerPrecision for Talos

https://reviewboard.mozilla.org/r/228488/#review234328

looks good. thanks!
Attachment #8959663 - Flags: review?(jmaher) → review+
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d8c5dec11b78
Disable privacy.reduceTimerPrecision for Talos r=jmaher
Keywords: checkin-needed
Backout by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/81633daf4115
Backed out changeset d8c5dec11b78 for Linting failure on builds/worker/checkouts/gecko/testing/talos/talos/config.py
Note that the commend seems kinda backwards, relative to the one above it. Please fix that if you get the chance before the checkin-needed is serviced.
This should be good to go now.
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/22ca6f534784
Disable privacy.reduceTimerPrecision for Talos r=jmaher
https://hg.mozilla.org/mozilla-central/rev/22ca6f534784
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1447295
You need to log in before you can comment on or make changes to this bug.