Closed
Bug 1349363
Opened 6 years ago
Closed 6 years ago
[e10s-multi] Beta 54 experiment
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 55
People
(Reporter: mrbkap, Assigned: mrbkap)
References
Details
(Whiteboard: [e10s-multi:+])
Attachments
(8 files)
59 bytes,
text/x-review-board-request
|
Felipe
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
Felipe
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
Felipe
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
Felipe
:
review+
liuche
:
review+
|
Details |
6.92 KB,
patch
|
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
4.74 KB,
patch
|
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
8.82 KB,
patch
|
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
2.42 KB,
patch
|
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
This bug tracks the changes needed for the Beta 54 experiment for e10s multi. Here are some parameters that we have discussed (pieced together from multiple documents, Erin tell me if I misunderstood): 90% of users using e10s. Of them, -> 45% use 1 content process (i.e. e10s as it shipped) -> 15% use 2 content processes -> 15% use 4 content processes -> 15% use 8 content processes 10% of users will get single-process Firefox. I don't know if we wanted to add additional criteria (outside of non-Webextension add-ons).
Assignee | ||
Updated•6 years ago
|
Whiteboard: [e10s-multi:?]
Comment 1•6 years ago
|
||
:chutten do the above proportions look correct to you? :shell, is there anything we need to consider besides disabling e10s for boot strapped add-ons as outlined in Bug 1346288? Thanks both!
Flags: needinfo?(sescalante)
Flags: needinfo?(chutten)
Updated•6 years ago
|
Whiteboard: [e10s-multi:?] → [e10s-multi:+]
Comment 2•6 years ago
|
||
I'm not sure what the point of having a single-process cohort is. e10s is already released, so testing e10s-eligible clients running !e10s seems... odd. Therefore, I recommend ditching the single-process cohort and dispersing its members across the other cohorts. So long as 15% is large enough to get good stats out of for infrequent events (like slow script notices, hangs, crashes), then I have no problem with the proportions. If 15% _isn't_ big enough, I like that the control branch (e10s as shipped) is the same size as a combined test branch (all the multis). This is more out of liking when numbers line up more than any particular piece of statistical rigour, though :) Oh, you'll probably want a definition of "big enough"... uh... ask Ben what the size of his addons cohort was in 50 when he couldn't get confidence on slow script numbers. Bigger than that :)
Flags: needinfo?(chutten)
Comment 3•6 years ago
|
||
ok, I reviewing my notes, I think we can cut down the single process group. Shell let us know she "will only needed about 10% we'll e10s no multi with and without add-ons for experiments adding more add-ons (that don't use shims) and just removing shims entirely." :Chutten: ^ and :mrbkap: ^
Assignee | ||
Comment 4•6 years ago
|
||
I was hoping to have a patch for this today but hit some snags. I should have something ready by tomorrow.
Assignee | ||
Comment 5•6 years ago
|
||
Obviously, that didn't happen. Here's my current plan: * Introduce nsIXULAppInfo::browserUseMultipleWebProcesses (and a related function in nsAppRunner.cpp) analogous to BrowserTabsRemoteAutostart. * Use the new function in all of the places where we use "dom.ipc.processCount" in the code. * Use dom.ipc.processCount.web as the programmatic way for the experiment addon to set the process count for the user. That way, users can opt out or in of the experiment by setting "dom.ipc.processCount" to 1 or more, respectively. At the same time, the addon can set the .web version of the pref without confusing itself as to whether there's a "user-set" value. Once we have that, I think the rest should be pretty simple: If we're on "esr" or "release", we have a 100% cohort, no multi. Otherwise, if we're on "beta" and we would have been enabling the e10s experiment, I can break us into further groups and use those settings. I've been traveling the last couple of days, so I probably won't get to this before Monday or Tuesday of next week.
Flags: needinfo?(felipc)
Comment 7•6 years ago
|
||
We're dangerously close to Beta 1 of 54 - what is the status of this?
Flags: needinfo?(mrbkap)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•6 years ago
|
||
I've done some testing that opting in and opting out work. I haven't tested the non-WebExtension add-on stuff. Felipe, one thing I went back and forth on was whether to add a new user sample out of 1 or to re-use the e10s sample. This way seemed clearer to me, though, so I went with it.
Assignee | ||
Comment 16•6 years ago
|
||
Oh, I also wanted to avoid adding prefs for each (potential) experiment, so I took this approach for opting out. If a user really wants to opt out for all time, they can set the pref to a large value. Otherwise, setting the pref to the current experiment will leave them eligible for the next experiment.
Comment 17•6 years ago
|
||
mozreview-review |
Comment on attachment 8858963 [details] Bug 1349363 - Centralize pref-checking code for e10s-multi control. https://reviewboard.mozilla.org/r/130962/#review133858 I'll leave this r? pending in case I misunderstood what I commented below. If you agree with the comment and it's correct to remove that check, then consider this an r+ with that fixed. ::: toolkit/xre/nsAppRunner.cpp:5113 (Diff revision 2) > + return std::max(1, Preferences::GetInt(optInPref, 1)); > + } > + > + // If there are add-ons that would make the user's experience poor, don't > + // use more than one web content process. > + if (Preferences::GetBool("extensions.e10sMultiBlocksEnabling", false) || I'm not sure this is right. This is just the pref to enable the checking of addons for the e10s-multi criteria. Its value should be false for Nightly and true for Beta. You could use a && instead, but I believe all you need is check the second pref, "e10sMultiBlockedByAddons".
Attachment #8858963 -
Flags: review?(felipc)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 22•6 years ago
|
||
mozreview-review |
Comment on attachment 8858964 [details] Bug 1349363 - Use a centralized function to tell if e10s-multi is on. https://reviewboard.mozilla.org/r/130964/#review133870 I'm still a bit confused about how dom.ipc.processCount will override things, but I gue
Attachment #8858964 -
Flags: review?(felipc) → review+
Comment 23•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8858964 [details] Bug 1349363 - Use a centralized function to tell if e10s-multi is on. https://reviewboard.mozilla.org/r/130964/#review133870 Sry this comment was left as a draft in reviewboard, but you explained to me how it works on irc
Comment 24•6 years ago
|
||
mozreview-review |
Comment on attachment 8858965 [details] Bug 1349363 - Run an experiment in beta for e10s-multi. https://reviewboard.mozilla.org/r/130966/#review133898 ::: browser/extensions/e10srollout/bootstrap.js:150 (Diff revision 3) > + } else if (getAddonsDisqualifyForMulti()) { > + // If addons prevent us from enabling e10s-multi, allow for e10s to be > + // enabled with a single content process. This will give us more users in > + // the single content process category than intended, but that's fine. > + setCohort(`${cohortPrefix}multiDisqualifiedByAddons`); > + Preferences.set(PREF_TOGGLE_E10S, true); > + Preferences.reset(PREF_E10S_PROCESSCOUNT + ".web"); I believe we can invert things a bit here.. This will end up putting everyone who has non-webextensions in this group, without distrubuting them to test/control. I believe you can just entirely remove this here, and in the block " // Don't enable e10s-multi on any channel", also check `getAddonsDisqualifyForMulti()`, and, in that case, do the early return. That way we will get: - 10% control users - 90% test And from the test users, everyone who qualifies for multi will be part of the multi experiment (and since 25% will remain as multi-1-process, the single process case continues to be tested well) ::: browser/extensions/e10srollout/bootstrap.js:171 (Diff revision 3) > + > + // Don't enable e10s-multi on any channel other than beta. If the user opted > + // into multi, then they've already set the proper prefs and we don't need > + // to do anything more. > + if (updateChannel !== "beta" || !inMultiExperiment || userOptedIn.multi) { > + return; Before returning, please reset PREF_E10S_PROCESSCOUNT + ".web", so that it's always clear ::: browser/extensions/e10srollout/bootstrap.js:185 (Diff revision 3) > + }; > + > + let multiUserSample = getUserSample(true); > + for (let sampleName of Object.getOwnPropertyNames(BUCKETS)) { > + if (multiUserSample < BUCKETS[sampleName]) { > + Preferences.set(PREF_E10S_PROCESSCOUNT + ".web", sampleName); You'll also want to set a cohort name here to make it easier to track this on dashboards, etc. (and not to not get this mixed up with the existing cohorts). I suggest ``multi-bucket${sampleName}``
Attachment #8858965 -
Flags: review?(felipc)
Comment 25•6 years ago
|
||
mozreview-review |
Comment on attachment 8859290 [details] Bug 1349363 - Track both .processCount and .web to get a complete view of the user's content processes. https://reviewboard.mozilla.org/r/131302/#review133910 ::: toolkit/components/telemetry/TelemetryEnvironment.jsm:189 (Diff revision 1) > ["devtools.chrome.enabled", {what: RECORD_PREF_VALUE}], > ["devtools.debugger.enabled", {what: RECORD_PREF_VALUE}], > ["devtools.debugger.remote-enabled", {what: RECORD_PREF_VALUE}], > ["dom.ipc.plugins.asyncInit.enabled", {what: RECORD_PREF_VALUE}], > ["dom.ipc.plugins.enabled", {what: RECORD_PREF_VALUE}], > - ["dom.ipc.processCount", {what: RECORD_PREF_VALUE, requiresRestart: true}], > + ["dom.ipc.processCount", {what: RECORD_PREF_VALUE}], thanks for fixing this one ::: toolkit/components/telemetry/TelemetryEnvironment.jsm:190 (Diff revision 1) > ["devtools.debugger.enabled", {what: RECORD_PREF_VALUE}], > ["devtools.debugger.remote-enabled", {what: RECORD_PREF_VALUE}], > ["dom.ipc.plugins.asyncInit.enabled", {what: RECORD_PREF_VALUE}], > ["dom.ipc.plugins.enabled", {what: RECORD_PREF_VALUE}], > - ["dom.ipc.processCount", {what: RECORD_PREF_VALUE, requiresRestart: true}], > + ["dom.ipc.processCount", {what: RECORD_PREF_VALUE}], > + ["dom.ipc.processCount.web", {what: RECORD_PREF_VALUE}], Since this pref is managed by the code, I wouldn't even track it here (when the code sets it, it will trigger an unecessary telemetry ping split). r+ with this removed
Attachment #8859290 -
Flags: review?(felipc) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 30•6 years ago
|
||
mozreview-review |
Comment on attachment 8858963 [details] Bug 1349363 - Centralize pref-checking code for e10s-multi control. https://reviewboard.mozilla.org/r/130962/#review133914
Attachment #8858963 -
Flags: review?(felipc) → review+
Comment 31•6 years ago
|
||
mozreview-review |
Comment on attachment 8858965 [details] Bug 1349363 - Run an experiment in beta for e10s-multi. https://reviewboard.mozilla.org/r/130966/#review133916 \o/
Attachment #8858965 -
Flags: review?(felipc) → review+
Comment 32•6 years ago
|
||
mozreview-review |
Comment on attachment 8859290 [details] Bug 1349363 - Track both .processCount and .web to get a complete view of the user's content processes. https://reviewboard.mozilla.org/r/131302/#review133918
Assignee | ||
Comment 33•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cab1830aa715347748a0cb518daf7b5fd222a7b3 My original try push was slightly orange. I believe it has to do with the two addons-related prefs logical operator confusion (|| vs &&). I'm hoping this push will stay green. I've also added a dependency on bug 1346288 which much land before this bug on beta.
Assignee | ||
Comment 34•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4b082d38fae12c2e150fe99bfef4a5d4c1abc6e (with the fully updated patch :-/)
Comment 35•6 years ago
|
||
Pushed by mrbkap@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fa4cbef6f517 Centralize pref-checking code for e10s-multi control. r=Felipe https://hg.mozilla.org/integration/autoland/rev/c0c849826a14 Use a centralized function to tell if e10s-multi is on. r=Felipe https://hg.mozilla.org/integration/autoland/rev/483168efae35 Run an experiment in beta for e10s-multi. r=Felipe https://hg.mozilla.org/integration/autoland/rev/bf54a5711184 Track both .processCount and .web to get a complete view of the user's content processes. r=Felipe
Comment 36•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fa4cbef6f517 https://hg.mozilla.org/mozilla-central/rev/c0c849826a14 https://hg.mozilla.org/mozilla-central/rev/483168efae35 https://hg.mozilla.org/mozilla-central/rev/bf54a5711184
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Assignee | ||
Comment 37•6 years ago
|
||
Assignee | ||
Comment 38•6 years ago
|
||
MozReview-Commit-ID: JQJtCanIv3a
Assignee | ||
Comment 39•6 years ago
|
||
Assignee | ||
Comment 40•6 years ago
|
||
Assignee | ||
Comment 41•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a272037cce32ef689407fe0b5da1a58937cb347
Assignee | ||
Comment 42•6 years ago
|
||
Comment on attachment 8859723 [details] [diff] [review] First patch rebased to beta Approval Request Comment [Feature/Bug causing the regression]: e10s-multi (not a regression) [User impact if declined]: n/a [Is this code covered by automated tests?]: No. [Has the fix been verified in Nightly?]: No. [Needs manual test from QE? If yes, steps to reproduce]: Yes, see bug 1352388. [List of other uplifts needed for the feature/fix]: bug 1346288 [Is the change risky?]: Not really. [Why is the change risky/not risky?]: While this changes the meaning of a couple of prefs, the patch tries very hard to respect the user's current opinion. The experiment code itself might fail, but if it does so then there will be no visible bugs to the end users, only a lack of incoming information for us. [String changes made/needed]: n/a
Attachment #8859723 -
Flags: approval-mozilla-beta?
Assignee | ||
Updated•6 years ago
|
Attachment #8859724 -
Flags: approval-mozilla-beta?
Assignee | ||
Updated•6 years ago
|
Attachment #8859725 -
Flags: approval-mozilla-beta?
Assignee | ||
Updated•6 years ago
|
Attachment #8859726 -
Flags: approval-mozilla-beta?
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(sescalante)
Assignee | ||
Comment 43•6 years ago
|
||
Comment on attachment 8859723 [details] [diff] [review] First patch rebased to beta Approval Request Comment: same as for beta
Attachment #8859723 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 44•6 years ago
|
||
Comment on attachment 8859723 [details] [diff] [review] First patch rebased to beta Mossop pointed out that as of today Aurora is no more.
Attachment #8859723 -
Flags: approval-mozilla-aurora?
Updated•6 years ago
|
status-firefox54:
--- → affected
Comment 45•6 years ago
|
||
Comment on attachment 8859723 [details] [diff] [review] First patch rebased to beta Support e10s-multi in Beta54. Beta54+. Should be in 54 beta 1.
Attachment #8859723 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•6 years ago
|
Attachment #8859724 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•6 years ago
|
Attachment #8859725 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•6 years ago
|
Attachment #8859726 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 46•6 years ago
|
||
Got conflicts, please rebase. grafting 404033:fa4cbef6f517 "Bug 1349363 - Centralize pref-checking code for e10s-multi control. r=Felipe" merging dom/ipc/ContentParent.cpp merging toolkit/xre/nsAppRunner.cpp 2017-04-20 12:26:09.374 FileMerge[16921:5548590] Unable to load platform at path /Applications/Xcode.app/Contents/Developer/Platforms/AppleTVOS.platform 2017-04-20 12:26:09.376 FileMerge[16921:5548590] Unable to load platform at path /Applications/Xcode.app/Contents/Developer/Platforms/AppleTVSimulator.platform 2017-04-20 12:26:09.377 FileMerge[16921:5548590] Unable to load platform at path /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform 2017-04-20 12:26:09.379 FileMerge[16921:5548590] Unable to load platform at path /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform 2017-04-20 12:26:09.384 FileMerge[16921:5548590] Unable to load platform at path /Applications/Xcode.app/Contents/Developer/Platforms/WatchOS.platform 2017-04-20 12:26:09.386 FileMerge[16921:5548590] Unable to load platform at path /Applications/Xcode.app/Contents/Developer/Platforms/WatchSimulator.platform
Flags: needinfo?(mrbkap)
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(mrbkap)
Comment 47•6 years ago
|
||
Pushed: https://hg.mozilla.org/releases/mozilla-beta/rev/f6a1a69f8b06 - Blake Kaplan - Bug 1349363 - Centralize pref-checking code for e10s-multi control. r=Felipe. a=gchang https://hg.mozilla.org/releases/mozilla-beta/rev/25da34a87bee - Blake Kaplan - Bug 1349363 - Use a centralized function to tell if e10s-multi is on. r=Felipe. a=gchang https://hg.mozilla.org/releases/mozilla-beta/rev/1c0e19d64704 - Blake Kaplan - Bug 1349363 - Run an experiment in beta for e10s-multi. r=Felipe. a=gchang https://hg.mozilla.org/releases/mozilla-beta/rev/bfa8f90200d1 - Blake Kaplan - Bug 1349363 - Track both .processCount and .web to get a complete view of the user's content processes. r=Felipe. a=gchang
Comment 48•6 years ago
|
||
Did you get a data review for adding webProcessCount to every ping's environment? (per https://wiki.mozilla.org/Firefox/Data_Collection )
Flags: needinfo?(mrbkap)
Assignee | ||
Comment 49•6 years ago
|
||
Comment on attachment 8859290 [details] Bug 1349363 - Track both .processCount and .web to get a complete view of the user's content processes. Chenxia, would you mind doing the data collection review for this?
Flags: needinfo?(mrbkap)
Attachment #8859290 -
Flags: review?(liuche)
Comment 50•6 years ago
|
||
mozreview-review |
Comment on attachment 8859290 [details] Bug 1349363 - Track both .processCount and .web to get a complete view of the user's content processes. https://reviewboard.mozilla.org/r/131302/#review135356 r+ for data review. dom.ipc.processCount doesn't add anything new, and these aren't new probes in any case (and are already collected elsewhere). Collecting a count of processes isn't identifiable or privacy invading.
Attachment #8859290 -
Flags: review?(liuche) → review+
You need to log in
before you can comment on or make changes to this bug.
Description
•