1651645 - Make IsPackedArray not depend on TI

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P2)

Description

[Jan de Mooij [:jandem]]

If TI is disabled we always return false from IsPackedArray. This hurts performance for things like new Set(arr) or new <TypedArray>(arr) because fast paths depend on IsPackedArray.

To fix this we can add a NON_PACKED flag to ObjectElements. We can then use that instead of the TI flag in IsPackedArray. Even with TI enabled this should be more precise because the flag is specific to the object instead of the whole group.

This will also make it possible to emit CacheIR for the IsPackedArray intrinsic.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Bug 1651645 part 1 - Inline setDenseElementMaybeConvertDouble into its sole caller. r?evilpie!

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Bug 1651645 part 2 - Simplify DeleteArrayElement, make some methods private. r?evilpie!

Depends on D82958

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Bug 1651645 part 3 - Use markDenseElementsNotPacked in setDenseElementHole. r?evilpie!

Depends on D82959

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Bug 1651645 part 4 - Use setDenseElementHole in removeDenseElementForSparseIndex. r?evilpie!

The goal is to have just one place where OBJECT_FLAG_NON_PACKED is set.

Depends on D82960

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1651645 part 5 - Assert initDenseElement/setDenseElement aren't called with the hole MagicValue. r?evilpie!

Depends on D82961

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1651645 part 6 - Fold ensureDenseInitializedLengthNoPackedCheck into ensureDenseInitializedLength and call from ensureDenseElements. r?evilpie!

The old code in ensureDenseElements marked non-packed before extending the
elements capacity. That's a problem because it can be the statically-allocated
EmptyObjectElements and we can't set the flag on that.

It's clearer to do the non-packed check in ensureDenseInitializedLength.

Depends on D82962

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1651645 part 7 - Clean up ensureDenseInitializedLength a bit. r?evilpie!

Re-organize the code. Add an early return instead of a big if-statement.
Change the initlen reference to a plain uint32_t.

Depends on D82963

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Bug 1651645 part 8 - Fold writeToIndexWouldMarkNotPacked into its sole caller. r?evilpie!

Also move it down a bit because we only need to do this when writing to an
index > initlength.

Depends on D82964

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Bug 1651645 part 9 - Add NON_PACKED flag to ObjectElements, use it in IsPackedArray. r?evilpie!

This way IsPackedArray does not depend on TI and can be used with Warp.

Adds MStoreHoleValueElement for JSOp::InitElemArray writing a hole value. This
instruction writes the hole value and sets the NON_PACKED flag. ICs don't
optimize writing holes.

We now also check in debug builds that if IsPackedArray returns true, the first
few elements aren't the magic hole value.

Depends on D82965

Comment 10

[Tom S. (please needinfo tschuster)]

Just to clarify. This doesn't actually remove the TI support for NON_PACKED. And MIsPackedArray + MCallOptimize both still depend on TI.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1b1c0c802a6b
part 1 - Inline setDenseElementMaybeConvertDouble into its sole caller. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/4a7666aa3cc6
part 2 - Simplify DeleteArrayElement, make some methods private. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/d34278f7a4a7
part 3 - Use markDenseElementsNotPacked in setDenseElementHole. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/c7a06239c860
part 4 - Use setDenseElementHole in removeDenseElementForSparseIndex. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/9a6e4bca8f6a
part 5 - Assert initDenseElement/setDenseElement aren't called with the hole MagicValue. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/726d3e7fd042
part 6 - Fold ensureDenseInitializedLengthNoPackedCheck into ensureDenseInitializedLength and call from ensureDenseElements. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/5c435dbe3a72
part 7 - Clean up ensureDenseInitializedLength a bit. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/783890691a76
part 8 - Fold writeToIndexWouldMarkNotPacked into its sole caller. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/f112125cab4d
part 9 - Add NON_PACKED flag to ObjectElements, use it in IsPackedArray. r=evilpie,anba

Comment 12

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/1b1c0c802a6b
https://hg.mozilla.org/mozilla-central/rev/4a7666aa3cc6
https://hg.mozilla.org/mozilla-central/rev/d34278f7a4a7
https://hg.mozilla.org/mozilla-central/rev/c7a06239c860
https://hg.mozilla.org/mozilla-central/rev/9a6e4bca8f6a
https://hg.mozilla.org/mozilla-central/rev/726d3e7fd042
https://hg.mozilla.org/mozilla-central/rev/5c435dbe3a72
https://hg.mozilla.org/mozilla-central/rev/783890691a76
https://hg.mozilla.org/mozilla-central/rev/f112125cab4d

Comment 13

[Geoff Lankow (:darktrojan)]

Would you have a look into this failure please? Your new assertion is triggering in this comm-central test.
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=309809202&repo=comm-central&lineNumber=3711

Comment 14

[Jan de Mooij [:jandem]]

(In reply to Geoff Lankow (:darktrojan) from comment #13)

Would you have a look into this failure please? Your new assertion is triggering in this comm-central test.
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=309809202&repo=comm-central&lineNumber=3711

Sorry about that. Fuzzing found the same bug and I have a patch for it that will hopefully land soon.