821816 - Add separate INITELEM bytecode for array initializers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

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.

Comment 1

[Brian Hackett [Laid off!]]

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.

Comment 2

[Jan de Mooij [:jandem]]

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

Comment 3

[Jan de Mooij [:jandem]]

Attachment: 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.

Comment 4

[Jan de Mooij [:jandem]]

Try run not finished yet but looks green so far:

https://tbpl.mozilla.org/?tree=Try&rev=64b81c9bd128

Comment 5

[Brian Hackett [Laid off!]]

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 (!).

Comment 6

[Jan de Mooij [:jandem]]

Pushed with comment addressed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4170cba8bf47

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4170cba8bf47