Optimize ArrayPush in Warp
Categories
(Core :: JavaScript Engine: JIT, task, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox78 | --- | fixed |
People
(Reporter: evilpie, Assigned: evilpie)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
|
Assignee | |
Comment 1•4 months ago
|
||
Due to the previous guards emitted by CacheIR we know that this is always going to load a static prototype.
For example using MGetPrototypeOf would also try to handle dynamic prototypes.
I added a JIT assertion to make sure this assumption holds.
|
|
Assignee | |
Comment 2•4 months ago
|
||
I tried to compare BaselineCacheIRCompiler::emitArrayPush and CodeGenerator::emitArrayPush to make sure I didn't miss any implicit guards.
It seems like the Ion inline path is actually more limited though.
MArrayPush is typed in Ion to return Int32, that is however not really true, because array.length can overflow INT32_MAX. Ion currently
handles this via TI invalidation. For simplicity I decided to instead add a bailout before we even try to push. In the future we should look into
some approach that doesn't require invalidation to handle this.
Depends on D75925
|
||
Comment 3•4 months ago
|
||
One question, how would we handle multiple arguments for Array.push?
This used to be a perf hit on Speedometer until Bug 966743.
|
|
Assignee | |
Comment 4•4 months ago
|
||
I haven't really thought about it. Array.push with multiple arguments has been disabled in Ion since two years because of bug 1493903.
|
||
Comment 5•4 months ago
|
||
Several of the call IC ops read a variable number of arguments from the stack based on argc. In principle, we could do something similar for multiple-argument Array.push.
Alternatively, if it's only the array extension that can fail, and the pushes afterwards are infallible, then we could split ArrayPush into two ops: ArrayExtendN, which fallibly attempts to expand the array, and then a sequence of infallible ArrayPush. That might be easier for Ion, because the argument loads would happen explicitly in CacheIR instead of implicitly inside the op.
Pushed by evilpies@gmail.com: https://hg.mozilla.org/integration/autoland/rev/1704b3d9cf33 Transpile LoadProto. r=jandem https://hg.mozilla.org/integration/autoland/rev/6e5ab322dc4d Transpile ArrayPush. r=jandem
|
||
Comment 7•4 months ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/1704b3d9cf33
https://hg.mozilla.org/mozilla-central/rev/6e5ab322dc4d
Description
•