Closed Bug 793823 Opened 7 years ago Closed 7 years ago

GC: exact rooting for Bindings

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: terrence, Assigned: terrence)

References

Details

(Whiteboard: [rooting-example])

Attachments

(2 files, 3 obsolete files)

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.
Attached patch v0: templatized version (obsolete) — Splinter Review
This approach templatizes Bindings on the underlying Shape* type.
Attachment #664186 - Flags: feedback?(wmccloskey)
Attachment #664186 - Flags: feedback?(luke)
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.
Attachment #664191 - Flags: feedback?(wmccloskey)
Attached patch v0: Rooted<Bindings> (obsolete) — Splinter Review
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=.
Attachment #664192 - Flags: feedback?(wmccloskey)
Attachment #664192 - Flags: feedback?(luke)
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.
Attachment #664186 - Flags: feedback?(wmccloskey) → feedback+
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.
Attachment #664191 - Flags: feedback?(wmccloskey) → feedback-
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.
Attachment #664192 - Flags: feedback?(wmccloskey) → feedback+
Attachment #664186 - Flags: feedback?(luke)
Attachment #664192 - Flags: feedback?(luke)
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.
Attachment #664186 - Attachment is obsolete: true
Attachment #664191 - Attachment is obsolete: true
Attachment #664192 - Attachment is obsolete: true
Attachment #664278 - Flags: review?(wmccloskey)
Attachment #664278 - Flags: review?(wmccloskey) → review+
Attached patch v0Splinter Review
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.
Attachment #665177 - Flags: review?(wmccloskey)
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.
Attachment #665177 - Flags: review?(wmccloskey) → review+
Whiteboard: [leave-open]
https://hg.mozilla.org/mozilla-central/rev/7cdce684b523
https://hg.mozilla.org/mozilla-central/rev/c2c611cc8df4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Whiteboard: [rooting-example]
You need to log in before you can comment on or make changes to this bug.