The default bug view has changed. See this FAQ.

IonMonkey: Fast path for JSOP_SETPROP adding case

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dvander, Assigned: djvj)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Needed for v8 - a fair number of addprop cases hit there. IC should be fine.
(Reporter)

Comment 1

5 years ago
// vim: set ts=4 sw=4 tw=99 et:
function f() {
    var t = [];
    for (var i = 0; i < 100000; i++) {
        var x = { };
        x.y = i;
        t[i] = x;
    }
    return t;
}
f();

Small test case that demonstrates when we'd need an IC. Here, |x| starts out with initial shape S1. When |x.y = i| runs, we want to transition x's shape to S2 before storing i. JM does this in PolyIC.cpp, under SetPropCompiler::update (adding=true).
(Assignee)

Updated

5 years ago
Assignee: general → kvijayan
(Assignee)

Comment 2

5 years ago
Created attachment 612295 [details]
Patch v1

Very rough description:

If:

1. Additional space exists in IC
2. Object is simple native
3. Property is not defined on object
4. All prototypes of the object are native
5. All prototypes of the object either do not define the property or have a default setter for the property
6. The new shape of the object after setting the property does not cause a reallocation of the slots array for the object

Then an additional case is added to the IC which inlines the transition of the object to its new shape and the setting of the property value.
Attachment #612295 - Flags: review?(bhackett1024)
(Assignee)

Comment 3

5 years ago
Created attachment 612296 [details] [diff] [review]
Patch v1 again.

Didn't mark previous patch as a patch, so it was showing up as plain text.
Attachment #612295 - Attachment is obsolete: true
Attachment #612295 - Flags: review?(bhackett1024)
(Assignee)

Updated

5 years ago
Attachment #612296 - Flags: review?(bhackett1024)
(Assignee)

Updated

5 years ago
Attachment #612296 - Flags: review?(dvander)
(Reporter)

Comment 4

5 years ago
Comment on attachment 612296 [details] [diff] [review]
Patch v1 again.

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

::: js/src/ion/IonCaches.cpp
@@ +392,5 @@
> +        masm.loadPtr(Address(protoReg, offsetof(types::TypeObject, proto)), protoReg);
> +
> +        // ensure that the prototype is not NULL and that its shape matches
> +        masm.branchPtr(Assembler::Equal, protoReg, ImmWord(uintptr_t(0)), &protoFailures);
> +        masm.branchTestObjShape(Assembler::NotEqual, protoReg, protoShape, &protoFailures);

The first branchPtr is better as branchTestPtr (which performs a bitwise-and test):
   branchTestPtr(Assembler::Zero, protoReg, &protoFailures);

existing nit: branchTestObjShape should be renamed "branchObjShape" because it uses subtract-based comparison, not and-based.

@@ +488,5 @@
> +
> +                // walk up the object prototype chain and ensure that all prototypes
> +                // are native, and that all prototypes have no getter or setter
> +                // defined on the property
> +                for(JSObject *proto = obj->getProto(); proto; proto = proto->getProto()) {

nit: space in between for and (

To make this function easier to read, it might be better to split this loop out into a separate function that returns whether prototypes are ok.
Attachment #612296 - Flags: review?(dvander) → review+
Comment on attachment 612296 [details] [diff] [review]
Patch v1 again.

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

::: js/src/ion/IonCaches.cpp
@@ +409,5 @@
> +        masm.storeConstantOrRegister(value(), addr);
> +    } else {
> +        Register slotsReg = object();
> +
> +        masm.loadPtr(Address(object(), JSObject::offsetOfSlots()), slotsReg);

Does the object reg need to be saved/restored here too?  I think the SETPROP IC is supposed to preserve the object reg's value.
Attachment #612296 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 6

5 years ago
The case for attachNativeExisting doesn't seem to save the object() register value in its version of the code either.

So either both are incorrect or neither.  I tried looking at the code, but I'm not sure what to check for to see if this is the case (object register is required to be preserved).

For now, I'm doing final tests on the patch without saving object register.

Can someone who knows the requirements for IC advise?  If it's required, I'll open another bug for that and fix both cases there.
(Assignee)

Comment 7

5 years ago
Created attachment 612710 [details] [diff] [review]
Patch v2

Asking for review on this again for the following reason:

1. Don't know answer to bhackett's statement above.  Should I do more work to preserve the object() register through the IC?

2. I had a question about the correctness of one piece of logic.

Within SetPropertyCache, in the conditional branch for the addprop case, I do the following:
  * setGeneric on the object to manually set the property
  * attachNativeAdd to add a case to the IC

If setGeneric succeeds, but the attachNativeAdd fails, then we will return with a 'false', but will have performed the effect that was requested.  On the other hand, this will only be the case if the SETPROP itself is idempotent (since the second failure will happen if-and-only-if the property has no special behaviour, so a repeat of the SETPROP will have no discernable high-level effect).

However, I'm not sure if this might still trigger some issue, or invalidate some assumptions within the VM.
Attachment #612296 - Attachment is obsolete: true
Attachment #612710 - Flags: review?(dvander)
(Reporter)

Comment 8

5 years ago
Comment on attachment 612710 [details] [diff] [review]
Patch v2

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

::: js/src/ion/IonCaches.cpp
@@ +391,5 @@
> +        masm.loadPtr(Address(protoReg, JSObject::offsetOfType()), protoReg);
> +        masm.loadPtr(Address(protoReg, offsetof(types::TypeObject, proto)), protoReg);
> +
> +        // ensure that the prototype is not NULL and that its shape matches
> +        masm.branchTestPtr(Assembler::Equal, protoReg, protoReg, &protoFailures);

nit: for branchTestPtr, you want to test Assembler::Zero (they're technically the same flag but it's clearer to write Zero)

@@ +448,5 @@
> +    return true;
> +}
> +
> +static bool IsEligibleForInlinePropertyAdd(JSContext *cx, JSObject *obj, jsid propId, uint32_t oldSlots,
> +                                         const Shape **propShapeOut)

Thanks for moving this out! Style nit: static bool goes on its own line.
Attachment #612710 - Flags: review?(dvander) → review+
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/projects/ionmonkey/rev/7ac0cbabb3d7
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.