Closed
Bug 1000344
Opened 10 years ago
Closed 10 years ago
Optimize polymorphic property gets/sets accessing the same slot
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: jandem, Assigned: jandem)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
21.73 KB,
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
Octane-Gameboy has a ton of GetPropertyPolymorphic instructions with two different object shapes but in many cases the property is in the same slot. It seems like in this case we don't want GetPropertyPolymorphic/SetPropertyPolymorphic but a (new) MGuardShapePolymorphic instruction that guards on multiple shapes followed by a LoadSlot/StoreSlot. Then we can hoist/coalesce the shape guards separately and also emit smaller code.
Assignee | ||
Comment 1•10 years ago
|
||
This is a small win on Octane-Gameboy; most of the property accesses there can be optimized this way. The patch also gets rid of some unnecessary branches for GetPropertyPolymorphic/SetPropertyPolymorphic.
Attachment #8411810 -
Flags: review?(bhackett1024)
Comment 2•10 years ago
|
||
Comment on attachment 8411810 [details] [diff] [review] Patch Review of attachment 8411810 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/IonBuilder.cpp @@ +8870,5 @@ > + for (size_t i = 0; i < shapes.length(); i++) { > + Shape *objShape = shapes[i]; > + Shape *shape = objShape->searchLinear(id); > + MOZ_ASSERT(shape); > + if (!propShapes.append(shape)) This looks like it can be infallibleAppend.
Attachment #8411810 -
Flags: review?(bhackett1024) → review+
Assignee | ||
Comment 3•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1e4a93e5b6c
Comment 4•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d1e4a93e5b6c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment 5•10 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #1) > This is a small win on Octane-Gameboy 17% win according to AWFY on all but the ARMs. That places it almost the same as V8 on Mac x86! Well done!
Comment 6•10 years ago
|
||
Comment on attachment 8411810 [details] [diff] [review] Patch Review of attachment 8411810 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/CodeGenerator.cpp @@ +1513,5 @@ > + return false; > + } > + } else { > + masm.branchPtr(Assembler::NotEqual, scratch, ImmGCPtr(mir->objShape(i)), &next); > + } I thinks this could be handled a bit more elegant: > for (size_t i = 0; i < mir->numShapes(); i++) { > Label next; > masm.branchPtr(Assembler::NotEqual, scratch, ImmGCPtr(mir->objShape(i)), &next); > > ... > > if (i != mir->numShapes() - 1) { > masm.jump(&done); > masm.bind(&next); > } else { > // Processing last shape, so there is no next. Bail in that case. > if (!bailoutFrom(&next, ins->snapshot())) > return false; > } > } @@ -1525,5 @@ > } > > - // Bailout if no shape matches. > - if (!bailout(ins->snapshot())) > - return false; When "mir->numShapes() == 0" we should bail. In the patched version we don't do that anymore! Is there a reason we don't need to bailout in that case? Or is this oversight? @@ -1586,5 @@ > } > > - // Bailout if no shape matches. > - if (!bailout(ins->snapshot())) > - return false; Same here? @@ +1753,5 @@ > + } else { > + masm.branchPtr(Assembler::Equal, temp, ImmGCPtr(shape), &done); > + } > + } > + Same here?
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Hannes Verschore [:h4writer] from comment #6) > I thinks this could be handled a bit more elegant: Yeah, we could use bailoutFrom but it's pretty straight-forward either way. > When "mir->numShapes() == 0" we should bail. In the patched version we don't > do that anymore! Is there a reason we don't need to bailout in that case? Or > is this oversight? It makes no sense for numShapes to be <= 1, we assert it's > 1.
You need to log in
before you can comment on or make changes to this bug.
Description
•