Last Comment Bug 735448 - IonMonkey: Fast path for JSOP_SETPROP adding case
: IonMonkey: Fast path for JSOP_SETPROP adding case
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Kannan Vijayan [:djvj]
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: IonSpeed
  Show dependency treegraph
Reported: 2012-03-13 14:31 PDT by David Anderson [:dvander]
Modified: 2012-04-05 17:54 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch v1 (6.70 KB, text/plain)
2012-04-04 12:28 PDT, Kannan Vijayan [:djvj]
no flags Details
Patch v1 again. (6.70 KB, patch)
2012-04-04 12:29 PDT, Kannan Vijayan [:djvj]
bhackett1024: review+
dvander: review+
Details | Diff | Splinter Review
Patch v2 (6.59 KB, patch)
2012-04-05 15:34 PDT, Kannan Vijayan [:djvj]
dvander: review+
Details | Diff | Splinter Review

Description David Anderson [:dvander] 2012-03-13 14:31:22 PDT
Needed for v8 - a fair number of addprop cases hit there. IC should be fine.
Comment 1 David Anderson [:dvander] 2012-03-13 15:10:19 PDT
// 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;

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).
Comment 2 Kannan Vijayan [:djvj] 2012-04-04 12:28:15 PDT
Created attachment 612295 [details]
Patch v1

Very rough description:


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.
Comment 3 Kannan Vijayan [:djvj] 2012-04-04 12:29:40 PDT
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.
Comment 4 David Anderson [:dvander] 2012-04-04 20:09:42 PDT
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.
Comment 5 Brian Hackett (:bhackett) 2012-04-05 08:46:38 PDT
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.
Comment 6 Kannan Vijayan [:djvj] 2012-04-05 14:40:14 PDT
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.
Comment 7 Kannan Vijayan [:djvj] 2012-04-05 15:34:13 PDT
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.
Comment 8 David Anderson [:dvander] 2012-04-05 16:34:13 PDT
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.
Comment 9 Kannan Vijayan [:djvj] 2012-04-05 17:54:21 PDT

Note You need to log in before you can comment on or make changes to this bug.