Last Comment Bug 745057 - Remove StackIter::fp usage where we want to abstract over StackFrame and IonFrames
: Remove StackIter::fp usage where we want to abstract over StackFrame and IonF...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86_64 Linux
: -- normal (vote)
: mozilla15
Assigned To: Nicolas B. Pierron [:nbp]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 749933
Blocks: 744253
  Show dependency treegraph
 
Reported: 2012-04-12 18:51 PDT by Nicolas B. Pierron [:nbp]
Modified: 2012-04-29 14:46 PDT (History)
5 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Use StackIter in TypeObject::clearNewScript (9.97 KB, patch)
2012-04-16 16:54 PDT, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
Use StackIter in TypeObject::clearNewScript (10.86 KB, patch)
2012-04-18 15:24 PDT, Nicolas B. Pierron [:nbp]
luke: review+
mark.finkle: approval‑mozilla‑central-
Details | Diff | Splinter Review
Rename FrameregsIter to ScriptFrameIter and remove tautology. (18.45 KB, patch)
2012-04-18 15:29 PDT, Nicolas B. Pierron [:nbp]
luke: review+
mark.finkle: approval‑mozilla‑central-
Details | Diff | Splinter Review

Description Nicolas B. Pierron [:nbp] 2012-04-12 18:51:50 PDT
StackIter::fp and StackIter::sp are not compatible with IonMonkey frame representation because IonMonkey is using the C stack instead of producing a StackFrame.  To handle different kind of frames StackIter need to abstract over the different frame representations.

The goal is to progressively remove most of the usage of StackIter::fp and StackIter::sp.
Comment 1 Nicolas B. Pierron [:nbp] 2012-04-16 16:54:15 PDT
Created attachment 615538 [details] [diff] [review]
Use StackIter in TypeObject::clearNewScript

Add function to StackIter to avoid using fp in TypeObject::clearNewScript as a way to iterate on frames.
Comment 2 Luke Wagner [:luke] 2012-04-16 18:04:22 PDT
Comment on attachment 615538 [details] [diff] [review]
Use StackIter in TypeObject::clearNewScript

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

::: js/src/jsinfer.cpp
@@ +3050,5 @@
> +            iter.callee().toFunction() == newScript->fun &&
> +            iter.thisv().isObject() &&
> +            !iter.thisv().toObject().hasLazyType() &&
> +            iter.thisv().toObject().type() == this) {
> +            JSObject *obj = &iter.thisv().toObject();

For multi-line conditionals, the (new) SM style is

  if (one &&
      two)
  {
      first line

so that we can clearly see the condition separate from the body.

@@ +3094,1 @@
>                              break;

Wow, what a crazy piece of code.  I may be missing an angle, but I think we can fix this in a way that doesn't require adding any gunk to StackIter and removes some quadratic perf: can we maintain a Vector<jsbytecode*> where each iteration of the outer for loop appends iter.pc()?  That way, when the inner loop needs to start going "forward", we can just walk back from the end of Vector directly?

::: js/src/vm/Stack.h
@@ +1850,5 @@
>          return state_ == NATIVE || state_ == IMPLICIT_NATIVE;
>      }
>  
>      bool isFunctionFrame() const;
> +    bool isScriptFrame() const;

For the same reason as before, I'd rather not have isScript and isScriptFrame.

@@ +1859,5 @@
>      StackFrame *fp() const { JS_ASSERT(isScript()); return fp_; }
>      Value      *sp() const { JS_ASSERT(isScript()); return sp_; }
>      jsbytecode *pc() const { JS_ASSERT(isScript()); return pc_; }
>      JSScript   *script() const { JS_ASSERT(isScript()); return script_; }
>      JSObject   &callee() const;

Can you change callee() to return a JSFunction &?

@@ +1873,3 @@
>      void settle() {
> +        while (!done() && !isScript())
> +            this->StackIter::operator++();

Don't need "this->"

@@ +1881,2 @@
>  
> +    FrameRegsIter &operator++() { this->StackIter::operator++(); settle(); return *this; }

You can leave off "this->"
Comment 3 Nicolas B. Pierron [:nbp] 2012-04-16 20:46:17 PDT
(In reply to Luke Wagner [:luke] from comment #2)
> Comment on attachment 615538 [details] [diff] [review]
> Use StackIter in TypeObject::clearNewScript
> 
> Review of attachment 615538 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsinfer.cpp
> @@ +3050,5 @@
> > +            iter.callee().toFunction() == newScript->fun &&
> > +            iter.thisv().isObject() &&
> > +            !iter.thisv().toObject().hasLazyType() &&
> > +            iter.thisv().toObject().type() == this) {
> > +            JSObject *obj = &iter.thisv().toObject();
> 
> For multi-line conditionals, the (new) SM style is
> 
>   if (one &&
>       two)
>   {
>       first line
> 
> so that we can clearly see the condition separate from the body.
> 
> @@ +3094,1 @@
> >                              break;
> 
> Wow, what a crazy piece of code.  I may be missing an angle, but I think we
> can fix this in a way that doesn't require adding any gunk to StackIter and
> removes some quadratic perf: can we maintain a Vector<jsbytecode*> where
> each iteration of the outer for loop appends iter.pc()?  That way, when the
> inner loop needs to start going "forward", we can just walk back from the
> end of Vector directly?

bhackett told me that this was a corner case which is almost never happens.  It occurs when the "this" object is given as a function argument during the construction and that the function later cause a deoptimization of the shape of the current object, then I hope TI learn from its mistakes.

As this is a corner case I did not try to optimize it much, as suggested by bhackett, otherwise we can update the vector lazily by using a second iterator which join the outer loop one. which avoid consuming unused space on the common case.  What do you think ?

> ::: js/src/vm/Stack.h
> @@ +1850,5 @@
> >          return state_ == NATIVE || state_ == IMPLICIT_NATIVE;
> >      }
> >  
> >      bool isFunctionFrame() const;
> > +    bool isScriptFrame() const;
> 
> For the same reason as before, I'd rather not have isScript and
> isScriptFrame.

Currently I don't want to change the behaviour of the functions mapped such as one can be replaced by the other.
  iter.fp()->isScriptFrame()  --->  iter.isScriptFrame()

I was also surprise by this, but the line of TypeObject::clearNewType let me think that they currently have different semantic because FrameRegsIter iterate on "isScript" frames and we are still checking isScriptFrame on it:

    for (FrameRegsIter iter(cx); !iter.done(); ++iter) {
        StackFrame *fp = iter.fp();
        if (fp->isScriptFrame() && fp->isConstructing() &&

Should they be identical ?
Comment 4 Luke Wagner [:luke] 2012-04-17 08:41:51 PDT
(In reply to Nicolas B. Pierron [:pierron] from comment #3)
> As this is a corner case I did not try to optimize it much, as suggested by
> bhackett,

I wasn't trying to optimize, per se, just trying to replace doing something complex (finding the next frame (both the way it is currently done using StackSegments and the way in the patch with hasNext/nextSlow) with something simple (maintaining a list of frames).

> otherwise we can update the vector lazily by using a second
> iterator which join the outer loop one. which avoid consuming unused space
> on the common case.  What do you think ?

I suspect this whole function is relatively quite cold, so I think there is no problem maintaining just a Vector for common cases.  Also, if you use Vector<32> or so we shouldn't expect any malloc().

> Should they be identical ?

Since

  StackFrame::isScriptedFrame === !StackFrame::isDummyFrame

and since StackIter always skips dummy frames (see StackIter::settleOnNewState), isScriptFrame should be entirely superfluous when we know isScript() == true (which, by construction, we do for FrameRegsIter, which you should feel free to rename (ScriptedFrameIter?)).
Comment 5 Nicolas B. Pierron [:nbp] 2012-04-18 15:24:56 PDT
Created attachment 616313 [details] [diff] [review]
Use StackIter in TypeObject::clearNewScript

Apply nits, and use a vector to cache pc offsets in TypeObject::clearNewScript.
Comment 6 Nicolas B. Pierron [:nbp] 2012-04-18 15:29:07 PDT
Created attachment 616317 [details] [diff] [review]
Rename FrameregsIter to ScriptFrameIter and remove tautology.

Take advantage of the renaming to remove references to isScriptFrames as documented in comment 4.
Comment 7 Luke Wagner [:luke] 2012-04-18 16:02:41 PDT
Comment on attachment 616313 [details] [diff] [review]
Use StackIter in TypeObject::clearNewScript

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

Right on.  (Assuming green on try) this shouldn't affect mobile, so if I were you I'd request approval-mozilla-central.

::: js/src/jsinfer.cpp
@@ +3083,5 @@
>                      } else if (init->offset > offset) {
>                          /* Advanced past all properties which have been initialized. */
>                          break;
>                      } else if (init->offset == offset) {
> +                        offset = pcOffsets[++callDepth];

sweeeet

::: js/src/vm/Stack.cpp
@@ +1325,5 @@
> +        return false;
> +      case SCRIPTED:
> +      case NATIVE:
> +      case IMPLICIT_NATIVE:
> +        return fp()->isConstructing();

I think NATIVE and IMPLICIT_NATIVE would be in the JS_NOT_REACHED case.

@@ +1374,5 @@
> +        return Value();
> +      case SCRIPTED:
> +      case NATIVE:
> +      case IMPLICIT_NATIVE:
> +        return fp()->thisValue();

I think NATIVE and IMPLICIT_NATIVE need their own case (nativeArgs().thisv()).

::: js/src/vm/Stack.h
@@ +1834,5 @@
>      bool done() const { return state_ == DONE; }
>      StackIter &operator++();
>  
> +    bool hasNext() const;
> +    StackIter &slowNext();

I think these can be removed now.
Comment 8 Luke Wagner [:luke] 2012-04-18 16:05:08 PDT
Comment on attachment 616317 [details] [diff] [review]
Rename FrameregsIter to ScriptFrameIter and remove tautology.

Sweeeet.  Ditto comment about try + approval-mozilla-central.
Comment 9 Nicolas B. Pierron [:nbp] 2012-04-19 16:15:12 PDT
Comment on attachment 616313 [details] [diff] [review]
Use StackIter in TypeObject::clearNewScript

This patch is not working because the iteration over the saved pcOffsets is in the reverse order, restore the correct order with these modifications:

> +            size_t callDepth = 0;
> +            uint32_t offset = pcOffsets[callDepth];

            size_t callDepth = pcOffsets.length() -1;

> -                        pc = fp->pcQuadratic(cx->stack);
> +                        offset = pcOffsets[++callDepth];

                        if(!callDepth)
                            break;
                        offset = pcOffsets[--callDepth];
Comment 10 Nicolas B. Pierron [:nbp] 2012-04-19 16:16:03 PDT
Comment on attachment 616317 [details] [diff] [review]
Rename FrameregsIter to ScriptFrameIter and remove tautology.

Both patches are applied and build on the try server:
https://tbpl.mozilla.org/?tree=Try&rev=586f417093c8
Comment 11 Mark Finkle (:mfinkle) (use needinfo?) 2012-04-21 13:22:38 PDT
Comment on attachment 616317 [details] [diff] [review]
Rename FrameregsIter to ScriptFrameIter and remove tautology.

we think this could wait for 15
Comment 12 Mark Finkle (:mfinkle) (use needinfo?) 2012-04-21 13:22:47 PDT
Comment on attachment 616313 [details] [diff] [review]
Use StackIter in TypeObject::clearNewScript

we think this could wait for 15
Comment 13 David Anderson [:dvander] 2012-04-22 20:28:36 PDT
This is blocking important IonMonkey work.
Comment 14 Jeff Walden [:Waldo] (remove +bmo to email) 2012-04-28 10:21:09 PDT
I fixed a warning on thisv() not always returning a value, reported in bug 749951 and bug 749933:

https://hg.mozilla.org/integration/mozilla-inbound/rev/27d42a3feef1

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