1791162 - Use bailouts for jit::SetDenseElement and jit::ArrayPush

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[André Bargull [:anba]]

• Using a bailout for ArrayPush makes the alias-set for MArrayPush a bit more correct. It's currently Store(ObjectFields | Elements), but it can actually also modify slots when setOrExtendDenseElements fails and we execute DefineDataElement.
• Using a bailout for StoreElementHole allows to re-add the alias-set override which was removed in bug 1607443.

And finally as an additional improvement, bailouts also makes it easier to add mightAlias overrides for some guard operations, which allows to move guard instructions before loops. For example:

var arr = [];
for (var i = 0; i < N; ++i) {
  arr.push(i);
}

Currently executes the following guards in the loop body:

13 constant object 2a9dfe23c088 (Array)
14 constant function push at 2a9dfe2612a8
...
16 guardshape newarrayobject5:Object
17 guardshape constant13:Object
18 slots guardshape17:Object
51 loaddynamicslotandunbox slots18:Slots (slot 6)
21 guardspecificfunction loaddynamicslotandunbox51:Object constant14:Object
22 objectstaticproto guardshape16:Object
23 guardshape objectstaticproto22:Object
24 objectstaticproto guardshape23:Object
25 guardshape objectstaticproto24:Object

Adding appropriate mightAlias overrides makes it possible to move these guards before the loop body.

Comment 1

[André Bargull [:anba]]

Attachment: Bug 1791162 - Part 1: Bail out from SetDenseElement when we can't add a dense element. r=jandem!

Use a bailout instead of calling DefineDataElement. This won't lead to
performance issues, because calling DefineDataElement will add an indexed
property and indexed properties will cause shape guards to fail the next time
this operation is called.

That means we can also change the function to an ABI call, which is slightly
faster than a VM call. We need additional temp registers for the ABI call,
therefore change the Spectre-temp to a normal, unconditional temp register.

Comment 2

[André Bargull [:anba]]

Attachment: Bug 1791162 - Part 2: Bail out from ArrayPush when we can't add a dense element. r=jandem!

Similar to part 1, also prefer a bailout for ArrayPush.

Depends on D157548

Comment 3

[André Bargull [:anba]]

Attachment: Bug 1791162 - Part 3: Re-add the alias-set to MStoreElementHole. r=jandem!

Part 1 allows to safely reintroduce the alias-set for MStoreElementHole,
which was removed in bug 1607443.

Depends on D157549

Comment 4

[André Bargull [:anba]]

Attachment: Bug 1791162 - Part 4: Handle StoreElementHole and ArrayPush in some mightAlias methods. r=jandem!

This allows to hoist shape guards in situations like:

var arr = [];
for (var i = 0; i < N; ++i) {
  arr.push(i);
}

Before this patch, we executed the following guards in the loop body:

13 constant object 2a9dfe23c088 (Array)
14 constant function push at 2a9dfe2612a8
...
16 guardshape newarrayobject5:Object
17 guardshape constant13:Object
18 slots guardshape17:Object
51 loaddynamicslotandunbox slots18:Slots (slot 6)
21 guardspecificfunction loaddynamicslotandunbox51:Object constant14:Object
22 objectstaticproto guardshape16:Object
23 guardshape objectstaticproto22:Object
24 objectstaticproto guardshape23:Object
25 guardshape objectstaticproto24:Object

After this patch all guards are moved before the loop body.

Similar improvements for this code snippet:

var arr = [];
for (var i = 0; i < N; ++i) {
  arr[i] = i;
}

Where these guards are now moved before the loop body:

7 guardshape newarrayobject5:Object
8 objectstaticproto guardshape7:Object
9 guardshape objectstaticproto8:Object
10 objectstaticproto guardshape9:Object
11 guardshape objectstaticproto10:Object

Depends on D157550

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b82ed0bc340
Part 1: Bail out from SetDenseElement when we can't add a dense element. r=jandem
https://hg.mozilla.org/integration/autoland/rev/ca1acbc1f294
Part 2: Bail out from ArrayPush when we can't add a dense element. r=jandem
https://hg.mozilla.org/integration/autoland/rev/241609720395
Part 3: Re-add the alias-set to MStoreElementHole. r=jandem
https://hg.mozilla.org/integration/autoland/rev/51423d631a9a
Part 4: Handle StoreElementHole and ArrayPush in some mightAlias methods. r=jandem

Comment 6

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/4b82ed0bc340
https://hg.mozilla.org/mozilla-central/rev/ca1acbc1f294
https://hg.mozilla.org/mozilla-central/rev/241609720395
https://hg.mozilla.org/mozilla-central/rev/51423d631a9a

Comment 7

[Acasandrei Beatrice (needinfo me)]

Perfherder has detected a talos performance improvement from push 51423d631a9a. Possible source of the improvement is this bug, Bug 1790990 or Bug 1790989. Which one of these bugs seem like the most plausible to have caused this improvement?

== Change summary for alert #35435 (as of Wed, 21 Sep 2022 17:56:54 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
5%	kraken	macosx1015-64-shippable-qr	e10s fission stylo webrender	1,435.76 -> 1,366.65
5%	kraken	macosx1015-64-shippable-qr	e10s fission stylo webrender	1,432.94 -> 1,364.35
5%	kraken	windows10-64-shippable-qr	e10s fission stylo webrender	1,258.41 -> 1,199.58

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=35435

Comment 8

[André Bargull [:anba]]

(In reply to Acasandrei Beatrice (needinfo me) from comment #7)

Possible source of the improvement is this bug, Bug 1790990 or Bug 1790989. Which one of these bugs seem like the most plausible to have caused this improvement?

This bug is the most likeliest to have caused the improvement.