Closed Bug 795801 Opened 7 years ago Closed 7 years ago

IonMonkey: Enable ICing of StrictPropertyOp setters

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: djvj, Unassigned)

References

Details

(Whiteboard: [ion:p2])

Attachments

(1 file)

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.
Blocks: 786126
Blocks: 801105
Testing patch for this in try:

https://tbpl.mozilla.org/?tree=Try&rev=3e1fead68c70
Tested android build on try as well, seems clean:

https://tbpl.mozilla.org/?tree=Try&rev=a0417dbcf6e7
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.
Attachment #673448 - Flags: review?(dvander) → review+
Whiteboard: [ion:t] → [ion:p2]
(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
https://hg.mozilla.org/mozilla-central/rev/2583a19e59ef
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Depends on: 805747
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:
Attachment #673448 - Flags: approval-mozilla-aurora?
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 !
Attachment #673448 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 819610
Depends on: 819611
You need to log in before you can comment on or make changes to this bug.