Closed Bug 1349363 Opened 6 years ago Closed 6 years ago

[e10s-multi] Beta 54 experiment

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: mrbkap, Assigned: mrbkap)

References

Details

(Whiteboard: [e10s-multi:+])

Attachments

(8 files)

Bug 1349363 - Centralize pref-checking code for e10s-multi control.
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1349363 - Use a centralized function to tell if e10s-multi is on.
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1349363 - Run an experiment in beta for e10s-multi.
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1349363 - Track both .processCount and .web to get a complete view of the user's content processes.
59 bytes, text/x-review-board-request
Felipe
: review+
liuche
: review+
Details
First patch rebased to beta
6.92 KB, patch
gchang
: approval-mozilla-beta+
Details | Diff | Splinter Review
Second patch rebased to beta
4.74 KB, patch
gchang
: approval-mozilla-beta+
Details | Diff | Splinter Review
Third patch rebased to beta
8.82 KB, patch
gchang
: approval-mozilla-beta+
Details | Diff | Splinter Review
Fourth patch rebased to beta
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).
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)
Whiteboard: [e10s-multi:?] → [e10s-multi:+]
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: ^
I was hoping to have a patch for this today but hit some snags. I should have something ready by tomorrow.
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)
I'll have a patch today.
Flags: needinfo?(mrbkap)
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.
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 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 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 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 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 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 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 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 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
Depends on: 1346288
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.
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
MozReview-Commit-ID: JQJtCanIv3a
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?
Attachment #8859724 - Flags: approval-mozilla-beta?
Attachment #8859725 - Flags: approval-mozilla-beta?
Attachment #8859726 - Flags: approval-mozilla-beta?
Flags: needinfo?(sescalante)
Depends on: 1357909
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?
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?
Depends on: 1357959
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+
Attachment #8859724 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8859725 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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)
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
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)
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 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: 1362493
Depends on: 1438872
You need to log in before you can comment on or make changes to this bug.