Closed Bug 1527905 Opened 6 years ago Closed 6 years ago

Switch from zone-per-tab to zone-per-toplevel-load

Categories

(Core :: DOM: Core & HTML, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

Conceptually, if zones are supposed to represent similar-lifetime objects, zone-per-toplevel-load makes a lot more sense than zone-per-tab.

I want to make this change because it makes bug 1523843 much simpler, and in particular prevents performance and memory regressions due to old objects being kept alive because we shared a compartment across navigations in a single tab.

On its own, this change is a memory regression, but bug 1527742 mitigates most of that, and the combination of the two is a memory win (though a slightly smaller win than just bug 1527742 on its own).

The main reason for this change is that we don't really want to share
compartments across same-origin navigations in the same tab (because that will
tend to keep the oldest global involved alive, due to CCWs and
XPCWrappedNatives allocated in that global). We could somehow flag
compartments as not sharable when we navigate, but it's simpler to just switch
zones, since we restrict our search of shareable compartments to a single zone.

A side benefit is that this way the lifetime of objects in a single zone is
more likely to be similar.

ccing some people who might care about GC stuff.

Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4548b735febd
Switch from zone-per-tab to zone-per-toplevel-load.  r=mccr8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Performance improvements noticed! \0/

== Change summary for alert #19391 (as of Fri, 15 Feb 2019 00:16:48 GMT) ==

Improvements:

3% raptor-tp6-slides-firefox fcp linux64 pgo 797.08 -> 773.38
3% raptor-tp6-slides-firefox linux64 pgo 931.02 -> 906.75

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=19391

Depends on: 1528763

Performance improvements noticed! \0/

Those are from bug 1527742.

Well, actually, I don't know for sure. There should be memory improvements from bug 1527742, but these are performance improvements...

This seems to have introduced very high frequency js failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=jsreftest%2Cwindows7-32&fromchange=591ffe90d0edbd8190427f09a14e39a92a327aee&tochange=182057bee133b28a51e32c391ac44dedeb1e0575&selectedJob=229243735

At the time of the landing these tests were not ran, so the failures were not spotted on time.
The changes were merged around to central and inbound.

:bz :jandem :jmaher

Flags: needinfo?(jmaher)
Flags: needinfo?(jdemooij)
Flags: needinfo?(bzbarsky)

(In reply to Bogdan Tara[:bogdan_tara] from comment #8)

This seems to have introduced very high frequency js failures:

I'm looking into this FWIW, should have some more data later today.

The failures are an OOM. The patch here shouldn't affect JS shell behavior, which means bug 1527742 is going to be directly at fault. Maybe there's some issue around reusing decommitted memory that that change exposed.

(In reply to Andrew McCreight [:mccr8] from comment #10)

The failures are an OOM. The patch here shouldn't affect JS shell behavior, which means bug 1527742 is going to be directly at fault. Maybe there's some issue around reusing decommitted memory that that change exposed.

It's the jsreftests running in the browser..

What I'm seeing on Try is that the runtime has 5000 zones when we OOM. I'll look into our poke-GC-on-navigation heuristics..

Depends on: 1529306

The patch in bug 1529306 seems to fix this on Try:

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=4b1f26f3c1cf4931c159a99c65b1dc603607b862

(Also includes the patch for bug 1529265 but I don't think that one is required for this.)

Flags: needinfo?(jdemooij)

thanks for working on this :jandem, sounds like we are close to a fix.

Flags: needinfo?(jmaher)

== Change summary for alert #19564 (as of Fri, 22 Feb 2019 06:32:34 GMT) ==

Improvements:

5% JS linux64-stylo-sequential opt stylo-sequential 113,904,145.01 -> 108,580,789.31

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=19564

Again, that's from bug 1527742, not this bug.

Flags: needinfo?(bzbarsky)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: