Closed Bug 1222894 Opened 9 years ago Closed 9 years ago

[meta] Run an e10s A/B experiment on Beta 43

Categories

(Firefox :: General, defect)

42 Branch
defect
Not set
normal

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox43 - ---

People

(Reporter: vladan, Assigned: vladan)

References

Details

Attachments

(2 files, 8 obsolete files)

We're not doing an opt-in on Beta 43 anymore (bug 1218475) and we have Nick's approval to do an A/B experiment on Beta. This will be the same experiment code as bug 1193089, but we should wait for a few issues to be fixed and uplifted (see blockers)
Summary: Run an e10s A/B experiment on Beta 43 → [meta] Run an e10s A/B experiment on Beta 43
Depends on: 1222895
Depends on: 1220198
No longer depends on: 1218266
Tracking this so I know what's up for 43.
The current plan is to run this experiment in beta 4 of Beta 43. Beta 4 will be built on Monday, Nov 16th and it will be released on Wednesday, Nov 18th.
Depends on: 1223800
Depends on: 1211411
Depends on: 1219751
Depends on: 1215540
No longer depends on: 1221674
Noting that there are a few patches about to uplift and land on beta 4, bug 1213780, bug 1211411, bug 1215540, and bug 1219751.
Also, usually we release beta on Tuesday. As far as I know, beta 4 should release on Tuesday Nov. 17th. The timing depends on QE signoff and whether we run into problems.
Attached patch e10s experiment on beta (obsolete) (deleted) — Splinter Review
This is the updated TelemetryExperiment for Beta 43 Notes: - Experiment start date is Nov 18 noon EST, end date is Dec 18 noon EST, max duration is still 10 days - Sampling is set to 15% of Beta population - There is a concurrent search experiment running on 10% of Beta population - I'll also post an interdiff against the previous experiment patch Questions: - How do I restrict this experiment to beta4 and higher? manifest.json's version field doesn't like the b4 suffix. Is there a buildID field?
Attachment #8689913 - Flags: review?(rvitillo)
Attachment #8689913 - Flags: review?(felipc)
Attached patch interdiff patch (obsolete) (deleted) — Splinter Review
Depends on: 1226487
(In reply to Vladan Djeric (:vladan) -- please needinfo! PTO Nov 16/19/20 from comment #5) > Questions: > - How do I restrict this experiment to beta4 and higher? manifest.json's > version field doesn't like the b4 suffix. Is there a buildID field? Have a look at https://dxr.mozilla.org/mozilla-central/source/browser/experiments/docs/manifest.rst#163
Comment on attachment 8689913 [details] [diff] [review] e10s experiment on beta Review of attachment 8689913 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me.
Attachment #8689913 - Flags: review?(rvitillo) → review+
(In reply to Vladan Djeric (:vladan) -- please needinfo! PTO Nov 16/19/20 from comment #5) > Created attachment 8689913 [details] [diff] [review] > e10s experiment on beta > > This is the updated TelemetryExperiment for Beta 43 > > Notes: > - Experiment start date is Nov 18 noon EST, end date is Dec 18 noon EST, max > duration is still 10 days Makes me sad that it will be active while we need to qualify beta as being ready for release. I probably will have to guess if we are fine to release as I do not have the tooling right now to qualify without counting those e10s users in the experiment and there is 0 hours time left for me to figure something out this year.
Comment on attachment 8689913 [details] [diff] [review] e10s experiment on beta Review of attachment 8689913 [details] [diff] [review]: ----------------------------------------------------------------- The backupAndSetPref/restorePref functions (used for POPUP_PREF/DISPLAY_PREF) will not be used in beta, so I'd prefer to have these parts removed from this experiment.
Attachment #8689913 - Flags: review?(felipc) → feedback+
This experiment hasn't started yet, and there is still one more thing to uplift for Thursday's beta 7 desktop build. So we can aim for after that and see what data we can gather. For measuring the non-e10s crash rate for 43, I'm hoping we can figure that out from crash-stats searches but I realize KaiRo's usual tools may not be prepared yet to handle two streams in beta. KaiRo is there anything we can do in support?
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #11) > For measuring the non-e10s crash rate for 43, I'm hoping we can figure that > out from crash-stats searches but I realize KaiRo's usual tools may not be > prepared yet to handle two streams in beta. KaiRo is there anything we can > do in support? With the tools I have in my hands right now, what we can provide is searches for crashes where e10s is vs. search for crashes where it's off. That's all I have. Crash rates we cannot measure with the tooling I have at this time. We can do that via Telemetry, but I will only get my first introduction into how to get any data from there at Mozlando, so I don't expect me to be of any help with that before Q1. (In fact, I know that running any experiment like this will mess up the only crash rate data I have for reporting right now and I will need to make educated guesses on beta health of the release until I have both better tooling based on Telemetry and historic comparative data of those metrics over some time.) So, someone well-versed with Telemetry will need to provide comparative crash rate data between the experiment buckets. I can help with search queries of submitted crashes where needed.
Roberto, are we filtering a11y users out of this set at all? I don't think we track a11y across sessions so it's probably not possible.
Flags: needinfo?(rvitillo)
(In reply to Jim Mathies [:jimm] from comment #13) > Roberto, are we filtering a11y users out of this set at all? I don't think > we track a11y across sessions so it's probably not possible. actually, this might be a better question for felipe..
Flags: needinfo?(felipc)
The experiment code itself does not check for a11y, but we are enabling the e10s a11y checks on beta in bug 1226487 which blocks this one. But now that you mention, it might be a good idea to check for that in the experiment and not even attempt to include users as part of the test population if they had a11y enabled.
Flags: needinfo?(felipc)
Flags: needinfo?(rvitillo)
(In reply to :Felipe Gomes from comment #15) > The experiment code itself does not check for a11y, but we are enabling the > e10s a11y checks on beta in bug 1226487 which blocks this one. > > But now that you mention, it might be a good idea to check for that in the > experiment and not even attempt to include users as part of the test > population if they had a11y enabled. I don't think we can check if it was enabled, but maybe we can check if it's currently enabled and if so, skip off those users? I don't see much in the current accessibility prefs to indicate a past run.
With the change in bug 1226487, it should set browser.tabs.remote.disabled-for-a11y whenever a11y runs, regardless if e10s was running or not. That way we can know the value of that pref in advance (a simpler version of the time-gated alternative that you're implementing)
Attached patch beta experiment v2 (obsolete) (deleted) — Splinter Review
* Experiment is now set to run from Friday Nov 27th to Monday Dec 7th (roughly 10 days). This means the experiment will stop about a week before beta 43 becomes Release 43 to allow RelMan to assess Beta stability before release. * The minimum buildID is beta7 or later. We need to use unofficially builds to test (see next comment) * I removed the POPUP_PREF/DISPLAY_PREF prefs that don't exist on Beta * I removed the checks for changes in default value of browser.tabs.remote.autostart.2 since this pref does not have a default value on Beta * I added 2 new branches "first-session-[control|experiment]"
Attachment #8692781 - Flags: review?(rvitillo)
Attachment #8692781 - Flags: review?(felipc)
Does the experiment code need to check that accessibility pref?
Flags: needinfo?(felipc)
Let's do some preliminary tests on this experiment tomorrow A few points: * There will be some additional cases to check around disabling the experiment when accessibility is in effect. * We need to test with an unreleased Beta build because a follow-up patch in bug 1226487 hasn't landed on the beta tree yet * There is an unofficial build ready here https://treeherder.mozilla.org/#/jobs?repo=try&revision=444d206f97bb&selectedJob=14176668, but you'll need to modify the channel to "beta" in defaults\pref\channel-prefs.js of the program (not profile) directory after installing
Flags: needinfo?(kjozwiak)
Attachment #8692781 - Flags: review?(rvitillo) → review+
Comment on attachment 8692781 [details] [diff] [review] beta experiment v2 Review of attachment 8692781 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #19) > Does the experiment code need to check that accessibility pref? Yeah, not strictly necessary but it would be nice (and easy). Just check that pref and don't try to enable e10s for these users (you could set a new branch "disabled-because-a11y"). Also, that pref might change on later sessions. You don't need to do anything about this if you don't want (you'll fall in the browserTabsRemoteAutostart != expected branch, and the branch will be set to force-disabled).
Attachment #8692781 - Flags: review?(felipc) → review+
The extra tests to perform would be: 1: - Run a beta7 build before the experiment starts, and use any tool that triggers accessibility (remote desktop for example). Make sure that the disabled-for-a11y pref is set - Restart firefox and don't use accessibility again - Install experiment - Make sure the experiment doesn't try to enable e10s, because a11y was used in this profile before 2: - Run a beta7 build, and install experiment before any a11y is used (force the branch to test to make sure e10s is activated) - Restart Firefox (so e10s is actually running) - Use an accessibility tool - Make sure that the popup saying that e10s must be disabled shows up - Accept the popup (which will restart Firefox) - Make sure that e10s got disabled, and that the experiment got set to force-disabled
Flags: needinfo?(felipc)
Attached patch beta experiment v3 patch (obsolete) (deleted) — Splinter Review
Added a check for the browser.tabs.remote.disabled-for-a11y pref, and a new experiment "a11y-disabled" branch
Assignee: nobody → vladan.bugzilla
Attachment #8689913 - Attachment is obsolete: true
Attachment #8689914 - Attachment is obsolete: true
Attachment #8692781 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8693080 - Flags: review+
So this adds to the experiment code the code that didn't get uplifted to beta 7. If accessibility is enabled on startup, or if it eventually gets enabled while running, it would set the pref browser.tabs.remote.disabled-for-a11y On next startup the browser won't use e10s anymore, and the experiment will figure that out and set the branch correctly (Untested)
Attachment #8693102 - Flags: review?(vladan.bugzilla)
Comment on attachment 8693102 [details] [diff] [review] Disable e10s (by setting the disabled-for-a11y pref) manually Review of attachment 8693102 [details] [diff] [review]: ----------------------------------------------------------------- ::: experiments/e10s-enabled-beta/code/bootstrap.js @@ +68,5 @@ > } > > + if (A11YWatcher.disabledForA11y) { > + return; > + } shouldn't we have this check after the switch-statement? i.e. the experiment check this flag on startup when it's also on the first-session-experiment or experiment branch
That is not really necessary but it avoids a race in A11YWatcher setting the branch (which is async), and we getting a null branch and calling Math.random() to possibly enable e10s for this user.
Attachment #8693102 - Flags: review?(vladan.bugzilla) → review+
Attached patch Merged patch with new xpi (obsolete) (deleted) — Splinter Review
Attached patch Merged patch, now with new xpi added (obsolete) (deleted) — Splinter Review
Attachment #8693147 - Attachment is obsolete: true
Attached patch Merged patch, v2 (deleted) — Splinter Review
The autostart.2 pref wasn't being reset when A11YWatcher switch the branch to a11y-disabled
Attachment #8693080 - Attachment is obsolete: true
Attachment #8693102 - Attachment is obsolete: true
Attachment #8693148 - Attachment is obsolete: true
Went through verification using the following builds: - https://archive.mozilla.org/pub/firefox/releases/43.0b7/ - http://archive.mozilla.org/pub/firefox/try-builds/vdjeric@mozilla.com-1cd5affd9b77b97f86ecc3125dca7b718d23d8d5/try-win32/ * Note: a11y testing was done on both of the builds listed below. All other testing was done using the currently released 43.0b7 build * Test Cases Used: https://public.etherpad-mozilla.org/p/e10sTelemetryExperimentBETA - if someone could take a quick look to make sure nothing obvious has been missed, that would much appreciated :) Possible Issues: ================ Issue #1: It looks like the experiment is still being installed even though datareporting.healthreport.service.enabled or datareporting.healthreport.uploadEnabled are set as false. Should the experiment be installed if these two prefs are set as false? The filter function is working correctly with toolkit.telemetry.enabled. datareporting.healthreport.service.enabled;false <-- experiment is still being installed datareporting.healthreport.uploadEnabled;false <--- experiment is still being installed toolkit.telemetry.enabled;false <-- PASSED (jsfilter-false) Issue #2: When using the try-build, the "would you like to disable e10s" doorhanger only appears when the user is in the "experiment" branch and not the "control" branch (which makes sense). If you use an accessible tool while you're on the "control" branch, the behaviour is exactly the same as if you were on the "experiment" branch but without the doorhanger. Just making sure that the "control" branch shouldn't be receiving a doorhanger as well. I'm guessing no because e10s isn't enabled so it's pointless but I would rather make sure then guess :) Other worthwhile mentions: - the "would you like to disable e10s" doorhanger only appears under the try-build and not the released 43.0b7 build
Flags: needinfo?(kjozwiak)
Forgot to mention this in the earlier comment :/ When you select "Don't Disable" under the "Disable e10s" doorhanger via the try-build, after restarting the browser you'll get the following: - browser.tabs.remote.autostart.2;true will be removed - browser.tabs.remote.disabled-for-a11y;false - Services.appinfo.browserTabsRemoteAutostart -> false (browser console) Cu.import("resource:///modules/experiments/Experiments.jsm"); Experiments.instance()._getActiveExperiment().branch; "a11y-disabled" Is this expected? I assumed "Don't Disable" wouldn't disable e10s?
It is correct in that neither control nor experiment should be running a11y and e10s isn't officially available outside the experiment. ...autostart.2=true is used as indication that e10s is on by default. bug 1198459 is likely to make changes for 44. My expectation is the vast majority a11y users will go straight onto the a11y-disabled branch. per comment 11/12: manual crash-stats search lets you pick the experiment and branch. The results are slightly poisoned due to the branch being set before a restart.
Attached file experiment.xpi (obsolete) (deleted) —
Jake: can you sign this experiment XPI and attach the signed version to this bug?
Flags: needinfo?(jthomas)
(In reply to Kamil Jozwiak [:kjozwiak] from comment #30) > Issue #1: > > It looks like the experiment is still being installed even though > datareporting.healthreport.service.enabled or > datareporting.healthreport.uploadEnabled are set as false. Should the > experiment be installed if these two prefs are set as false? The filter > function is working correctly with toolkit.telemetry.enabled. > > datareporting.healthreport.service.enabled;false <-- experiment is still > being installed > datareporting.healthreport.uploadEnabled;false <--- experiment is still > being installed > toolkit.telemetry.enabled;false <-- PASSED (jsfilter-false) Yeah, this is fine. There's no way for us to work around that. We'll depend on a general fix in bug 1217282. This is not really bad because 1) people would likely have both disabled, 2) if those prefs are false, no data from the user is sent anyway. They will just get a brief period of e10s.. > > Issue #2: > > When using the try-build, the "would you like to disable e10s" doorhanger > only appears when the user is in the "experiment" branch and not the > "control" branch (which makes sense). If you use an accessible tool while > you're on the "control" branch, the behaviour is exactly the same as if you > were on the "experiment" branch but without the doorhanger. Just making sure > that the "control" branch shouldn't be receiving a doorhanger as well. I'm > guessing no because e10s isn't enabled so it's pointless but I would rather > make sure then guess :) correct > > Other worthwhile mentions: > > - the "would you like to disable e10s" doorhanger only appears under the > try-build and not the released 43.0b7 build correct (In reply to Kamil Jozwiak [:kjozwiak] from comment #31) > Forgot to mention this in the earlier comment :/ > > When you select "Don't Disable" under the "Disable e10s" doorhanger via the > try-build, after restarting the browser you'll get the following: > > - browser.tabs.remote.autostart.2;true will be removed > - browser.tabs.remote.disabled-for-a11y;false > - Services.appinfo.browserTabsRemoteAutostart -> false (browser console) > > Cu.import("resource:///modules/experiments/Experiments.jsm"); > Experiments.instance()._getActiveExperiment().branch; > "a11y-disabled" > > Is this expected? I assumed "Don't Disable" wouldn't disable e10s? This is a bit unfortunate, but we can live with that. The experiment itself forces-disable e10s if accessibility was used, which will override the doorhanger decision.
Attached file experiment.xpi (deleted) —
Signed experiment.xpi.
Attachment #8693299 - Attachment is obsolete: true
Flags: needinfo?(jthomas)
The e10s A/B experiment is no longer shipping on Beta 43. We ran into delays with Beta not being ready for e10s (bug 1226487) and there is a consensus that the current Beta 8 is too late to ship the experiment, since uplift is less than 2 weeks away. Instead, we'll be running the Beta experiment in 2 weeks on Beta 44 beta1. The tracking bug is bug 1229104.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
Blocks: 1229104
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: