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.

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

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?

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

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

I just filed bug 759585 to cover the memory savings.

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

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

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.

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.

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 on attachment 706121 [details] [diff] [review] zone renaming part 2 This should split things up more fairly.

Comment on attachment 706117 [details] [diff] [review] add js/GCAPI.h Review of attachment 706117 [details] [diff] [review]: ----------------------------------------------------------------- Nice.

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

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

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 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?

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