793823 - GC: exact rooting for Bindings

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Bindings stores a Shape pointer and is itself stored both on the stack and on the heap.  It currently stores the Shape* as a HeapPtrShape.  This leads to a rooting problem when we store the Bindings on the stack because the exact rooting does not know about the internal Shape* in the Bindings.

Comment 1

[Terrence Cole [:terrence]]

Attachment: v0: templatized version

This approach templatizes Bindings on the underlying Shape* type.

Comment 2

[Terrence Cole [:terrence]]

Attachment: v0: templatize root analysis APIs

This patch cleans up the way we do root analysis and exact rooting so that we can specialize it enough to make it possible to add a Rooted for Bindings.  I think this patch is a good enough cleanup to stand on it's own, so I've split it out.

Comment 3

[Terrence Cole [:terrence]]

Attachment: v0: Rooted<Bindings>

This patch implements and uses Rooted<Bindings> using the new analysis functionality.  It also solves the issue uncovered by the root analysis, but less succinctly: since the internal pointer is still a HeapPtr stored on the stack, we have to be careful to ensure correct ordering between operator-> and operator= when assigning to it from a fallible method so that we do not corrupt the HeapPtr's |this| during operator=.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 664186 [details] [diff] [review]
v0: templatized version

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

::: js/src/jsscript.h
@@ +215,5 @@
>  
>      void trace(JSTracer *trc);
>  };
>  
> +typedef InternalHandle<BindingsBase<js::HeapPtrShape>*> HeapBindingsHandle;

Could you call this InternalBindingsHandle?

@@ +216,5 @@
>      void trace(JSTracer *trc);
>  };
>  
> +typedef InternalHandle<BindingsBase<js::HeapPtrShape>*> HeapBindingsHandle;
> +typedef InternalHandle<BindingsBase<RootedShape>*> StackBindingsHandle;

If I understand InternalHandle correctly, I don't think it would ever make sense to have a StackBindingsHandle.

::: js/src/vm/ScopeObject.cpp
@@ +1122,5 @@
>              JSScript *script = callobj.callee().script();
>              if (!script->ensureHasTypes(cx))
>                  return false;
>  
> +            HeapBindings &bindings = script->bindings;

Could you make this an InterbalHeapBindingsHandle instead? It seems safer.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 664191 [details] [diff] [review]
v0: templatize root analysis APIs

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

I'd rather not do this. It just seems to add more indirection that we don't actually need.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 664192 [details] [diff] [review]
v0: Rooted<Bindings>

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

I guess this is okay, except for the bits that overlap with the previous patch.

Comment 7

[Terrence Cole [:terrence]]

Attachment: v1: updated method 2

I realized when updating the exact marking code that I forgot to update it when I added rooters to the runtime, so I folded that change in as well.

Comment 8

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b88bc53d09a2

Comment 9

[Terrence Cole [:terrence]]

Attachment: v0

Currently, BindingIter stores a Bindings*: this is sometimes a pointer into a JSScript, which causes the analysis to poison it.  This patch straightforwardly replaces this Bindings* with an InternalBindingsHandle.

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b88bc53d09a2

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 665177 [details] [diff] [review]
v0

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

::: js/src/jsanalyze.cpp
@@ +141,5 @@
>      bool allVarsAliased = script->compartment()->debugMode();
>      bool allArgsAliased = allVarsAliased || script->argumentsHasVarBinding();
>  
> +    RootedScript script_(cx, script);
> +    for (BindingIter bi(script_); bi; bi++) {

It looks like Nick changed this recently, and you can just use the script_ field directly without need for the rooted.

::: js/src/jsscript.cpp
@@ +267,1 @@
>      for (BindingIter bi(bindings); bi; bi++) {

It seems like you could just pass fromScript into the bi constructor.

Comment 12

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7cdce684b523
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2c611cc8df4

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7cdce684b523
https://hg.mozilla.org/mozilla-central/rev/c2c611cc8df4