Closed Bug 1510607 Opened 11 months ago Closed 6 months ago

Implement JS::IsCCTraceKind using the JS_FOR_EACH_TRACEKIND data

Categories

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

61 Branch
enhancement

Tracking

()

RESOLVED DUPLICATE of bug 1522061
Tracking Status
firefox65 --- wontfix
firefox66 --- affected

People

(Reporter: jonco, Assigned: allstars.chh)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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.
Assignee: nobody → allstars.chh
Status: NEW → ASSIGNED
Yeah, hopefully that's the case.
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?
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.
(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.
(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.
Attached patch Patch (obsolete) — Splinter Review
Jonco, could you check if this is what you have in mind?

Thanks
Attachment #9029044 - Flags: feedback?(jcoppeard)
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.
Attachment #9029044 - Flags: feedback?(jcoppeard) → feedback+
(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
Attached patch Patch (obsolete) — Splinter Review
updated patch summary and commit message.
Attachment #9029044 - Attachment is obsolete: true
Attachment #9029447 - Flags: review?(jcoppeard)
Attachment #9029447 - Flags: review?(jcoppeard) → review+
Pushed by allstars.chh@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c48655b33ca
Implement JS::IsCCTraceKind using the JS_FOR_EACH_TRACEKIND data. r=jonco
https://hg.mozilla.org/mozilla-central/rev/2c48655b33ca
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla65 → ---
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?
Flags: needinfo?(continuation)
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.
Flags: needinfo?(continuation)
(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.
Flags: needinfo?(ryanvm)
Backed out from Beta for 65 as well.
https://hg.mozilla.org/releases/mozilla-beta/rev/1c0492ce0d72
Flags: needinfo?(ryanvm)

done this in bug 1522061 part 2 patch.

Status: REOPENED → RESOLVED
Closed: 11 months ago6 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1522061
You need to log in before you can comment on or make changes to this bug.