751618 - Add a "zone" concept to the JS engine

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Since compartment-per-global landed, we now have a lot more compartments. Previously, we stuck a lot of data structures on the compartment. Luke lifted a lot of those to the runtime. This is fine for data structures that are cleared on ever GC. But for things that need to be swept, it causes problems.

For example, the script filename table used to be on the compartment. We could sweep it during compartment GCs. Now it's on the runtime and we can only sweep it during full GCs. The same basic situation is true for atoms, although those have always been global.

So the idea is to have a concept of a compartment group. We would collect all the compartments in the group at the same time. We would only have one script filename table per group, which would save memory and allow sharing. Probably compartments from the same tab would be grouped together, although the grouping could in theory be arbitrary.

Comment 1

[Danial Horton]

by allowing sharing, do you mean bringing Bug 679942 closer to fruition or something entirely different?

Comment 2

[Justin Lebar (not reading bugmail)]

njn indicated in an e-mail that the intent here is (now) to have multiple compartments share arenas, bringing our arena-wasted-space back to pre-CPG levels.  Bill, am I understanding that right?

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Danial Horton from comment #1)
> by allowing sharing, do you mean bringing Bug 679942 closer to fruition or
> something entirely different?

I don't think this bug would help much with that. The layout of JSScript wouldn't change at all.

(In reply to Justin Lebar [:jlebar] from comment #2)
> njn indicated in an e-mail that the intent here is (now) to have multiple
> compartments share arenas, bringing our arena-wasted-space back to pre-CPG
> levels.  Bill, am I understanding that right?

This bug would be the first step on that path, yes.

Comment 4

[Nicholas Nethercote [inactive]]

> (In reply to Justin Lebar [:jlebar] from comment #2)
> > njn indicated in an e-mail that the intent here is (now) to have multiple
> > compartments share arenas, bringing our arena-wasted-space back to pre-CPG
> > levels.  Bill, am I understanding that right?
> 
> This bug would be the first step on that path, yes.

Do we have a bug for the full path?  Clawing back some/all of the CPG memory hit would be really nice...

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I just filed bug 759585 to cover the memory savings.

Comment 6

[Justin Lebar (not reading bugmail)]

(In reply to Bill McCloskey (:billm) from comment #5)
> I just filed bug 759585 to cover the memory savings.

And that's what we care about from the MemShrink PoV, so removing the tag from this bug.

Comment 7

[Eddy Bruel [:ejpbruel]]

How would we determine which compartments are grouped into the same zone?

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'm going to start posting patches here. These patches just typedef JS::Zone to JSCompartment. Then they rename lots of things to use the zone instead of the compartment. The eventual goal is for the Zone to be a separate type, with multiple compartments belonging to the same Zone. The GC would collect in units of zones, so mostly it wouldn't care about compartments anymore.

For now I'm planning on keeping most of the compartment-specific data (caches, JIT stuff, etc.) in the compartment. Only GC-specific data like gcBytes would move into the zone. We can revisit that once everything has stabilized a bit.

I sort of split up the renaming patches into little pieces that sort of go together, mostly based on the order in which I converted things.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: add js/GCAPI.h

This initial patch moves a lot of the GC API functions from jsfriendapi.h to js/GCAPI.h. I've always wanted to do something like this, and this seemed like a good opportunity.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: ResetIncrementalGC fix

This fixes a small problem I noticed in ResetIncrementalGC. It does some assertions on all the compartments being collected. However, the code runs after we've already reset everything, so I don't think it ever actually iterates over any compartments. This patch fixes the problem.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: zone renaming part 1

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: zone renaming part 2

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: zone renaming part 3

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: zone renaming part 4

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: zone renaming part 5

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: zone renaming part 6

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: zone renaming part 7

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: zone renaming part 8

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: zone renaming part 9

Comment 20

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 706121 [details] [diff] [review]
zone renaming part 2

This should split things up more fairly.

Comment 21

[Terrence Cole [:terrence]]

Comment on attachment 706117 [details] [diff] [review]
add js/GCAPI.h

Review of attachment 706117 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

Comment 22

[Terrence Cole [:terrence]]

Comment on attachment 706127 [details] [diff] [review]
zone renaming part 6

Review of attachment 706127 [details] [diff] [review]:
-----------------------------------------------------------------

Considering that I just changed all the post barriers to take a JSRuntime*, one of us is going to have a fun rebase.

Comment 23

[Jon Coppeard (:jonco)]

Comment on attachment 706122 [details] [diff] [review]
zone renaming part 3

Review of attachment 706122 [details] [diff] [review]:
-----------------------------------------------------------------

jsgc.cpp 2612, 2617, 2952, 2960: Maybe s/compartment/zone/ in comments that refer to the updated code.

Comment 24

[Terrence Cole [:terrence]]

Comment on attachment 706130 [details] [diff] [review]
zone renaming part 9

Review of attachment 706130 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry I didn't review this last night. I figured it would be another monster, like the prior patch.

Comment 25

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=6dac413f2703

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e0070650c153
https://hg.mozilla.org/mozilla-central/rev/03d905c608bb
https://hg.mozilla.org/mozilla-central/rev/d2ca700aa7c0
https://hg.mozilla.org/mozilla-central/rev/98e0c10da12f
https://hg.mozilla.org/mozilla-central/rev/09a8f6b60ee5
https://hg.mozilla.org/mozilla-central/rev/05880c8938cb
https://hg.mozilla.org/mozilla-central/rev/f1e00bbb8f08
https://hg.mozilla.org/mozilla-central/rev/888c54278022
https://hg.mozilla.org/mozilla-central/rev/1032a553ddfd
https://hg.mozilla.org/mozilla-central/rev/cb81443b42ec
https://hg.mozilla.org/mozilla-central/rev/576566ced8f6
https://hg.mozilla.org/mozilla-central/rev/6dac413f2703

Comment 27

[Nicholas Nethercote [inactive]]

So this was just a bunch of renaming without any real functional changes, is that right?  (That's fine if so, I just want to understand what happened.)

Comment 28

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 706117 [details] [diff] [review]
add js/GCAPI.h

Review of attachment 706117 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/public/GCAPI.h
@@ +65,5 @@
> +};
> +
> +} /* namespace gcreason */
> +
> +extern JS_FRIEND_API(void)

Not JS_PUBLIC_API?

Comment 29

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Nicholas Nethercote [:njn] from comment #27)
> So this was just a bunch of renaming without any real functional changes, is
> that right?  (That's fine if so, I just want to understand what happened.)

Correct.

(In reply to :Ms2ger from comment #28)
> Not JS_PUBLIC_API?

There's been some disagreement about this in the past so I decided to leave it JS_FRIEND_API. We can always change it later. If we do, it would make sense to remove JS_GC and the like.