Add separate INITELEM bytecode for array initializers

RESOLVED FIXED in mozilla20

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
JSOP_INITELEM is emitted for both array initializers and object initializers with numeric properties, like {1:x}

Ideally we'd emit a separate op for the array initializer case (and store the index as part of the op), so that the JITs can assume the object is a dense array and emit the store directly.
FWIW you can determine whether the initializer is an array by keeping track of which NEW{INIT,OBJECT,ARRAY} opcode each INITELEM corresponds to, but it is kind of ugly to have to do that separately in every JIT.
(Assignee)

Comment 2

5 years ago
(In reply to Brian Hackett (:bhackett) from comment #1)
> FWIW you can determine whether the initializer is an array by keeping track
> of which NEW{INIT,OBJECT,ARRAY} opcode each INITELEM corresponds to, but it
> is kind of ugly to have to do that separately in every JIT.

There's another problem: if one of the elements is a ternary or something, we no longer know the element index. Storing the index as part of the op avoids that.
(Assignee)

Comment 3

5 years ago
Created attachment 692572 [details] [diff] [review]
Patch

Adds a new INITELEM_DENSE op that's used for array initializers. The index is part of the op, so bytecode for [a, b, c] now looks like this:

00000:  newarray 3
00004:  getgname "a"
00009:  initelem_dense 0
00013:  getgname "b"
00018:  initelem_dense 1
00022:  getgname "c"
00027:  initelem_dense 2
00031:  endinit

This is nice for the JITs because we can inline this op without any VM calls or extra analysis.

INITELEM is still used for object initializers with numeric keys, but since that's rare we can always stub it.

INITELEM_INC is unchanged, it's used when the array literal contains a JSOP_SPREAD like [1, ...a, 3]. It increments the index stored on the stack after each element, because we don't know it statically.
Attachment #692572 - Flags: review?(bhackett1024)
(Assignee)

Comment 4

5 years ago
Try run not finished yet but looks green so far:

https://tbpl.mozilla.org/?tree=Try&rev=64b81c9bd128
Comment on attachment 692572 [details] [diff] [review]
Patch

Review of attachment 692572 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good but can you rename JSOP_INITELEM_DENSE to JSOP_INITELEM_ARRAY, and fixup comments mentioning 'dense'?  Dense arrays are an implementation detail separate from the structure of the bytecode, and will eventually be going away.  The fact that the array is dense is not guaranteed by the bytecode either, and, indeed, not too long ago we couldn't guarantee it would be dense, as a GC in the middle could slowify it (!).
Attachment #692572 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/mozilla-central/rev/4170cba8bf47
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
(Assignee)

Updated

5 years ago
Blocks: 822208
You need to log in before you can comment on or make changes to this bug.