1947767 - Use fuses to simplify or replace the ForOfPIC code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Bug 1947767 part 1 - Ignore no-op property modifications in Watchtower. r?mgaudet!

This makes fuses a little more robust when code messes with prototypes without
actually changing the property's value.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Bug 1947767 part 2 - Pass PropertyInfo to watchPropertyChange. r?mgaudet!

The callers have it available so there's no need to look it up again.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Bug 1947767 part 3 - Simplify Watchtower::watchPropertyChange. r?mgaudet!

Renames PropertyChange to PropertyFlagsChange because this operation now only
covers the property flags. This is now only called when we're actually changing the
flags.

Adds separate watchPropertyModification calls for the case where defineProperty
is used to change the property's value (or both the value and the flags).

These changes help avoid unnecessary Watchtower calls.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Bug 1947767 part 6 - Fix bug with cross-realm `arguments[Symbol.iterator]`. r?mgaudet!

This fixes a pre-exsting bug: the iterator must be created in the realm of the
arguments object.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1947767 part 7 - Add separate OptimizeArrayIteratorPrototypeFuse. r?mgaudet!

OptimizeGetIteratorFuse guards against:

1. Changes to Array.prototype[@@iterator].
2. Changes to %ArrayIteratorPrototype%.

This patch adds a separate ArrayIteratorPrototypeFuse that covers just the second
case, and also changes OptimizeGetIteratorFuse to be dependent on this fuse.

Later patches will use this new fuse to replace ForOfPIC::Chain::tryOptimizeArrayIteratorNext.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1947767 part 8 - Replace uses of ForOfPIC with fuse checks. r?mgaudet!

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1947767 part 9 - Remove now unused ForOfPIC and PIC code. r?mgaudet!

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Bug 1947767 part 10 - Make some functions infallible. r?mgaudet!

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Bug 1947767 part 11 - Rely on fuses in CacheIR code. r?mgaudet!

This changes the CacheIR code to match the VM code changes from the previous patches.

Telemetry shows that the OptimizeGetIterator fuse is popped on less than 0.05% of
websites. With the first two patches in this stack this might drop even more.

The ArrayIteratorPrototype fuse is a subset of the OptimizeGetIterator fuse
so worst-case its numbers should be similar.

Comment 10

[Jan de Mooij [:jandem]]

This micro-benchmark improves from 52 ms to 45 ms (best of 10 runs, with --spectre-mitigations=off):

function f() {
    var arr = [1];
    var t = new Date;
    for (var i = 0; i < 1_000_000; i++) {
        var res = new Set(arr);
    }
    print(new Date - t);
    return res;
}
f();

With new Int8Array(arr) it improves from 54 ms to 49 ms.

Checking the fuse is faster and simpler than the ForOfPIC code.

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Bug 1947767 part 4 - Rename PropertyModification hook to PropertyValueChange. r?mgaudet!

This way we have PropertyFlagsChange and PropertyValueChange.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Bug 1947767 part 5 - Make Watchtower::watchPropertyValueChange infallible. r?mgaudet!

The only reason for this to be fallible is the Watchtower testing code, but if we
recover from failure there we can simplify some of the callers. In particular in
NativeObject.cpp this moves the watchPropertyValueChange calls closer to the
setSlot calls.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f9ea525db663
part 1 - Ignore no-op property modifications in Watchtower. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/fb467cb72422
part 2 - Pass PropertyInfo to watchPropertyChange. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/5584386646bc
part 3 - Simplify Watchtower::watchPropertyChange. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/3692e56920cc
part 4 - Rename PropertyModification hook to PropertyValueChange. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/5db74dbc123f
part 5 - Make Watchtower::watchPropertyValueChange infallible. r=mgaudet

Comment 14

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/f9ea525db663
https://hg.mozilla.org/mozilla-central/rev/fb467cb72422
https://hg.mozilla.org/mozilla-central/rev/5584386646bc
https://hg.mozilla.org/mozilla-central/rev/3692e56920cc
https://hg.mozilla.org/mozilla-central/rev/5db74dbc123f

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c617c71a16ae
part 6 - Fix bug with cross-realm `arguments[Symbol.iterator]`. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/ffc35ef730e4
part 7 - Add separate OptimizeArrayIteratorPrototypeFuse. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/23cb8e597ba8
part 8 - Replace uses of ForOfPIC with fuse checks. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/4c817911289e
part 9 - Remove now unused ForOfPIC and PIC code. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/f5cf04c3b582
part 10 - Make some functions infallible. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/2a0b3de2486f
part 11 - Rely on fuses in CacheIR code. r=mgaudet

Comment 16

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/c617c71a16ae
https://hg.mozilla.org/mozilla-central/rev/ffc35ef730e4
https://hg.mozilla.org/mozilla-central/rev/23cb8e597ba8
https://hg.mozilla.org/mozilla-central/rev/4c817911289e
https://hg.mozilla.org/mozilla-central/rev/f5cf04c3b582
https://hg.mozilla.org/mozilla-central/rev/2a0b3de2486f

Comment 17

[Mayank Bansal]

(In reply to Pulsebot from comment #15)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c617c71a16ae
part 6 - Fix bug with cross-realm arguments[Symbol.iterator]. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/ffc35ef730e4
part 7 - Add separate OptimizeArrayIteratorPrototypeFuse. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/23cb8e597ba8
part 8 - Replace uses of ForOfPIC with fuse checks. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/4c817911289e
part 9 - Remove now unused ForOfPIC and PIC code. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/f5cf04c3b582
part 10 - Make some functions infallible. r=mgaudet
https://hg.mozilla.org/integration/autoland/rev/2a0b3de2486f
part 11 - Rely on fuses in CacheIR code. r=mgaudet

This lead to a 512 byte increase in BAse contetn JS opt fission on MacOS and Linux.