Closed Bug 1344463 Opened 7 years ago Closed 7 years ago

Emit JSOP_INITELEM for _DefineDataProperty calls in self-hosted code

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

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)

Attached patch Patch (obsolete) — 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)
Blocks: six-speed
Attached patch PatchSplinter Review
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)
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 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+
(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 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+
(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
https://hg.mozilla.org/mozilla-central/rev/c87ea81036b7
https://hg.mozilla.org/mozilla-central/rev/ba5e3b0a356e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1345160
You need to log in before you can comment on or make changes to this bug.