1199843 - Strongly type JS_TraceChildren (and avoid some overhead where possible)

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: js_TraceChildren_reductions-v0.diff

There are 3 patches here:

1) Call t->traceChildren(&trc) where possible in favor of js::TraceChildren(&trc, t). No reason to use tag dispatch unless we have to.

2) Where we're actually using the generality effectively, call js::TraceChildren instead of JS_TraceChildren everywhere where we don't actually need external linkage.

3) Rename JS_TraceChildren to JS::TraceChildren and pass through gecko's existing GCCellPtrs.

Comment 1

[Terrence Cole [:terrence]]

Attachment: reduce_external_TraceChildren_calls-v0.diff

Comment 2

[Terrence Cole [:terrence]]

Attachment: strongly_type_JS_TraceChildren-v0.diff

Comment 3

[Jon Coppeard (:jonco)]

Comment on attachment 8654420 [details] [diff] [review]
js_TraceChildren_reductions-v0.diff

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

This is fine, but for the gray unmarking case it just moves the typed dispatch somewhere else.

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 8654422 [details] [diff] [review]
reduce_external_TraceChildren_calls-v0.diff

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

::: js/public/UbiNode.h
@@ +759,5 @@
>  //
>  // Concrete instances of this class need not be as lightweight as Node itself,
>  // since they're usually only instantiated while iterating over a particular
>  // object's edges. For example, a dumb implementation for JS Cells might use
> +// js::TraceChildren to to get the outgoing edges, and then store them in an

The UbiNode interface is public, so shouldn't we be telling people to use (or at least talking about) the version with external linkage here?

Comment 5

[Jon Coppeard (:jonco)]

Comment on attachment 8654423 [details] [diff] [review]
strongly_type_JS_TraceChildren-v0.diff

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

Nice. r=me for the engine changes.

Comment 6

[Terrence Cole [:terrence]]

(In reply to Jon Coppeard (:jonco) from comment #3)
> Comment on attachment 8654420 [details] [diff] [review]
> js_TraceChildren_reductions-v0.diff
> 
> Review of attachment 8654420 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is fine, but for the gray unmarking case it just moves the typed
> dispatch somewhere else.

Yeah, it only removes it for shape, but that should be a fairly common case, so I think it's still a win on balance.

(In reply to Jon Coppeard (:jonco) from comment #4)
> Comment on attachment 8654422 [details] [diff] [review]
> reduce_external_TraceChildren_calls-v0.diff
> 
> Review of attachment 8654422 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/public/UbiNode.h
> @@ +759,5 @@
> >  //
> >  // Concrete instances of this class need not be as lightweight as Node itself,
> >  // since they're usually only instantiated while iterating over a particular
> >  // object's edges. For example, a dumb implementation for JS Cells might use
> > +// js::TraceChildren to to get the outgoing edges, and then store them in an
> 
> The UbiNode interface is public, so shouldn't we be telling people to use
> (or at least talking about) the version with external linkage here?

Yes, it doesn't matter which one we're using, so I'll change it back. Although it's a bit weird that we even mention the implementation details here in the interface at all.

Comment 7

[Andrew McCreight [:mccr8]]

Comment on attachment 8654423 [details] [diff] [review]
strongly_type_JS_TraceChildren-v0.diff

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

Nice. r=me for the non-spidermonkey changes

Sorry for the delay, I forgot about this.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eaa29e219ffc
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7ef78ecf5b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7d430e2d19e

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/eaa29e219ffc
https://hg.mozilla.org/mozilla-central/rev/e7ef78ecf5b8
https://hg.mozilla.org/mozilla-central/rev/a7d430e2d19e