The default bug view has changed. See this FAQ.

Restore per compartment empty call shape

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bhackett, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
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.
(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.
Attachment #544827 - Flags: review?(jorendorff)
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.
Attachment #544827 - Flags: review?(jorendorff) → review+
(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);
Adds back the ensureShape stuff.

http://hg.mozilla.org/tracemonkey/rev/7161655b839f
Whiteboard: fixed-in-tracemonkey
(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.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-tracemonkey
You need to log in before you can comment on or make changes to this bug.