1731792 - Major warmload regression in fission due to setting dom.ipc.processCount.webIsolated=4

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect, P1)

Description

[Randell Jesup [:jesup] (needinfo me)]

Landing of bug 1727158 caused a large warmload regression on many sites of 4-6% average. Worst case appears to be wikipedia, which regressed around 20%+ on linux.

Nika's assumption is that we're cycling through different content processes due to this + bfcache. I presume this means we get 4 in the same process, then it grabs another process, and this keeps happening (the warmload sequence is a cold load, then 25 iterations of about:blank then the page again). If this happens every 4 hits of the site, we'll end up using ~7 processes over the test instead of 1.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Does this analysis sound correct?

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

This is somewhat realworld I think, since a sequence of navigations within a site (without using back) like A -> A/B -> A -> A/C -> A -> A/D .... (very common on news sites) will cause the same sort maxing out of the pages in each process, causing a new process to be allocated every 2 pages you read in that sequence.

One other possible performance impact is the GC's associated with pages moving into the bfcache colliding with loading subsequent pages in the automated sequence.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1727158

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1731792 - Part 1: Avoid cycling between processes when navigating within a tab, r=smaug

This patch replaces the previous process selection infrastructure with a
new setup implemented entirely in C++, which should more accurately
track the set of processes in use, and should encourage re-use of the
existing content process when navigating by not counting the current
tab.

This approach intentionally allows for process switching to another
process during navigation if there is uneven load between processes to
encourage balanced process use.

I think this may also fix some of the session restore issues with many
tabs using the same process, rather than being spread over 4, as we now
track a tab earlier in its lifecycle before the BrowserParent instance
is created.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1731792 - Part 2: Respect AppType in ImageCacheKey, r=emilio

This attribute is not used in Gecko, but exists for use by other
applications. Specifically, the APP_TYPE_EDITOR type is given permission
to load privileged images as tested by browser_docshell_type_editor.js.
Before these changes, that test passed because the docshell was loaded
in a different process, so the cache was empty when each load occurred,
but after my changes the process ends up being re-used, so the image
cache bypasses this check.

This changes the image cache key to also include the app type
information so that it will be compared before re-using the entry.

Depends on D126405

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/51bdb1d4baab
Part 1: Avoid cycling between processes when navigating within a tab, r=smaug
https://hg.mozilla.org/integration/autoland/rev/44c28a29bbd8
Part 2: Respect AppType in ImageCacheKey, r=emilio

Comment 7

[Atila Butkovits]

Backed out for causing mochitest failures.

Backout link: https://hg.mozilla.org/integration/autoland/rev/74ab612973c49b7689b7f2241f70ec71452df276

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=b93SqhhfTKq68I-alhrvcg.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=44c28a29bbd8c09dfc9d06bc6c253392b0219f6e

Failure log: https://treeherder.mozilla.org/logviewer?job_id=352821650&repo=autoland&lineNumber=24358

Comment 8

[Chris Peterson [:cpeterson]]

Severity S2 because this is a bad perf regression.

Comment 9

[Nika Layzell [:nika] (ni? for response)]

(In reply to Atila Butkovits from comment #7)

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=b93SqhhfTKq68I-alhrvcg.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=44c28a29bbd8c09dfc9d06bc6c253392b0219f6e

I've failed to reproduce this failure, even with retriggers on the same job (https://treeherder.mozilla.org/jobs?repo=autoland&revision=44c28a29bbd8c09dfc9d06bc6c253392b0219f6e&searchStr=bc6) so I'm tempted to try re-landing it.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/db57b8568ae9
Part 1: Avoid cycling between processes when navigating within a tab, r=smaug
https://hg.mozilla.org/integration/autoland/rev/567d4eb7bc83
Part 2: Respect AppType in ImageCacheKey, r=emilio

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a23258edee0c
Backed out 2 changesets for causing devtools failures on browser_console_error_source_click.js

Comment 12

[Noemi Erli[:noemi_erli]]

R/B range: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&collapsedPushes=842382&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&fromchange=19e5efa238fb3bf0fcec254c9ea2651320f98dbc&searchStr=linux%2C18.04%2Cx64%2Cwebrender%2Cdebug%2Cmochitests%2Cwith%2Cfission%2Cenabled%2Ctest-linux1804-64-qr%2Fdebug-mochitest-devtools-chrome-fis-e10s%2Cdt4&tochange=6b6eed16fcafa9dee9c7eb3c8b63d1447e29386f&selectedTaskRun=CTDYciIqSA6tqmQXoBJViw.0

Backed out 2 changesets (Bug 1731792) for causing devtools failures on browser_console_error_source_click.js

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&collapsedPushes=842382&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&searchStr=linux%2C18.04%2Cx64%2Cwebrender%2Cdebug%2Cmochitests%2Cwith%2Cfission%2Cenabled%2Ctest-linux1804-64-qr%2Fdebug-mochitest-devtools-chrome-fis-e10s%2Cdt4&revision=567d4eb7bc834f7b50a7e1f0cef1248b3af5d2ba&selectedTaskRun=QNYh2pV2TjybHDX0DsTatQ.0

Log: https://treeherder.mozilla.org/logviewer?job_id=352887893&repo=autoland&lineNumber=16890

Backout: https://hg.mozilla.org/integration/autoland/rev/a23258edee0cb2cc55b13dd96ab64f81e48f0fd3

Comment 13

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1731792 - Don't switch processes for BFCached same-origin navigations, r=smaug

This is a much simplified patch compared to the earlier changes, and
aims to be less risky and avoid whatever issues are being caused by the
process selection changes in the earlier patches. Unfortunately, I think
this patch may reduce the ability for a noopener window to be given a
distinct process, so I would prefer a more complete fix in the future.

This new patch also no longer has a fix for bug 1728331, so that will
need to be fixed separately.

Comment 14

[Nika Layzell [:nika] (ni? for response)]

I've attached a new version of the patch which is much more simplified and hopefully won't have the same issues.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8f2f2cfb9620
Don't switch processes for BFCached same-origin navigations, r=smaug

Comment 16

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/8f2f2cfb9620

Comment 17

[Cristina Cozmuta (:CrissCozmuta)]

Hi Nika, can you please take a look over these dt failures?

Comment 18

[Nika Layzell [:nika] (ni? for response)]

(In reply to Cristina Cozmuta (:CrissCozmuta) from comment #17)

Hi Nika, can you please take a look over these dt failures?

Clearing the ni? as the devtools team has offered to look into these intermittent failures, and been ni?-ed on the other bug.

Comment 19

[Phabricator Automation]

Comment on attachment 9242559 [details]
Bug 1731792 - Part 1: Avoid cycling between processes when navigating within a tab, r=smaug

Revision D126405 was moved to bug 1728331. Setting attachment 9242559 [details] to obsolete.

Comment 20

[Phabricator Automation]

Comment on attachment 9242775 [details]
Bug 1731792 - Part 2: Respect AppType in ImageCacheKey, r=emilio

Revision D126557 was moved to bug 1728331. Setting attachment 9242775 [details] to obsolete.