Strongly type JS_TraceChildren (and avoid some overhead where possible)

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(3 attachments)

Assignee

Description

4 years ago
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.
Attachment #8654420 - Flags: review?(jcoppeard)
Assignee

Comment 2

4 years ago
Attachment #8654423 - Flags: review?(jcoppeard)
Attachment #8654423 - Flags: review?(continuation)
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.
Attachment #8654420 - Flags: review?(jcoppeard) → review+
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?
Attachment #8654422 - Flags: review?(jcoppeard) → review+
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.
Attachment #8654423 - Flags: review?(jcoppeard) → review+
Assignee

Comment 6

4 years ago
(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 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.
Attachment #8654423 - Flags: review?(continuation) → review+
You need to log in before you can comment on or make changes to this bug.