844253 - BaselineCompiler: Correctly perform UseNewType check when calling constructors in Baseline.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

This is one of the major reasons DeltaBlue is slow with Baseline right now.

|CreateThis| for constructing calls ends up assigning the same TypeObject to prototype objects which should have different types.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: Patch

Comment 2

[Brian Hackett [Laid off!]]

Comment on attachment 717298 [details] [diff] [review]
Patch

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

I have a couple concerns with this patch:

- It will be extremely rare for UseNewType to return true, and I think it would be cleaner if Baseline just didn't attach a stub in this case rather than add machinery so that the stub can construct 'this' objects with singleton type.

- The InvokeConstructor call made in DoCallFallback when there is not yet a stub doesn't look like it will create a singleton type for the new object.  The UseNewType bits seem to be ignored by this function when entering baseline or Ion code, and while the interpreter calls UseNewTypeAtEntry, that function just looks at StackFrames and will probably behave incorrectly when the previous frame is an Ion activation.  This is more invasive, but I think it would be better if the newType flag was passed through InvokeConstructor, that jitcode is not entered when newType is set, and the interpreter uses the passed in flag.

Comment 3

[Kannan Vijayan [:djvj]]

Good eye on catching that oversight.

I agree with most of your commentary, but I'm not sure about the suggested idea of threading the flag through the family of Invoke functions, through RunScript, and into Interpreter.

For such a rarely used flag that is largely tangential to real execution semantics, adding it in as an explicit argument seems to give it more explicit mention than it justifies.  Here's a new patch that computes that info within |RunScript| and passes it into |js::Interpret| (as well as the entry functions to Ion and Baseline).

I'm posting two patches:

The first changes RunScript to use a ScriptIter to calculate the newScript flag, and pass it into Ion, Baseline, and the Interpreter.  This necessitate adding a newScript boolean argment to js::Interpret.

The second patch changes newScript to be a bit on the flag field of the StackFrame object.  This allows eliminating newScript as an argument from many functions.

If this looks OK, I can push the second patch into inbound, and the first into baseline.

Comment 4

[Kannan Vijayan [:djvj]]

Attachment: Make RunScript calclualte useNewType

Comment 5

[Kannan Vijayan [:djvj]]

Attachment: Change useNewType to be a flag on StackFrame

Comment 6

[Brian Hackett [Laid off!]]

Comment on attachment 717631 [details] [diff] [review]
Change useNewType to be a flag on StackFrame

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

Nice!

::: js/src/vm/Stack.h
@@ +411,5 @@
>  
> +        JIT_REVISED_STACK  =   0x800000,  /* sp was revised by JIT for lowered apply */
> +
> +        /* Miscellaneous state. */
> +        USE_NEW_TYPE       =  0x1000000   /* Use new type constructed |this| object. */

missing 'for'

Comment 7

[Brian Hackett [Laid off!]]

Comment on attachment 717630 [details] [diff] [review]
Make RunScript calclualte useNewType

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

::: js/src/ion/BaselineIC.cpp
@@ +4338,3 @@
>  {
> +    // Ensure vp array is rooted - we may GC in here.
> +    AutoArrayRooter vpRoot(cx, argc + 2, vp);

Won't the values that vp points to be traced by a GC, since they are on the baseline stack?

Comment 8

[Kannan Vijayan [:djvj]]

(In reply to Brian Hackett (:bhackett) from comment #7)
> Comment on attachment 717630 [details] [diff] [review]
> Make RunScript calclualte useNewType
> 
> Review of attachment 717630 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/BaselineIC.cpp
> @@ +4338,3 @@
> >  {
> > +    // Ensure vp array is rooted - we may GC in here.
> > +    AutoArrayRooter vpRoot(cx, argc + 2, vp);
> 
> Won't the values that vp points to be traced by a GC, since they are on the
> baseline stack?

There are two copies of the arguments: one in "reverse" order in the baseline frame proper, and a copy (in correct order) in the stub frame.  The pointer passed to the C++ function is hte latter.  The value array in the stub frame aren't traced.  This is not an issue right now because the args are kept live from their traced roots in the BaselineFrame, but the copied array won't be stable once we have a moving GC.

Comment 9

[Kannan Vijayan [:djvj]]

Inbound patch to make useNewType a flag on StackFrame: https://hg.mozilla.org/integration/mozilla-inbound/rev/0fc2a36c23d8

Comment 10

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/96413f5e4533 for everyone's favorite, "Assertion failure: cx->typeInferenceEnabled()," in everyone's favorite spot, startup cache precompilation: https://tbpl.mozilla.org/php/getParsedLog.php?id=20056214&tree=Mozilla-Inbound

Comment 11

[Kannan Vijayan [:djvj]]

Fixed and repushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/5b26fb5a22b9

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5b26fb5a22b9

Comment 13

[Kannan Vijayan [:djvj]]

Sorry, forgot to add [leave open] to whiteboard.  There's another patch to land on this bug in the ionmonkey repos after this m-i push gets merged.

Comment 14

[Kannan Vijayan [:djvj]]

Second patch: https://hg.mozilla.org/projects/ionmonkey/rev/85a6a105c491

Leaving open until ion tbpl is green.