1600565 - FORGET_SKIPPABLE_FREQUENCY telemetry increase in Firefox Nightly builds from 2019-11-27

Status: RESOLVED FIXED

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

Description

[Jon Coppeard (:jonco)]

As detected by Cerberus, there was an increase in FORGET_SKIPPABLE_FREQUENCY after bug 1599445 landed:

https://telemetry.mozilla.org/new-pipeline/evo.html#!aggregates=Median!Mean!5th%2520percentile!25th%2520percentile!75th%2520percentile!95th%2520percentile&cumulative=0&end_date=null&include_spill=0&keys=&max_channel_version=nightly%252F72&measure=FORGET_SKIPPABLE_FREQUENCY&min_channel_version=nightly%252F72&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&start_date=null&trim=1&use_submission_date=0

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Bug 1600565 - Fix changes to CCRunnerFired made during previous refactoring r?mccr8

One thing I inadvertantly changed was that didDoWork gets set to true even if we don't call FireForgetSkippable and execute what was the third branch of the if statement:

https://searchfox.org/mozilla-central/rev/883d58bccf1a36465ef5ee14b62db0a9925ab85b/dom/base/nsJSEnvironment.cpp#1878-1883

Comment 2

[Jon Coppeard (:jonco)]

I'll leave this open until we can verify that the telemetry has returned to previous levels.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6ed525dce968
Fix changes to CCRunnerFired made during previous refactoring r=mccr8

Comment 4

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/6ed525dce968

Comment 5

[Jon Coppeard (:jonco)]

Closing as the telemetry looks like it's returned to previous levels:

https://telemetry.mozilla.org/new-pipeline/evo.html#!aggregates=Median!Mean!5th%2520percentile!25th%2520percentile!75th%2520percentile!95th%2520percentile&cumulative=0&end_date=null&include_spill=0&keys=!__none__!__none__&max_channel_version=nightly%252F73&measure=FORGET_SKIPPABLE_FREQUENCY&min_channel_version=nightly%252F72&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&start_date=null&trim=1&use_submission_date=0

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Does this need a Beta uplift request?

Comment 7

[Jon Coppeard (:jonco)]

Comment on attachment 9113863 [details]
Bug 1600565 - Fix changes to CCRunnerFired made during previous refactoring r?mccr8

Beta/Release Uplift Approval Request

• User impact if declined: Could affect performance by needlessly running ForgetSkippable too often
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a tricky part of the code. But this fix has been on central for 6 days and telemetry indicates that it has fixed the problem. So I'd say it's better to take the fix than leave the regression in 72.
• String changes made/needed:

Comment 8

[Julien Cristau [:jcristau]]

Comment on attachment 9113863 [details]
Bug 1600565 - Fix changes to CCRunnerFired made during previous refactoring r?mccr8

regression fix for 72.0b8

Comment 9

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/63a39a6f28d1