Last Comment Bug 670152 - Restore per compartment empty call shape
: Restore per compartment empty call shape
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: general
:
Mentors:
Depends on:
Blocks: TypeInference 669815
  Show dependency treegraph
 
Reported: 2011-07-08 08:09 PDT by Brian Hackett (:bhackett)
Modified: 2011-07-15 13:50 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
backout call shape changes (20.20 KB, patch)
2011-07-08 08:55 PDT, Brian Hackett (:bhackett)
jorendorff: review+
Details | Diff | Splinter Review

Description Brian Hackett (:bhackett) 2011-07-08 08:09:14 PDT
The TI merge changed how call shapes work, so that each script has a separate shape hierarchy rather than all the hierarchies being rooted at a per-compartment singleton.  This has significantly increased the number of shapes allocated on real workloads, and the original behavior needs to be restored.

The change was made so that the shape guard in NAME ICs determines the script being accessed, which is necessary for propagating types correctly when generating these ICs.  With the addition of type barriers this now only affects SETNAME ICs.

Can fix this by adding a branch to the IC, but looking again I think now the change was never necessary in the first place: scope chains are immutable and should be isomorphic between all executions of an opcode.  The call objects on the scope chain will differ and may have different properties, but the scripts themselves and the order they appear on the chain will always be the same.
Comment 1 Brian Hackett (:bhackett) 2011-07-08 08:55:28 PDT
Created attachment 544827 [details] [diff] [review]
backout call shape changes

Restore call shape stuff to what it looked like before the TI merge, also add a comment to SETNAME IC update() about what's going on.
Comment 2 Jason Orendorff [:jorendorff] 2011-07-08 09:08:51 PDT
(In reply to comment #0)
> Can fix this by adding a branch to the IC, but looking again I think now the
> change was never necessary in the first place: scope chains are immutable
> and should be isomorphic between all executions of an opcode.  The call
> objects on the scope chain will differ and may have different properties,
> but the scripts themselves and the order they appear on the chain will
> always be the same.

Yeah, this is definitely correct.
Comment 3 Jason Orendorff [:jorendorff] 2011-07-08 09:53:29 PDT
Comment on attachment 544827 [details] [diff] [review]
backout call shape changes

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

r=me with or without the changes suggested below. They're just suggestions; I know you're in a hurry.

::: js/src/jsscript.cpp
@@ +361,1 @@
>      AutoBindingsRooter rooter(cx, bindings);

FWIW, I like the code with Bindings::ensureShape better than explicitly doing all this each time a Bindings object is used. Suggest keeping ensureShape and just changing it to call getEmptyCallShape instead of EmptyShape::create.

Also, there's nothing wrong with that AutoShapeRooter, but AutoBindingsRooter roots the shape anyway.

@@ -436,5 @@
>  
> -    if (xdr->mode == JSXDR_DECODE) {
> -        if (!bindings.ensureShape(cx))
> -            return false;
> -        bindings.makeImmutable();

In case you decide to keep ensureShape around: I'm not sure why the ensureShape call here is necessary. It seems like makeImmutable should just do nothing if lastBinding is NULL.

::: js/src/jsscriptinlines.h
@@ +69,5 @@
>      bindings->lastBinding = NULL;
>  #endif
>  
>      /* Preserve back-pointer invariants across the lastBinding transfer. */
>      if (lastBinding && lastBinding->inDictionary())

Assuming you *don't* decide to keep ensureShape(), then lastBinding can't be NULL here, right? You could assert instead of checking.

::: js/src/methodjit/PolyIC.cpp
@@ +644,5 @@
>                  return disable("scripted setter");
>              if (shape->setterOp() != SetCallArg &&
>                  shape->setterOp() != SetCallVar) {
>                  return disable("setter");
>              }

Might as well assert after this that obj->isCall(), if only to help explain what's going on here.
Comment 4 Jason Orendorff [:jorendorff] 2011-07-08 09:55:20 PDT
(In reply to comment #3)
> ::: js/src/jsscript.cpp
> @@ +361,1 @@
> >      AutoBindingsRooter rooter(cx, bindings);
> 
> FWIW, I like the code with Bindings::ensureShape better than explicitly
> doing all this each time a Bindings object is used. Suggest keeping
> ensureShape and just changing it to call getEmptyCallShape instead of
> EmptyShape::create.
> 
> Also, there's nothing wrong with that AutoShapeRooter, but
> AutoBindingsRooter roots the shape anyway.

Sigh, stupid splinter. The actual intended context from the patch was:

>+        EmptyShape *emptyCallShape = EmptyShape::getEmptyCallShape(cx);
>+        if (!emptyCallShape) {
>+            fun = NULL;
>+            goto out2;
>+        }
>+        AutoShapeRooter shapeRoot(cx, emptyCallShape);
>+
>         AutoObjectRooter tvr(cx, FUN_OBJECT(fun));
>         MUST_FLOW_THROUGH("out");
> 
>-        Bindings bindings(cx);
>+        Bindings bindings(cx, emptyCallShape);
Comment 5 Brian Hackett (:bhackett) 2011-07-08 11:18:25 PDT
Adds back the ensureShape stuff.

http://hg.mozilla.org/tracemonkey/rev/7161655b839f
Comment 6 Nicholas Nethercote [:njn] 2011-07-08 14:38:47 PDT
(In reply to comment #4)
> 
> Sigh, stupid splinter.

Everyone complains about Splinter.  You just need to realize it always gives you five lines of context, and insert your comment accordingly.

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