Closed
Bug 735448
Opened 12 years ago
Closed 12 years ago
IonMonkey: Fast path for JSOP_SETPROP adding case
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: dvander, Assigned: djvj)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
6.59 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
Needed for v8 - a fair number of addprop cases hit there. IC should be fine.
Reporter | ||
Comment 1•12 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•12 years ago
|
Assignee: general → kvijayan
Assignee | ||
Comment 2•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
Attachment #612296 -
Flags: review?(bhackett1024)
Assignee | ||
Updated•12 years ago
|
Attachment #612296 -
Flags: review?(dvander)
Reporter | ||
Comment 4•12 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 5•12 years ago
|
||
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•12 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•12 years ago
|
||
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•12 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•12 years ago
|
||
https://hg.mozilla.org/projects/ionmonkey/rev/7ac0cbabb3d7
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•