obj->compartment() now gets the compartment from the ObjectGroup instead of the Shape/BaseShape. BaseShape::compartment_ is currently only used for some assertions and memory reporting stuff, and we can remove it. We can then also move the property tree and (Base)Shape maps from the compartment to the zone. This lets us effectively share (Base)Shapes across all compartments within that zone. What's a bit unfortunate is that accessor shapes store getter and setter JSObjects. That still works fine, but because a JSObject belongs to a single compartment, it means these shapes and all their descendants can't be shared with other compartments. (The branch of the Shape tree becomes compartment-local.) Still, there are many Shapes that *can* be shared, for instance the initial/empty shapes. There's also an issue with BaseShape tracing: it currently also traces compartment_->global, probably to keep the compartment alive. I think/hope we can move that code to ObjectGroup tracing. With a prototype patch that passes shell tests, I get the following results (assuming I didn't mess up the memory reporting bits): * Simple HTML page (example.com): Content process before: │ │ └──2,024,664 B (14.58%) -- shapes │ │ ├──1,708,024 B (12.30%) -- gc-heap │ │ │ ├──1,358,512 B (09.79%) ── tree │ │ │ ├────238,992 B (01.72%) ── dict │ │ │ └────110,520 B (00.80%) ── base │ │ └────316,640 B (02.28%) -- malloc-heap │ │ ├──139,552 B (01.01%) ── tree-kids │ │ ├───91,936 B (00.66%) ── dict-tables │ │ └───85,152 B (00.61%) ── tree-tables Content process after: │ ├──1,645,752 B (12.03%) -- shapes │ │ ├──1,350,968 B (09.88%) -- gc-heap │ │ │ ├──1,088,552 B (07.96%) ── tree │ │ │ ├────238,992 B (01.75%) ── dict │ │ │ └─────23,424 B (00.17%) ── base │ │ └────294,784 B (02.15%) -- malloc-heap │ │ ├──116,448 B (00.85%) ── tree-kids │ │ ├───92,448 B (00.68%) ── dict-tables │ │ └───85,888 B (00.63%) ── tree-tables Parent process before: │ │ └───9,684,800 B (16.22%) -- shapes │ │ ├──7,934,624 B (13.29%) -- gc-heap │ │ │ ├──6,117,584 B (10.25%) ── tree │ │ │ ├──1,307,720 B (02.19%) ── dict │ │ │ └────509,320 B (00.85%) ── base │ │ └──1,750,176 B (02.93%) -- malloc-heap │ │ ├────657,280 B (01.10%) ── tree-kids │ │ ├────601,472 B (01.01%) ── tree-tables │ │ └────491,424 B (00.82%) ── dict-tables Parent process after: │ ├───7,989,768 B (14.09%) -- shapes │ │ ├──6,374,056 B (11.24%) -- gc-heap │ │ │ ├──4,929,248 B (08.69%) ── tree │ │ │ ├──1,300,968 B (02.29%) ── dict │ │ │ └────143,840 B (00.25%) ── base │ │ └──1,615,712 B (02.85%) -- malloc-heap │ │ ├────611,616 B (01.08%) ── tree-tables │ │ ├────516,448 B (00.91%) ── tree-kids │ │ └────487,648 B (00.86%) ── dict-tables * GMail + a random Facebook profile, content process: Shapes total: 15,347,088 -> 14,537,256 - BaseShapes: 557,680 -> 302,112 * TechCrunch, content process: Shapes total: 9,181,640 -> 8,435,904 - BaseShapes: 434,880 -> 182,080 Note that especially BaseShapes are shared aggressively: for a content process that has mostly chrome JS (many JSM compartments in the system zone), we see 110,520 -> 23,424 bytes (4.7x smaller).

The per-compartment shape tables are now per-zone and I added a new per-zone reporter for these shape tables. There are some wins there as well. In the example.com case for the content process: Before: │ ├────490,752 B (03.41%) ── compartment-tables After: │ ├────166,784 B (01.22%) ── compartment-tables ... │ ├────121,856 B (00.89%) ── shape-tables compartment-tables + shape-tables is much smaller than the old compartment-tables.

r? njn for the memory reporter changes. I added a ShapeInfo class that's stored in ZoneStats. I removed some unused code to report Shapes by class name: adding that requires a lot more changes and we can't use that code atm anyway. r? jonco for the rest. As discussed, I moved the "trace the global" code from BaseShape to ObjectGroup.

CC'ing GC people in case they want to take a look too.

I'm getting a failure on Try in this test: devtools/shared/heapsnapshot/tests/unit/test_HeapSnapshot_takeCensus_08.js The problem is the last test there, the one for the shapes. I think the census only looks at the debuggee compartment, and with this patch shapes are no longer allocated per compartment but per zone, so we no longer count the shapes and the test fails. The similar jit-test (tests/debug/Memory-takeCensus-08.js) still passes, I think because that one looks at the whole runtime? Nick, any suggestions?

Comment on attachment 8782386 [details] [diff] [review] Patch Review of attachment 8782386 [details] [diff] [review]: ----------------------------------------------------------------- I thought this was going to be complicated but this is a nice simple change. r=me. ::: js/src/gc/Tracer.cpp @@ -183,5 @@ > - // doing it for every Shape in our lineage, since it's always the same > - // global. > - JSObject* global = shape->compartment()->unsafeUnbarrieredMaybeGlobal(); > - MOZ_ASSERT(global); > - DoCallback(trc, &global, "global"); I was worried by this at first, but I think this is handled by moving the tracing of the global to the object group. Calling TraceCycleCollectorChildren on an ObjectGroup* will trace the global by calling traceChildren.

(In reply to Jan de Mooij [:jandem] from comment #4) > The problem is the last test there, the one for the shapes. I think the > census only looks at the debuggee compartment, and with this patch shapes > are no longer allocated per compartment but per zone, so we no longer count > the shapes and the test fails. It's probably the code here: http://searchfox.org/mozilla-central/rev/ae78ab94fadabc89fc6258d03c4a1a70f763f43a/devtools/shared/heapsnapshot/HeapSnapshot.cpp#1341-1354 I think this will include atoms and the object's last shape, but it won't include the shape's ancestors. > The similar jit-test (tests/debug/Memory-takeCensus-08.js) still passes It makes sense: the code in devtools/ won't affect shell tests.

This fixes HeapSnapshotHandler to include edges from nodes that don't belong to any compartment, like Shapes now. It fixes the failing test. Does this make sense to you?

Comment on attachment 8782473 [details] [diff] [review] HeapSnapshot fix Review of attachment 8782473 [details] [diff] [review]: ----------------------------------------------------------------- Makes perfect sense -- thanks! I was going to also suggest making sure that the global is in its own zone, but it looks like we are already covered on that front: http://searchfox.org/mozilla-central/rev/ae78ab94fadabc89fc6258d03c4a1a70f763f43a/devtools/shared/heapsnapshot/tests/unit/head_heapsnapshot.js#73

Unfortunately I still get HeapSnapshot GTest failures with that patch. When we have the following nodes: A (compartment 1) -> B (compartment 2) We do include node B when we're interested in compartment 1, but we don't include its outgoing edges. This still works exactly the same as before. Now, when we have these nodes: A (compartment 1) -> B (no compartment) -> C (no compartment) -> D (compartment 2) We used to include just A and B; my previous patch changed that to include all of the edges/nodes, so we include A, B, C, D. The patch I'm attaching tries to fix it so that we include A, B, C, but not D. This fix doesn't work though: we fail the following check, because we include the C -> D edge but not D: http://searchfox.org/mozilla-central/rev/ae78ab94fadabc89fc6258d03c4a1a70f763f43a/devtools/shared/heapsnapshot/HeapSnapshot.cpp#451-457 So what do we want to happen in the second case, should we include D, similar to case 1? If we don't want to include node D, how can I do that?

(In reply to Jan de Mooij [:jandem] from comment #9) > We used to include just A and B; my previous patch changed that to include > all of the edges/nodes, so we include A, B, C, D. We can also revert this change, and only visit the first Shape. Then we just have to remove the shape test from test_HeapSnapshot_takeCensus_08.js...

(In reply to Jan de Mooij [:jandem] from comment #9) > The patch I'm attaching tries to fix it so that we include A, B, C, but not > D. Yes, this would be ideal. > This fix doesn't work though: we fail the following check, because we > include the C -> D edge but not D: > > http://searchfox.org/mozilla-central/rev/ > ae78ab94fadabc89fc6258d03c4a1a70f763f43a/devtools/shared/heapsnapshot/ > HeapSnapshot.cpp#451-457 > > So what do we want to happen in the second case, should we include D, > similar to case 1? If we don't want to include node D, how can I do that? Ok, this is slightly trickier than I'd hoped for, but I think this should work: * Make StreamWriter have a reference to the target compartments * When EdgePolicy::INCLUDE_EDGES in StreamWriter::writeNode, skip edges whose referent both has a compartment and the compartment is not in our target set (if we don't have a target set, it means we want everything). Right after this: http://searchfox.org/mozilla-central/rev/f433f0dd7158d7bfc4c4607161fc6baa88b5a9f4/devtools/shared/heapsnapshot/HeapSnapshot.cpp#1242

Comment on attachment 8782845 [details] [diff] [review] More HeapSnapshot changes Review of attachment 8782845 [details] [diff] [review]: ----------------------------------------------------------------- Clearing feedback to wait for the other approach. ::: devtools/shared/heapsnapshot/HeapSnapshot.cpp @@ +1335,3 @@ > // We aren't targeting a particular set of compartments, so serialize all the > // things! > + nodeCount++; Great catch!

Comment on attachment 8782386 [details] [diff] [review] Patch Review of attachment 8782386 [details] [diff] [review]: ----------------------------------------------------------------- Apologies for the slow review. Have all shapes and related structures been moved from compartments to zones? If so, the memory reporter changes look ok. I'm still sad that we can't have class identification for shapes :(

(In reply to Nicholas Nethercote [:njn] from comment #13) > Apologies for the slow review. Not that slow; I also still have an UbiNode issue to fix before I can land this. > Have all shapes and related structures been > moved from compartments to zones? Yes.

Thanks for the suggestion. I factored the code out into a new ShouldIncludeEdge function. Then we can call that from both places, without duplicating any of the logic in it. This is green on Try.

Comment on attachment 8783623 [details] [diff] [review] HeapSnapshot changes Review of attachment 8783623 [details] [diff] [review]: ----------------------------------------------------------------- Beautiful! Thanks for following through with this! ::: devtools/shared/heapsnapshot/HeapSnapshot.cpp @@ +1065,5 @@ > + // edges. This case is relevant for Shapes: they don't belong to a specific > + // compartment and contain edges to parent/kids Shapes we want to include. Note > + // that these Shapes may contain pointers into our target compartment (the > + // Shape's getter/setter JSObjects). However, we do not serialize nodes in other > + // compartments that are reachable from these non-compartment nodes. Thanks for documenting this stuff!

Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0de811b52c94 Share Shapes and BaseShapes across compartments. r=jonco,fitzgen,njn

I was curious if this patch affected AWSY, but unfortunately it hasn't been updating lately. Is that a known issue?

(In reply to Jan de Mooij [:jandem] from comment #18) > I was curious if this patch affected AWSY, but unfortunately it hasn't been > updating lately. Is that a known issue? The testing server is having difficulties, I'm actively looking into it and will let you know when it's back up.

(In reply to Eric Rahm [:erahm] from comment #19) > (In reply to Jan de Mooij [:jandem] from comment #18) > > I was curious if this patch affected AWSY, but unfortunately it hasn't been > > updating lately. Is that a known issue? > > The testing server is having difficulties, I'm actively looking into it and > will let you know when it's back up. It's back up, currently backfilling 278 builds so it'll take a few days to get data. I'll definitely keep an eye out for improvements!

(In reply to Eric Rahm [:erahm] from comment #20) > It's back up, currently backfilling 278 builds so it'll take a few days to > get data. I'll definitely keep an eye out for improvements! Thanks! It probably won't be visible, but just curious :)

I came across a couple of places where the comments talk about shapes having a compartment. Here's a patch. Also I removed an unused BaseShape constructor.

Comment on attachment 8784347 [details] [diff] [review] bug1295967-update-comments Review of attachment 8784347 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! I should have caught these. ::: js/src/gc/Zone.h @@ +117,5 @@ > // different zone and do nothing if it's in the same zone. Thus, transferring > // strings within a zone is very efficient. > // > +// - Shapes and base shapes belong to a zone and are shared between compartments > +// compartments in that zone where possible. Accessor shapes store getter and Nit: rm one of the 'compartments'

Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5bc7fa6fdd66 Update some comments now shapes are shared within a zone r=jandem

we see a talos improvement from this patch! == Change summary for alert #2813 (as of August 27 2016 03:00 UTC) == Improvements: 24% tp5o responsiveness windowsxp opt e10s 9.14 -> 6.98 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=2813

(In reply to Jan de Mooij [:jandem] from comment #21) > (In reply to Eric Rahm [:erahm] from comment #20) > > It's back up, currently backfilling 278 builds so it'll take a few days to > > get data. I'll definitely keep an eye out for improvements! > > Thanks! It probably won't be visible, but just curious :) Looks like possibly an average 5MiB improvement in JS memory usage on AWSY after this landed. Nice job!