Last Comment Bug 763096 - GC: Make ScopeIter support exact stack rooting
: GC: Make ScopeIter support exact stack rooting
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Terrence Cole [:terrence]
:
Mentors:
Depends on:
Blocks: ExactRooting
  Show dependency treegraph
 
Reported: 2012-06-08 15:49 PDT by Terrence Cole [:terrence]
Modified: 2012-06-15 05:59 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v0 (27.52 KB, patch)
2012-06-08 15:49 PDT, Terrence Cole [:terrence]
luke: review+
Details | Diff | Review

Description Terrence Cole [:terrence] 2012-06-08 15:49:18 PDT
Created attachment 631559 [details] [diff] [review]
v0

ScopeIter stores two object pointers internally *and* it needs to live on both the stack and the heap.  The attached patch makes ScopeIter's internal object pointers Rooted<> and splits out the hashing code (e.g. heap storage) to a ScopeIterKey.  ScopeIter implicitly converts to a ScopeIterKey, so no changes were required to users, other than the changes needed to not use ScopeIter as a temporary, as required by Rooted.

Given the strict requirements and the use of JS_GUARD_OBJECT gunk, this came out surprisingly cleanly.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=b70a60c7dcd9
Comment 1 Luke Wagner [:luke] 2012-06-11 15:25:07 PDT
Comment on attachment 631559 [details] [diff] [review]
v0

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

Thanks, great job!  (Sorry you had to deal with it, should've expected this.)

It really seems like ScopeIter shouldn't have value semantics and thus shouldn't be implicitly copyable.  Could you write another patch for this bug that (1) makes ScopeIter's copy constructor explicit (which should break si.enclosing(), tell me if it doesn't), (2) changes enclosing() to operator++?

::: js/src/vm/ScopeObject.cpp
@@ +1497,4 @@
>              e.removeFront();
>              continue;
> +        } else {
> +            e.rekeyFront(scope);

I think this part gets left out for now :)

@@ +1590,5 @@
>      return true;
>  }
>  
>  void
> +DebugScopes::onPopCall(JSContext *cx, StackFrame *fp)

Can you make 'cx' be the last parameter (to indicate infallibility) on this and every other member you added a 'cx' to?
Comment 2 Terrence Cole [:terrence] 2012-06-11 15:44:12 PDT
(In reply to Luke Wagner [:luke] from comment #1)
> It really seems like ScopeIter shouldn't have value semantics and thus
> shouldn't be implicitly copyable.  Could you write another patch for this
> bug that (1) makes ScopeIter's copy constructor explicit (which should break
> si.enclosing(), tell me if it doesn't), (2) changes enclosing() to
> operator++?

I actually tried non-copyable semantics first.  I backed off of it because enclosing() broke severely and I wasn't sure you'd want such a huge change to the usage.  :-)
 
> ::: js/src/vm/ScopeObject.cpp
> @@ +1497,4 @@
> >              e.removeFront();
> >              continue;
> > +        } else {
> > +            e.rekeyFront(scope);
> 
> I think this part gets left out for now :)

This particular instance is buggy -- we use |e| further down in loop.  That said, it shouldn't hurt to keep it around (in the correct place, of course). It won't get called and it serves as a reminder to anyone hacking in these parts to keep moving GC in mind.

I do realize, however, that "It's convenient for me" isn't necessarily the best reason to carry this, so I'm open to counter arguments.
 
> @@ +1590,5 @@
> >      return true;
> >  }
> >  
> >  void
> > +DebugScopes::onPopCall(JSContext *cx, StackFrame *fp)
> 
> Can you make 'cx' be the last parameter (to indicate infallibility) on this
> and every other member you added a 'cx' to?

Huh, I had totally missed that subtlety.  I figured the methods that took cx last were just non-standard stuff from the long-ago-before-time.
Comment 3 Luke Wagner [:luke] 2012-06-11 15:52:59 PDT
(In reply to Terrence Cole [:terrence] from comment #2)
> > > +        } else {
> > > +            e.rekeyFront(scope);
> > 
> > I think this part gets left out for now :)
> 
> This particular instance is buggy -- we use |e| further down in loop.  That
> said, it shouldn't hurt to keep it around (in the correct place, of course).
> It won't get called and it serves as a reminder to anyone hacking in these
> parts to keep moving GC in mind.

It will get called, it's just a nop, I believe.  Also, it does hurt, b/c it makes the code more confusing since it's a nop.  I don't think it will remind anyone of anything in particular; if you do have something to say, use a comment.
Comment 4 Terrence Cole [:terrence] 2012-06-14 10:20:05 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e079dfd1285e
Comment 5 Ed Morley [:emorley] 2012-06-15 05:59:32 PDT
https://hg.mozilla.org/mozilla-central/rev/e079dfd1285e

Note You need to log in before you can comment on or make changes to this bug.