1186626 - Replace AutoShapeVector with Rooted<ShapeVector>

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: use_rooted_for_baseline_shapevec-v0.diff

This was eminently straightforward. I split this up more than I probably needed to, but it should at least help raise awareness of the new tools.

Comment 1

[Terrence Cole [:terrence]]

Attachment: use_rooted_for_selfhosting_shapevec-v0.diff

And the ShapeVectors in self-hosting.

Comment 2

[Terrence Cole [:terrence]]

Attachment: use_rooted_for_scopeobj_shapevec-v0.diff

Comment 3

[Terrence Cole [:terrence]]

Attachment: use_rooted_for_obj_shapes-v0.diff

Comment 4

[Terrence Cole [:terrence]]

Attachment: remove_AutoShapeVector-v0.diff

And those should be all the users.

Comment 5

[Till Schneidereit [:till]]

Comment on attachment 8637448 [details] [diff] [review]
use_rooted_for_selfhosting_shapevec-v0.diff

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

r=me

Comment 6

[Eric Faust [:efaust]]

Comment on attachment 8637449 [details] [diff] [review]
use_rooted_for_scopeobj_shapevec-v0.diff

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

APPROVED.

::: js/src/vm/ScopeObject.cpp
@@ +850,5 @@
>  
>      for (Shape::Range<NoGC> r(srcBlock->lastProperty()); !r.empty(); r.popFront())
> +        shapes[srcBlock->shapeToIndex(r.front())] = &r.front();
> +
> +    RootedId id(cx);

I like how you guys come through all the time and are like "people don't understand how to use Rooted at all" and just fix it. Thanks :)

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8637447 [details] [diff] [review]
use_rooted_for_baseline_shapevec-v0.diff

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

What about RootedShapeVector/HandleShapeVector typedefs for consistency, or do you think that's confusing?

Comment 8

[Terrence Cole [:terrence]]

(In reply to Jan de Mooij [:jandem] from comment #7)
> Comment on attachment 8637447 [details] [diff] [review]
> use_rooted_for_baseline_shapevec-v0.diff
> 
> Review of attachment 8637447 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What about RootedShapeVector/HandleShapeVector typedefs for consistency, or
> do you think that's confusing?

I recently noticed that someone typedefed RootedScriptSource to Rooted<ScriptSourceObject>. Of course this was only after a frantic hour of trying to figure out how in the world Rooted<ScriptSource> could possibly have compiled and what GC bugs it had introduced. This sort of thing makes me think that the typedefs were a mistake.

Comment 9

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

Comment on attachment 8637452 [details] [diff] [review]
remove_AutoShapeVector-v0.diff

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

\o/

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22e64a727f5c
https://hg.mozilla.org/integration/mozilla-inbound/rev/898eaedb48cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b25dd5d9f0e

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/22e64a727f5c
https://hg.mozilla.org/mozilla-central/rev/898eaedb48cb
https://hg.mozilla.org/mozilla-central/rev/3b25dd5d9f0e

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cc0bc6b707d
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cbb4de9b9c6

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6cc0bc6b707d
https://hg.mozilla.org/mozilla-central/rev/7cbb4de9b9c6