Closed Bug 1385396 Opened 7 years ago Closed 7 years ago

Calling setExperimentActive forces graphics initialization


(Toolkit :: Telemetry, defect, P1)




Performance Impact high
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 + fixed
firefox57 --- fixed


(Reporter: Felipe, Assigned: alexical)




(2 files)

Attached file stack
Calling setExperimentActive forces graphics initialization due to getGfxField("ContentBackend", null) being called [1]

Hopefully there's some way around that, so that an experiment that starts using setExperimentActive doesn't cause the startup order to change.

We first notice this because this was causing the BrowserTabsRemoteAutostart getter to run too early and cache a wrong value.

Stack attached
We should just cache the data from any early startup calls to setExperimentActive() and not initialize the global environment.
Then when the experiment global is actually initialized we can pick that cached data up.
Priority: -- → P1
Whiteboard: [qf]
Whiteboard: [qf] → [qf:p1]
Ran into this same problem working on Bug 1361500. Should we add some asserts in BrowserTabsRemoteAutostart to make it faster to diagnose these problems when they happen?

In any case, I'll work on this unless anyone else was already planning to.
Assignee: nobody → dothayer
Comment on attachment 8893119 [details]
Bug 1385396 - Cache early setExperimentActive calls

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:62
(Diff revision 1)
> +    for (const [id, {branch, options}] of gActiveExperimentStartupBuffer.entries()) {
> +      gGlobalEnvironment.setExperimentActive(id, branch, options);
> +    }
> +    gActiveExperimentStartupBuffer = null;

The other initialization happens inside `function EnvironmentCache()`, lets move this there as well.
[Tracking Requested - why for this release]:
Because bug 1367129 landed in 56
Depends on: 1367129
Comment on attachment 8893119 [details]
Bug 1385396 - Cache early setExperimentActive calls

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:873
(Diff revisions 1 - 2)
>    }
> +  for (const [id, {branch, options}] of gActiveExperimentStartupBuffer.entries()) {
> +    this.setExperimentActive(id, branch, options);
> +  }
> +  gActiveExperimentStartupBuffer = null;

This fails test_TelemetryEnvironment.js with: "gActiveExperimentStartupBuffer is null at resource://gre/modules/TelemetryEnvironment.jsm:870"

You need to reset `gActiveExperimentStartupBuffer` in `testCleanRestart()`.
Attachment #8893119 - Flags: review?(gfritzsche)
Comment on attachment 8893119 [details]
Bug 1385396 - Cache early setExperimentActive calls

Attachment #8893119 - Flags: review?(gfritzsche) → review+
Pushed by
Cache early setExperimentActive calls r=gfritzsche
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Please request Beta approval on this when you get a chance.
Flags: needinfo?(dothayer)
Comment on attachment 8893119 [details]
Bug 1385396 - Cache early setExperimentActive calls

Approval Request Comment
[Feature/Bug causing the regression]: Not a regression, but needed for Bug 1367129.
[User impact if declined]: The e10s cohort will be cached incorrectly.
[Is this code covered by automated tests?]: Partially. The out of order startup case is not, but the existing uses of the API are well covered. I manually tested the startup reordering by adding early setExperimentActive calls and observing my e10s prefs being ignored, and then applying the fix to see them working correctly.
[Has the fix been verified in Nightly?]: No. The code that prompted this change is specific to beta and release. The only way at present to verify this fix is to edit a local build, as detailed above.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: The only code paths affected by this change are code paths that already cause broken e10s behavior without it.
[String changes made/needed]: None.
Flags: needinfo?(dothayer)
Attachment #8893119 - Flags: approval-mozilla-beta?
Comment on attachment 8893119 [details]
Bug 1385396 - Cache early setExperimentActive calls

Needed for e10s metrics. Let's uplift for 56 beta 1.
Attachment #8893119 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in before you can comment on or make changes to this bug.