1510607 - Implement JS::IsCCTraceKind using the JS_FOR_EACH_TRACEKIND data

Status: RESOLVED DUPLICATE of bug 1522061

(Core :: JavaScript: GC, enhancement, P3)

Description

[Jon Coppeard (:jonco)]

We now have all information about which trace kinds are CC participants defined in a single file.  We should now make it so there is a single source for this data, the JS_FOR_EACH_TRACEKIND table.

Comment 1

[Jon Coppeard (:jonco)]

Oh, these two don't actually match up do they.  I'm hoping it's just that the CC doesn't see all our internal GC trace kinds.

Comment 2

[Andrew McCreight [:mccr8]]

Yeah, hopefully that's the case.

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

There are some types, whose TraceKind is NOT added in IsCCTraceKind,
for example, BaseShape::TraceKind is TraceKind::BaseShape
https://searchfox.org/mozilla-central/rev/f2028b4c38bff2a50ed6aa1763f6dc5ee62b0cc4/js/src/vm/Shape.h#566

But its IsCCTraceKind value in JS_FOR_EACH_TRACEKIND is true,
https://searchfox.org/mozilla-central/rev/e22c0d152060f4f8d4ca8904094f15f65a1b6f93/js/public/TraceKind.h#108

Should I just add BaseShare in IsCCTraceKind?

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Or maybe I don't fully understand what Jonco mentioned here.
I thought this bug is to use the value returned from IsCCTraceKind instead of hard-coding true/false in JS_FOR_EACH_TRACEKIND,
but from the summary it looks like it's the other way around? like IsCCTraceKind should return the hard-coded value in JS_FOR_EACH_TRACEKIND base on the type information.

Comment 5

[Steve Fink [:sfink] [:s:]]

(In reply to Yoshi Huang [:allstars.chh] from comment #4)
> Or maybe I don't fully understand what Jonco mentioned here.
> I thought this bug is to use the value returned from IsCCTraceKind instead
> of hard-coding true/false in JS_FOR_EACH_TRACEKIND,
> but from the summary it looks like it's the other way around? like
> IsCCTraceKind should return the hard-coded value in JS_FOR_EACH_TRACEKIND
> base on the type information.

Yes, the latter is what we want. The list of types should be removed from IsCCTraceKind and replaced with a lookup from the JS_FOR_EACH_TRACEKIND table, preferably in some way that compiles down to small and fast code.

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Steve Fink [:sfink] [:s:] from comment #5)
> (In reply to Yoshi Huang [:allstars.chh] from comment #4)
> > Or maybe I don't fully understand what Jonco mentioned here.
> > I thought this bug is to use the value returned from IsCCTraceKind instead
> > of hard-coding true/false in JS_FOR_EACH_TRACEKIND,
> > but from the summary it looks like it's the other way around? like
> > IsCCTraceKind should return the hard-coded value in JS_FOR_EACH_TRACEKIND
> > base on the type information.
> 
> Yes, the latter is what we want. The list of types should be removed from
> IsCCTraceKind and replaced with a lookup from the JS_FOR_EACH_TRACEKIND
> table, preferably in some way that compiles down to small and fast code.

Thanks, I understand now.

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch

Jonco, could you check if this is what you have in mind?

Thanks

Comment 8

[Jon Coppeard (:jonco)]

Comment on attachment 9029044 [details] [diff] [review]
Patch

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

Yes, this is what I had in mind.  Thanks!

Did you a try build for this to run the browser tests?  Probably a good idea since this could affect the cycle collector.

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Jon Coppeard (:jonco) from comment #8)
> Comment on attachment 9029044 [details] [diff] [review]
> Patch
> 
> Review of attachment 9029044 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yes, this is what I had in mind.  Thanks!
> 
> Did you a try build for this to run the browser tests?  Probably a good idea
> since this could affect the cycle collector.

Yeah, I submitted a try last week, the result is 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d004b0dba203ea770059bbf5f5abadef563cad7&selectedJob=214961438

Looks green.

I'll update the commit message and submit another r?

Thanks

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch

updated patch summary and commit message.

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch. v2

rebase,
with try result
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc4ffd6c64b8324d4cb5ebf273e5ba51519de145

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c48655b33ca
Implement JS::IsCCTraceKind using the JS_FOR_EACH_TRACEKIND data. r=jonco

Comment 13

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/2c48655b33ca

Comment 14

[Eliza Balazs [:ebalazs_]]

Backed out changeset 2c48655b33ca (Bug 1510607) for causing regression in Bug 1513471, requested by allstars.chh

Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&revision=50aa7a3a1240007b830fc85d3c09499842ebc93f

Comment 15

[Jon Coppeard (:jonco)]

I think what's happening here is that we are conflating several concepts:

  1. kinds of GC things that can be marked gray (that's what the JS_FOR_EACH_TRACEKIND table has)
  2. kinds of GC things that the cycle collector creates nodes for
  3. kinds of GC things that can be weakmap keys

This block:

  https://searchfox.org/mozilla-central/source/xpcom/base/CycleCollectedJSRuntime.cpp#400-425

handles Shapes and ObjectGroups after things for which IsCCTraceKind returns true, which implies that this should not return true for those kinds and therefore 1 and 2 are not the same.  I don't know why this is though.

I don't know whether 2 and 3 are the same.  I imagine there are types the CC reasons about that don't make sense as keys though.

We'd like to assert about weakmap keys in the engine so I guess these all need to be defined in js/public/TraceKind.h.

Andrew, does this sound right?

Comment 16

[Andrew McCreight [:mccr8]]

Ah, yeah, maybe these are three separate concepts.

The CC doesn't create nodes for shapes or objectgroups, because there are so many of them it would bloat up the graph. Instead, there's special handling. But yeah, I guess the GC still needs to mark those gray. We should probably have different names for these three similar but not identical concepts.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

(In reply to Eliza Balazs [:ebalazs_] from comment #14)
> Backed out changeset 2c48655b33ca (Bug 1510607) for causing regression in
> Bug 1513471, requested by allstars.chh

Ugh, this backout landed after the Gecko version bump. This needs backing out from Beta still.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Backed out from Beta for 65 as well.
https://hg.mozilla.org/releases/mozilla-beta/rev/1c0492ce0d72

Comment 19

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

done this in bug 1522061 part 2 patch.