Share Shapes and BaseShapes across compartments

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

2 years ago
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).
(Assignee)

Comment 1

2 years ago
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.
(Assignee)

Comment 2

2 years ago
Created attachment 8782386 [details] [diff] [review]
Patch

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.
Attachment #8782386 - Flags: review?(n.nethercote)
Attachment #8782386 - Flags: review?(jcoppeard)
CC'ing GC people in case they want to take a look too.
(Assignee)

Comment 4

2 years ago
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?
Flags: needinfo?(nfitzgerald)
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.
Attachment #8782386 - Flags: review?(jcoppeard) → review+
(Assignee)

Comment 6

2 years ago
(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.
(Assignee)

Comment 7

2 years ago
Created attachment 8782473 [details] [diff] [review]
HeapSnapshot fix

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?
Attachment #8782473 - Flags: feedback?(nfitzgerald)
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
Attachment #8782473 - Flags: feedback?(nfitzgerald) → review+
Flags: needinfo?(nfitzgerald)
(Assignee)

Comment 9

2 years ago
Created attachment 8782845 [details] [diff] [review]
More HeapSnapshot changes

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?
Attachment #8782845 - Flags: feedback?(nfitzgerald)
(Assignee)

Comment 10

2 years ago
(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!
Attachment #8782845 - Flags: feedback?(nfitzgerald)
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 :(
Attachment #8782386 - Flags: review?(n.nethercote) → review+
(Assignee)

Comment 14

2 years ago
(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.
(Assignee)

Comment 15

2 years ago
Created attachment 8783623 [details] [diff] [review]
HeapSnapshot changes

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.
Attachment #8782473 - Attachment is obsolete: true
Attachment #8782845 - Attachment is obsolete: true
Attachment #8783623 - Flags: review?(nfitzgerald)
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!
Attachment #8783623 - Flags: review?(nfitzgerald) → review+

Comment 17

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

Comment 18

2 years ago
I was curious if this patch affected AWSY, but unfortunately it hasn't been updating lately. Is that a known issue?
Flags: needinfo?(erahm)

Comment 19

2 years ago
(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.
Flags: needinfo?(erahm)

Comment 20

2 years ago
(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!
(Assignee)

Comment 21

2 years ago
(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 :)
Created attachment 8784347 [details] [diff] [review]
bug1295967-update-comments

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.
Attachment #8784347 - Flags: review?(jdemooij)
(Assignee)

Comment 23

2 years ago
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'
Attachment #8784347 - Flags: review?(jdemooij) → review+

Updated

2 years ago
Whiteboard: [MemShrink] → [MemShrink:P2]

Comment 24

2 years ago
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

Comment 25

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0de811b52c94
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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

Comment 28

2 years ago
(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!
(Assignee)

Updated

2 years ago
Blocks: 1298878
(Assignee)

Updated

2 years ago
Blocks: 1299107
Depends on: 1300548
You need to log in before you can comment on or make changes to this bug.