Closed Bug 1731792 Opened 1 month ago Closed 24 days ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
94 Branch
Fission Milestone MVP
Tracking Status
firefox-esr78 --- unaffected
firefox-esr91 --- unaffected
firefox92 --- unaffected
firefox93 --- unaffected
firefox94 + fixed

People

(Reporter: jesup, Assigned: nika)

References

(Blocks 1 open bug, Regressed 1 open bug, Regression)

Details

(Keywords: regression, Whiteboard: [fission-perf][qf] fission-hard-blocker)

Attachments

(3 files)

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.

Does this analysis sound correct?

Flags: needinfo?(nika)

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.

See Also: → 1731604

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.

Assignee: nobody → nika
Status: NEW → ASSIGNED
Flags: needinfo?(nika)
Blocks: 1731604
See Also: 1731604
Attachment #9242559 - Attachment description: Bug 1731792 - Avoid cycling between processes when navigating within a tab, r=smaug → Bug 1731792 - Part 1: Avoid cycling between processes when navigating within a tab, r=smaug

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

Pushed by nlayzell@mozilla.com:
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

Severity S2 because this is a bad perf regression.

Severity: -- → S2
Fission Milestone: ? → MVP
Priority: -- → P1
Whiteboard: [fission-perf][qf] → [fission-perf][qf] fission-hard-blocker
Pushed by nlayzell@mozilla.com:
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
Backout by ccozmuta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a23258edee0c
Backed out 2 changesets for causing devtools failures on browser_console_error_source_click.js

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.

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

Flags: needinfo?(nika)
Pushed by nlayzell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8f2f2cfb9620
Don't switch processes for BFCached same-origin navigations, r=smaug
Status: ASSIGNED → RESOLVED
Closed: 24 days ago
Resolution: --- → FIXED
Target Milestone: --- → 94 Branch

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

Flags: needinfo?(nika)
Regressions: 1733272

(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.

Flags: needinfo?(nika)
Regressions: 1732818
Regressions: 1733957
No longer regressions: 1733957
Regressions: 1734046
Regressions: 1734051
See Also: → 1731604
You need to log in before you can comment on or make changes to this bug.