795801 - IonMonkey: Enable ICing of StrictPropertyOp setters

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

Needed for Dromaeo regression.  Enable the generation of IC stubs for calling of StrictPropertyOp setters.  This can reuse the PropertyOp exit frame type from bug 786126.  The only extra parameter for StrictPropertyOp is a boolean argument that doesn't need to be rooted.

Comment 1

[Kannan Vijayan [:djvj]]

Testing patch for this in try:

https://tbpl.mozilla.org/?tree=Try&rev=3e1fead68c70

Comment 2

[Kannan Vijayan [:djvj]]

Tested android build on try as well, seems clean:

https://tbpl.mozilla.org/?tree=Try&rev=a0417dbcf6e7

Comment 3

[Kannan Vijayan [:djvj]]

Attachment: Handle StrictPropertyOp setters in SetProp ICs

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 673448 [details] [diff] [review]
Handle StrictPropertyOp setters in SetProp ICs

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

Yay! Just a few nits:

::: js/src/ion/IonCaches.cpp
@@ +887,5 @@
> +    {
> +        RegisterSet regSet(RegisterSet::All());
> +        regSet.take(AnyRegister(object()));
> +        if (!value().constant())
> +            regSet.maybeTake(value().reg());

Does this work on x86/ARM, where values have payloadReg/typeReg?

@@ +902,5 @@
> +        masm.movePtr(ImmGCPtr(holder), scratchReg);
> +        masm.branchPtr(Assembler::NotEqual,
> +                       Address(scratchReg, JSObject::offsetOfShape()),
> +                       ImmGCPtr(holder->lastProperty()),
> +                       &protoFailure);

Out of curiosity, why doesn't GeneratePrototypeGuards also guard on the holder's lastprop?

@@ +953,5 @@
> +    // WARNING:
> +    CodeOffsetLabel stubCodePatchOffset = masm.pushWithPatch(ImmWord(uintptr_t(-1)));
> +    // Manually adjust framePushed to account for this push which is not otherwise
> +    // accounted for.
> +    masm.setFramePushed(masm.framePushed() + sizeof(uintptr_t));

This pattern is a little unfortunate. Could we have masm.PushWithPatch? (One day, I'd like to get rid of framePushed entirely....)

@@ +1203,5 @@
> +    // (which implies JSNative setter).
> +    if (shape->hasSetterValue())
> +        return false;
> +
> +    fprintf(stderr, "KVKV: Got inlineable setter call!\n"); fflush(stderr);

nit: Don't forget to remove this.

Comment 5

[Kannan Vijayan [:djvj]]

(In reply to David Anderson [:dvander] from comment #4)
> Comment on attachment 673448 [details] [diff] [review]
> Handle StrictPropertyOp setters in SetProp ICs
> 
> Review of attachment 673448 [details] [diff] [review]:
> -----------------------------------------------------------------
> Does this work on x86/ARM, where values have payloadReg/typeReg?


Yes

> Out of curiosity, why doesn't GeneratePrototypeGuards also guard on the
> holder's lastprop?

Not sure, that's how they were before.  Looking at it, it certainly can be consolidated, but I'm reserving that for a general cleanup patch in IonCache since I have some other stuff to fix up with getter ICs anyway.

> This pattern is a little unfortunate. Could we have masm.PushWithPatch? (One
> day, I'd like to get rid of framePushed entirely....)

Good call, PushWithPatch already existed.  I just didn't realize it :)

Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/2583a19e59ef

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2583a19e59ef

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 673448 [details] [diff] [review]
Handle StrictPropertyOp setters in SetProp ICs

[Approval Request Comment]
Bug caused by (feature/regressing bug #): IonMonkey; this is the last piece of bug 803730 which is tracking-firefox18+
User impact if declined: Significant DOM performance regression
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): This code is very isolated, but ICs have been known to introduce bugs. This potentially affects a lot of DOM-heavy JavaScript. So far though, we haven't seen any regressions, it passes Dromaeo, and it is very easy to disable.
String or UUID changes made by this patch:

Comment 8

[bhavana bajaj [:bajaj]]

Comment on attachment 673448 [details] [diff] [review]
Handle StrictPropertyOp setters in SetProp ICs

Low risk, easy to disable patch for a perf win and needed as a piece for a  bug 803730 tracking 18. Approving on Aurora !

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3f6cc32f62e2