935999 - IonMonkey: add-property stub: support type checks

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

Just like the SetSlot stub.

WIP patches for bug 935932 and this win about 1500-2000 points on Octane-Typescript, but Chrome is still 10000 points ahead.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

Wins about ~12% on Octane-Typescript. Patch is fairly straight-forward, it moves some code into a new function, CanInlineSetPropTypeCheck, so that we can use it for both sets and adds.

Comment 2

[Jan de Mooij [:jandem]]

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

Shu is on PTO and merge is in a few weeks, switching review to efaust.

Comment 3

[Shu-yu Guo [:shu]]

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

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

Stealing review back because Jan mistakenly thought I was on PTO the entire week. :)

Looks good to me.

::: js/src/jit/IonCaches.cpp
@@ +2556,5 @@
>  
>      // Guard shapes along prototype chain.
>      masm.branchTestObjShape(Assembler::NotEqual, object, oldShape, &failures);
>  
> +    Label failuresPopObject;

Oh good, this rename makes the logic clearer.

@@ +2571,5 @@
> +        Register scratchReg = object;
> +        masm.push(scratchReg);
> +
> +        masm.guardTypeSet(valReg, propTypes, scratchReg, &failuresPopObject);
> +        masm.pop(object);

Seems like we only need to push the object reg once when |checkTypeset| is true instead of popping it then pushing it again.

@@ +2638,5 @@
>      return linkAndAttachStub(cx, masm, attacher, ion, "adding");
>  }
>  
>  static bool
> +CanInlineSetPropTypeCheck(JSObject *obj, jsid id, ConstantOrRegister val, bool *checkTypeset)

Nit: the function name confused me, since it starts with "Can", I thought it'd be a simple predicate that returns true/false to mean whether or not the setprop can typecheck. Not sure what a better name would be though.

Comment 4

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ff280bab60c

(In reply to Shu-yu Guo [:shu] from comment #3)
> Seems like we only need to push the object reg once when |checkTypeset| is
> true instead of popping it then pushing it again.

Fixed, thanks.

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2ff280bab60c