Closed
Bug 1352006
Opened 7 years ago
Closed 7 years ago
Inline NewArrayIterator in Ion
Categories
(Core :: JavaScript Engine: JIT, enhancement)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: evilpie, Assigned: evilpie)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
20.93 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
NewArrayIterator is extremely hot in the es6 destructuring microbenchmark, so at least inlining the allocation is already a big win. I hope afterwards we can figure out how to completely eliminate the allocation.
Attachment #8852824 -
Flags: review?(jdemooij)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → evilpies
Comment 1•7 years ago
|
||
Comment on attachment 8852824 [details] [diff] [review] Inline NewArrayIterator in Ion Review of attachment 8852824 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/MIR.h @@ +3539,5 @@ > +class MNewArrayIterator : public MNullaryInstruction > +{ > + CompilerObject templateObj_; > + > + explicit MNewArrayIterator(JSObject* templateObj) Do not attach JSObject* in MIR instruction, use a MConstant to old the template object, as done in MNewObject and MNewArray. This is mandatory for implementing recover instructions, which is mandatory for removing the allocation of ArrayIterator when scalar replacement kicks-in.
Comment 2•7 years ago
|
||
Comment on attachment 8852824 [details] [diff] [review] Inline NewArrayIterator in Ion Review of attachment 8852824 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay. Looks great. If we want this to work with SR we should use MConstant as nbp suggested. ::: js/src/jit/CodeGenerator.cpp @@ +5645,5 @@ > + Register objReg = ToRegister(lir->output()); > + Register tempReg = ToRegister(lir->temp()); > + JSObject* templateObject = lir->mir()->templateObj(); > + > + // If we have a template object, we can inline iterator object creation. Nit: I think we can remove this comment because we always have a template ::: js/src/jit/MIR.h @@ +3543,5 @@ > + explicit MNewArrayIterator(JSObject* templateObj) > + : MNullaryInstruction(), > + templateObj_(templateObj) > + { > + setResultType(MIRType::Object); To make sure we have good type information, we should pass |constraints| and add: setResultTypeSet(MakeSingletonTypeSet(constraints, templateObj())); See MNewTypedArray for instance.
Attachment #8852824 -
Flags: review?(jdemooij) → review+
Pushed by evilpies@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b26588027af0 Inline NewArrayIterator in Ion. r=jandem
Assignee | ||
Comment 4•7 years ago
|
||
Thank you both for your review/feedback. Implementing recover instructions actually looks quite simple, so I will probably do that next and see what kind of improvements we get. I implemented your suggestion of using MConstant, but I wonder if it is really necessary, because we don't need the template object to create a new iterator.
Comment 5•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b26588027af0
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•