1460052 - AddToCCKind list in TraceKind.h does not match CycleCollector AddToCCKind function

Status: RESOLVED DUPLICATE of bug 1463343

(Core :: JavaScript: GC, enhancement)

Description

[Jan de Mooij [:jandem]]

Why is there a difference between:

https://searchfox.org/mozilla-central/rev/5ff2d7683078c96e4b11b8a13674daded935aa44/xpcom/base/CycleCollectedJSRuntime.h#414

And:

https://searchfox.org/mozilla-central/source/js/public/TraceKind.h#88

Does it matter? If yes which one should we fix?

Comment 1

[Andrew McCreight [:mccr8]]

I think the idea is that the one in CCJSRuntime returns true for less things because those other types that return true in TraceKind are not exposed to the browser. Maybe that is flawed logic. It does feel like we could define the CCJSRuntime one in terms of the other one.

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Andrew McCreight [:mccr8] from comment #1)
> I think the idea is that the one in CCJSRuntime returns true for less things
> because those other types that return true in TraceKind are not exposed to
> the browser. Maybe that is flawed logic.

Hm so for instance why does the one in CCJSRuntime return true for RegExpShared (an internal JS engine GC type that's not very interesting to the CC I imagine) but not for JitCode (a similar internal GC type).

I'm just wondering if there could be subtle perf issues here because both sides have different ideas about what things the CC cares about.

Comment 3

[Andrew McCreight [:mccr8]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> Hm so for instance why does the one in CCJSRuntime return true for
> RegExpShared (an internal JS engine GC type that's not very interesting to
> the CC I imagine) but not for JitCode (a similar internal GC type).

Because I saw the addition of RegExpShared and suggested that it get added to this list. Like I said, I think this should probably just be defined based on TraceKind.h.

By the way, the perf issue in the CC I was thinking of was bug 1301301. You can skip ahead to around comment 42 to see my analysis.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Andrew McCreight [:mccr8] from comment #3)
> By the way, the perf issue in the CC I was thinking of was bug 1301301. You
> can skip ahead to around comment 42 to see my analysis.

Thanks! I wonder if the Google Inbox case (bug 1242861) is hitting a similar bug for shapes instead of scopes? AddToCCKind returns false for Shapes.

I know we do trace shape/group children here:

https://searchfox.org/mozilla-central/rev/5ff2d7683078c96e4b11b8a13674daded935aa44/xpcom/base/CycleCollectedJSRuntime.cpp#435-438

TraceCycleCollectorChildren then traces the shape's propid, which is pretty slow in the profile in the other bug. It goes jsid -> JSString*/Symbol* -> realize CC isn't interested. Maybe we should just stop tracing propid in TraceCycleCollectorChildren.

https://searchfox.org/mozilla-central/rev/5ff2d7683078c96e4b11b8a13674daded935aa44/js/src/gc/Tracer.cpp#197

Also the comment in that file says:

// Thus, we walk the Shape
// lineage, but only report non-Shape things. This effectively makes the entire
// shape lineage into a single node in the CC, saving tremendous amounts of
// space and time in its algorithms.

Maybe instead of this we could only add shapes with (native?) getter/setter properties?

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> Maybe instead of this we could only add shapes with (native?) getter/setter
> properties?

Hm no that would still have the same perf issue when tracing lots of shapes.

I'm happy to try adding shapes to the CC graph (despite my CC ignorance) if you think it's a good idea, but I don't want to regress the "tremendous space/time" wins mentioned in the comment :)

Comment 6

[Andrew McCreight [:mccr8]]

(In reply to Jan de Mooij [:jandem] from comment #5)
> I'm happy to try adding shapes to the CC graph (despite my CC ignorance) if
> you think it's a good idea, but I don't want to regress the "tremendous
> space/time" wins mentioned in the comment :)

The profile in the Google Inbox case looked like it was spending almost all of its time tracing propid, which as you pointed out is useless. I think we should just fix that and then see if shape traversal is still a problem.

It would likely still make sense to ensure that both lists of CCKinds match up, though!

Comment 7

[Jan de Mooij [:jandem]]

Duping forward to bug 1463343 because it has a patch.