Closed Bug 1651645 Opened 1 year ago Closed 1 year ago

Make IsPackedArray not depend on TI

Categories

(Core :: JavaScript Engine, task, P2)

task

Tracking

()

RESOLVED FIXED
mozilla80
Tracking Status
firefox80 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(9 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

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.

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

Depends on D82960

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

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

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

Depends on D82964

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

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

Pushed by jdemooij@mozilla.com:
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
Blocks: 1652685

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

Flags: needinfo?(jdemooij)

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

Flags: needinfo?(jdemooij)
You need to log in before you can comment on or make changes to this bug.