[e10s-multi] Beta 54 experiment

RESOLVED FIXED in Firefox 54

Status

()

Firefox
General
RESOLVED FIXED
2 months ago
25 days ago

People

(Reporter: mrbkap, Assigned: mrbkap)

Tracking

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed, firefox55 fixed)

Details

(Whiteboard: [e10s-multi:+])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(8 attachments)

(Assignee)

Description

2 months ago
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

2 months ago
Whiteboard: [e10s-multi:?]
: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

2 months ago
Whiteboard: [e10s-multi:?] → [e10s-multi:+]

Comment 2

2 months 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)
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

2 months 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

2 months 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)
Sounds great to me!
Flags: needinfo?(felipc)
We're dangerously close to Beta 1 of 54 - what is the status of this?
Flags: needinfo?(mrbkap)
(Assignee)

Comment 8

a month ago
I'll have a patch today.
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

a month 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

a month 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

a month 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

a month 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

a month 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

a month 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

a month 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

a month 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

a month 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

a month 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)

Updated

a month ago
Depends on: 1346288
(Assignee)

Comment 33

a month 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

a month ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4b082d38fae12c2e150fe99bfef4a5d4c1abc6e (with the fully updated patch :-/)

Comment 35

a month 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

a month 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
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(Assignee)

Comment 37

a month ago
Created attachment 8859723 [details] [diff] [review]
First patch rebased to beta
(Assignee)

Comment 38

a month ago
Created attachment 8859724 [details] [diff] [review]
Second patch rebased to beta

MozReview-Commit-ID: JQJtCanIv3a
(Assignee)

Comment 39

a month ago
Created attachment 8859725 [details] [diff] [review]
Third patch rebased to beta
(Assignee)

Comment 40

a month ago
Created attachment 8859726 [details] [diff] [review]
Fourth patch rebased to beta
(Assignee)

Comment 41

a month ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a272037cce32ef689407fe0b5da1a58937cb347
(Assignee)

Comment 42

a month 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

a month ago
Attachment #8859724 - Flags: approval-mozilla-beta?
(Assignee)

Updated

a month ago
Attachment #8859725 - Flags: approval-mozilla-beta?
(Assignee)

Updated

a month ago
Attachment #8859726 - Flags: approval-mozilla-beta?
(Assignee)

Updated

a month ago
Flags: needinfo?(sescalante)
(Assignee)

Updated

a month ago
Depends on: 1357909
(Assignee)

Comment 43

a month 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

a month 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?
(Assignee)

Updated

a month ago
Depends on: 1357959

Updated

a month ago
status-firefox54: --- → affected
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

a month ago
Attachment #8859724 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

a month ago
Attachment #8859725 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

a month ago
Attachment #8859726 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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

a month ago
Flags: needinfo?(mrbkap)
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
status-firefox54: affected → fixed

Comment 48

a month 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

a month 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

a month 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+
Depends on: 1360354

Updated

25 days ago
Depends on: 1362493
You need to log in before you can comment on or make changes to this bug.