Emit JSOP_INITELEM for _DefineDataProperty calls in self-hosted code

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Posted 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)
(Assignee)

Updated

2 years ago
Blocks: 1177735
(Assignee)

Comment 1

2 years ago
Posted 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)
(Assignee)

Comment 2

2 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 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

2 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 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

2 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.

Comment 7

2 years ago
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

Comment 8

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba5e3b0a356e
followup - Fix comment copy/paste issue. r=me

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c87ea81036b7
https://hg.mozilla.org/mozilla-central/rev/ba5e3b0a356e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(Assignee)

Updated

2 years ago
Depends on: 1345160
You need to log in before you can comment on or make changes to this bug.