Closed Bug 751618 Opened 8 years ago Closed 7 years ago

Add a "zone" concept to the JS engine

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: billm, Assigned: billm)

References

Details

(Whiteboard: [js:t])

Attachments

(11 files)

52.02 KB, patch
terrence
: review+
Details | Diff | Splinter Review
779 bytes, patch
jonco
: review+
Details | Diff | Splinter Review
28.49 KB, patch
jonco
: review+
Details | Diff | Splinter Review
29.67 KB, patch
jonco
: review+
Details | Diff | Splinter Review
18.24 KB, patch
jonco
: review+
Details | Diff | Splinter Review
11.70 KB, patch
terrence
: review+
Details | Diff | Splinter Review
18.90 KB, patch
jonco
: review+
Details | Diff | Splinter Review
30.44 KB, patch
terrence
: review+
Details | Diff | Splinter Review
6.21 KB, patch
jonco
: review+
Details | Diff | Splinter Review
98.88 KB, patch
terrence
: review+
Details | Diff | Splinter Review
4.46 KB, patch
terrence
: review+
Details | Diff | Splinter Review
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?
Whiteboard: [MemShrink]
(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.
Whiteboard: [MemShrink] → [js:t][MemShrink]
Whiteboard: [js:t][MemShrink] → [js:t][MemShrink:P1]
> (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.
Summary: Add a "compartment group" concept to the JS engine → Add a "zone" concept to the JS engine
(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.
Whiteboard: [js:t][MemShrink:P1] → [js:t]
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.
Attached patch add js/GCAPI.hSplinter Review
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.
Attachment #706117 - Flags: review?(terrence)
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.
Attachment #706119 - Flags: review?(jcoppeard)
Comment on attachment 706121 [details] [diff] [review]
zone renaming part 2

This should split things up more fairly.
Attachment #706121 - Flags: review?(terrence) → review?(jcoppeard)
Comment on attachment 706117 [details] [diff] [review]
add js/GCAPI.h

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

Nice.
Attachment #706117 - Flags: review?(terrence) → review+
Attachment #706123 - Flags: review?(terrence) → review+
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.
Attachment #706127 - Flags: review?(terrence) → review+
Attachment #706129 - Flags: review?(terrence) → review+
Attachment #706119 - Flags: review?(jcoppeard) → review+
Attachment #706120 - Flags: review?(jcoppeard) → review+
Attachment #706121 - Flags: review?(jcoppeard) → review+
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.
Attachment #706122 - Flags: review?(jcoppeard) → review+
Attachment #706126 - Flags: review?(jcoppeard) → review+
Attachment #706128 - Flags: review?(jcoppeard) → review+
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.
Attachment #706130 - Flags: review?(terrence) → review+
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.
You need to log in before you can comment on or make changes to this bug.