Closed
Bug 1344463
Opened 8 years ago
Closed 8 years ago
Emit JSOP_INITELEM for _DefineDataProperty calls in self-hosted code
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
evilpie
:
review+
till
:
review+
|
Details | Diff | Splinter Review |
The website in bug 1342797 spends multiple seconds under _DefineDataProperty calls from self-hosted code. I filed bug 1344177 to add a fast path to that intrinsic for dense elements, but it just occurred to me we could instead emit JSOP_INITELEM when the intrinsic is called with 3 arguments.
That should give us much better performance, also from Baseline because it uses the SetElem IC for JSOP_INITELEM. The second step then is to optimize JSOP_INITELEM the same way in Ion. This is pretty easy since the CacheIR conversion, because Baseline already uses the same IC for JSOP_INITELEM. So this lets us kill two birds with one stone: it will speed up certain object literals and make _DefineDataProperty faster.
The attached patch shaves off at least 6 seconds from the website in bug 1342797. It also improves the SixSpeed object-literal-ext-es6 test from 848 ms to 476 ms.
r? till for the self-hosting changes, evilpie for the rest.
Attachment #8843562 -
Flags: review?(till)
Attachment #8843562 -
Flags: review?(evilpies)
Assignee | ||
Comment 1•8 years ago
|
||
Some minor changes. I also removed --unboxed-arrays from a jit-test: unboxed arrays are not enabled anywhere and pretty broken atm (a lot of jit-tests fail if you use --unboxed-arrays).
Attachment #8843562 -
Attachment is obsolete: true
Attachment #8843562 -
Flags: review?(till)
Attachment #8843562 -
Flags: review?(evilpies)
Attachment #8843569 -
Flags: review?(till)
Attachment #8843569 -
Flags: review?(evilpies)
Assignee | ||
Comment 2•8 years ago
|
||
Btw, after this we could self-host Object.defineProperty and make it call the 3-argument _DefineDataProperty when possible. That should make defineProperty much faster.
I'm not sure how common it is to call defineProperty with the "default" descriptor though.
Comment 3•8 years ago
|
||
Comment on attachment 8843569 [details] [diff] [review]
Patch
Review of attachment 8843569 [details] [diff] [review]:
-----------------------------------------------------------------
\o/
r=me on the self-hosting and the bytecode emitter parts. I also looked at the jit parts and am pretty sure I can meaningfully say that they're ok, but it's better to have evilpie take a look at those, too.
::: js/src/frontend/BytecodeEmitter.cpp
@@ +9004,5 @@
> + return false;
> +
> + // This will leave the object on the stack instead of pushing |undefined|,
> + // but that's fine because the self-hosted code doesn't use the return
> + // value.
Would it make sense and be possible to assert this? Seems like we could assert that the next op isn't JSOP_POP? I mean, I don't really see how this could go wrong in self-hosted code, but who knows.
Attachment #8843569 -
Flags: review?(till) → review+
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #2)
> Btw, after this we could self-host Object.defineProperty and make it call
> the 3-argument _DefineDataProperty when possible. That should make
> defineProperty much faster.
And of course we could do the same thing for defining getters/setters (emit JSOP_INITELEM_{G,S}ETTER), non-enumerable properties (emit JSOP_INITHIDDENELEM), etc. So we could reuse a lot of our INITELEM infrastructure to optimize defineProperty.
Comment 5•8 years ago
|
||
Comment on attachment 8843569 [details] [diff] [review]
Patch
Review of attachment 8843569 [details] [diff] [review]:
-----------------------------------------------------------------
Nice! I am happy about the object-literal-ext-es6 win. Good idea to share the optimization here.
::: js/src/jit/CacheIR.cpp
@@ +2286,1 @@
> MOZ_ASSERT(IsPropertySetOp(JSOp(*pc)));
Can we add this assert to tryAttachProxy[Element].
Attachment #8843569 -
Flags: review?(evilpies) → review+
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Till Schneidereit [till] from comment #3)
> Would it make sense and be possible to assert this? Seems like we could
> assert that the next op isn't JSOP_POP?
Not easily, unfortunately, because we're still emitting the bytecode. I think we would have to pass a bool from emitStatement to emitCallOrNew. I think that's what arai's unused-rval patches are doing so it might be easier to add this assert after that lands, but I don't think it's strictly necessary.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c87ea81036b7
Optimize JSOP_INITELEM in Ion and emit it for 3-arguments _DefineDataProperty in self-hosted code. r=till,evilpie
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba5e3b0a356e
followup - Fix comment copy/paste issue. r=me
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c87ea81036b7
https://hg.mozilla.org/mozilla-central/rev/ba5e3b0a356e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•