1445243 - Disable privacy.reduceTimerPrecision for Talos/AWFY

Status: RESOLVED FIXED

(Testing :: Talos, enhancement, P1)

Description

[Tom Ritter [:tjr]]

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?

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 2

[Tom Ritter [:tjr]]

Some results are still coming in, but there do appear to be noticeable differences:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=d20c9d41e473&newProject=try&newRevision=806bba662f5e&framework=1

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

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 :)

Comment 4

[Tom Ritter [:tjr]]

Okay, thanks! I'm interpreting that as WONTFIX; if you disagree, feel free to reopen.

Comment 5

[Bobby Holley (:bholley)]

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.

Comment 6

[Tom Ritter [:tjr]]

I'll do whatever Joel/others think we should do. :)

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 8

[Bobby Holley (:bholley)]

I think fixing this is as simple as setting the appropriate prefs in the talos profile.

Comment 9

[Bobby Holley (:bholley)]

(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.

Comment 10

[Tom Ritter [:tjr]]

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.

Comment 11

[Bobby Holley (:bholley)]

(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.

Comment 12

[Tom Ritter [:tjr]]

(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?

Comment 13

[Bobby Holley (:bholley)]

I think we should fix both. There's no reason to be intentionally adding noise to our performance measurements.

Comment 14

[Tom Ritter [:tjr]]

Attachment: Bug 1445243 Disable privacy.reduceTimerPrecision for Talos

This will make the tests more accurate and less noisy.

Review commit: https://reviewboard.mozilla.org/r/228488/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228488/

Comment 15

[Tom Ritter [:tjr]]

https://github.com/mozilla/arewefastyet/pull/195

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8959663 [details]
Bug 1445243 Disable privacy.reduceTimerPrecision for Talos

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

looks good. thanks!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8c5dec11b78
Disable privacy.reduceTimerPrecision for Talos r=jmaher

Comment 18

[Pulsebot]

Backout by [e-mail]:
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

Comment 19

[Tom Ritter [:tjr]]

Comment on attachment 8959663 [details]
Bug 1445243 Disable privacy.reduceTimerPrecision for Talos

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228488/diff/1-2/

Comment 20

[Bobby Holley (:bholley)]

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.

Comment 21

[Tom Ritter [:tjr]]

Comment on attachment 8959663 [details]
Bug 1445243 Disable privacy.reduceTimerPrecision for Talos

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228488/diff/2-3/

Comment 22

[Tom Ritter [:tjr]]

This should be good to go now.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/22ca6f534784
Disable privacy.reduceTimerPrecision for Talos r=jmaher

Comment 24

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/22ca6f534784