1642412 - Optimize table.get/set for anyref tables

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement, P3)

Description

[Ryan Hunt [:rhunt]]

Our current implementation uses a call to Instance::tableGet/tableSet.

I observed this adding overhead with my interface-types prototype where a rust module using reference-types via wasm-bindgen would frequently use these instructions to manage the anyref heap.

I believe for anyref (not funcref) this should be as simple as a bounds check and load/store from/to array. Unsure if we need a barrier, would need to double check that.

Comment 1

[Ryan Hunt [:rhunt]]

Attachment: Bug 1642412 - wasm: Optimize baseline table.size/get/set. r?lth

This commit adds a fast path for table.get/set when the table
representation is anyref, and an unconditional fast path for
table.set.

TableTls is changed so that it contains a pointer to elements
when the representation is anyref, not just when it's storing
functions.

Comment 2

[Ryan Hunt [:rhunt]]

Attachment: Bug 1642412 - wasm: Optimize ion table.get/set/size. r?lth

This commit adds ion fast paths for table.get/set when the table
representation is anyref. An unconditional fast path is added for
table.size.

Depends on D127311

Comment 3

[Ryan Hunt [:rhunt]]

Posting my WIP patches. They pass tests and should be very close to review, but I need some more time.

Comment 4

[Ryan Hunt [:rhunt]]

Attachment: Bug 1642412 - wasm: Remove unnecessary code from Instance for inline table methods. r?lth

This commit removes support for anyref in Instance::tableGet/tableSet,
making it clear that these functions are only used for function tables.
Instance::tableSize is removed as no longer needed.

Depends on D127312

Comment 5

[Ryan Hunt [:rhunt]]

Now ready for review.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/40f861f62a0f
wasm: Optimize baseline table.size/get/set. r=lth
https://hg.mozilla.org/integration/autoland/rev/90fa382b5a20
wasm: Optimize ion table.get/set/size. r=lth
https://hg.mozilla.org/integration/autoland/rev/67ae30727ed8
wasm: Remove unnecessary code from Instance for inline table methods. r=lth

Comment 7

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/40f861f62a0f
https://hg.mozilla.org/mozilla-central/rev/90fa382b5a20
https://hg.mozilla.org/mozilla-central/rev/67ae30727ed8

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for causing bug 1748602:
https://hg.mozilla.org/mozilla-central/rev/349728105c40

Comment 9

[Ryan Hunt [:rhunt]]

Attachment: Bug 1642412 - wasm: Rollup of inline table.get/set for anyref. r?lth

Rollup of the previous inline table.get/set patches.

Comment 10

[Ryan Hunt [:rhunt]]

Attachment: Bug 1642412 - wasm: Add test for post-write barrier issue. r?jseward

Add test for issue with a post-write barrier that doesn't remove
store buffer entries when used on a table that may grow.

Depends on D141024

Comment 11

[Ryan Hunt [:rhunt]]

Attachment: Bug 1642412 - wasm: Refactor the baseline post-write barrier out of the barriered store function. r?jseward

The barriered store function emits the post-write barrier directly, but calls
out to a function for the pre-write barrier. We should factor the post-write
barrier out so that we can use a different post-write barrier implementation
depending on requirements more easily.

Depends on D141025

Comment 12

[Ryan Hunt [:rhunt]]

Attachment: Bug 1642412 - wasm: Implement a 'precise' post-write barrier in baseline. r?jseward

Add a post-write barrier which will remove a previous store buffer entry if the
new value stored in the field would not require it. This requires the previous
value to be loaded before the store and propagated to the post-write barrier
call.

We only require a precise post-write barrier for storing into tables, as they
are implemented using a GCVector<HeapPtr<..>> which requires the invariant
that each write of null/tenured when the previous value was nursery will
remove the existing store buffer entry.

Depends on D141026

Comment 13

[Ryan Hunt [:rhunt]]

Attachment: Bug 1642412 - wasm: Switch ion to use the precise post-write barrier. r?jseward

Ion has an easier time switching to the precise post-write barrier
as it's already unconditionally doing a call (postBarrierFiltering),
and so there's little extra cost in switching that call to be the
precise post-write barrier everywhere.

We cannot remove the postBarrierFiltering builtin because it is
still used by cranelift.

Depends on D141027

Comment 14

[Ryan Hunt [:rhunt]]

Attachment: Bug 1642412 - wasm: Don't read a call site line in operations that AsmJS won't emit one for. r?lth

AsmJS will emit a call site line number for call, call indirect, and math builtins.
We currently read from the call site line number array for more operations than
these. This is normally fine, as long as the operations that try the read are
never emitted from AsmJS. If that is not the case, we will get out of sync and
can read outside of the array. This caused an issue when I added a use of
readCallSiteOrBytecode() in TeeGlobal, copying from SetGlobal. This commit
fixes this.

Depends on D141029

Comment 15

[Ryan Hunt [:rhunt]]

I've returned to this work and have posted new patches that fix an issue with barriers so that this can land again.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/25adf505426d
wasm: Rollup of inline table.get/set for anyref. r=lth
https://hg.mozilla.org/integration/autoland/rev/3cdc18a4ee38
wasm: Add test for post-write barrier issue. r=jseward
https://hg.mozilla.org/integration/autoland/rev/d190ffcf25bf
wasm: Refactor the baseline post-write barrier out of the barriered store function. r=jseward
https://hg.mozilla.org/integration/autoland/rev/a41351c7413b
wasm: Implement a 'precise' post-write barrier in baseline. r=jseward
https://hg.mozilla.org/integration/autoland/rev/285174ef42f1
wasm: Switch ion to use the precise post-write barrier. r=jseward
https://hg.mozilla.org/integration/autoland/rev/0d1d9fa72512
wasm: Don't read a call site line in operations that AsmJS won't emit one for. r=lth

Comment 17

[Sandor Molnar[:smolnar]]

Backed out 6 changesets (bug 1642412) for causing spider-monkey failures in wasm/ref-types/externref-global-prebarrier.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/9e51620948d8e9f1daa197835367c8332eb86fa5

Push with failures

Failure log

TEST-PASS | js/src/jit-test/tests/wasm/ref-types/externref.js | Success (code 0, args "--ion-eager --ion-offthread-compile=off --more-compartments") [0.3 s]
[task 2022-03-22T20:35:59.554Z] /builds/worker/checkouts/gecko/js/src/jit-test/lib/wasm.js:417:11 Error: no plausible stacks found, observed: /!>/0,!>//0,!>/<,0,!>/GC postbarrier,0,!>/<,0,!>/0,!>/!>/
[task 2022-03-22T20:35:59.555Z] /builds/worker/checkouts/gecko/js/src/jit-test/lib/wasm.js:417:11 Expected one of:
[task 2022-03-22T20:35:59.555Z] /builds/worker/checkouts/gecko/js/src/jit-test/lib/wasm.js:417:11 /!>/0,!>//0,!>/<,0,!>/filtering GC postbarrier,0,!>/<,0,!>/0,!>/!>/
[task 2022-03-22T20:35:59.555Z] /builds/worker/checkouts/gecko/js/src/jit-test/lib/wasm.js:417:11 /!>/0,!>//0,!>/!>/
[task 2022-03-22T20:35:59.555Z] Stack:
[task 2022-03-22T20:35:59.555Z]   WasmHelpers.assertEqPreciseStacks@/builds/worker/checkouts/gecko/js/src/jit-test/lib/wasm.js:417:11
[task 2022-03-22T20:35:59.555Z]   @/builds/worker/checkouts/gecko/js/src/jit-test/tests/wasm/ref-types/externref-global-prebarrier.js:55:22
[task 2022-03-22T20:35:59.555Z] Exit code: 3
[task 2022-03-22T20:35:59.555Z] FAIL - wasm/ref-types/externref-global-prebarrier.js
[task 2022-03-22T20:35:59.555Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/wasm/ref-types/externref-global-prebarrier.js | /builds/worker/checkouts/gecko/js/src/jit-test/lib/wasm.js:417:11 Error: no plausible stacks found, observed: /!>/0,!>//0,!>/<,0,!>/GC postbarrier,0,!>/<,0,!>/0,!>/!>/ (code 3, args "--no-blinterp --no-baseline --no-ion --more-compartments") [1.1 s]
[task 2022-03-22T20:35:59.555Z] INFO exit-status     : 3

Comment 18

[Ryan Hunt [:rhunt]]

Naming of a builtin changed, this stack trace test needed to be updated. Don't know why this was only encountered in arm32.

[1] https://treeherder.mozilla.org/jobs?repo=try&revision=a577209849cc61a7d5151a288317e24e79df56e1

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ce799cb566ac
wasm: Rollup of inline table.get/set for anyref. r=lth
https://hg.mozilla.org/integration/autoland/rev/aaf8c5eef48e
wasm: Add test for post-write barrier issue. r=jseward
https://hg.mozilla.org/integration/autoland/rev/eabb888a0851
wasm: Refactor the baseline post-write barrier out of the barriered store function. r=jseward
https://hg.mozilla.org/integration/autoland/rev/89db2f360b8f
wasm: Implement a 'precise' post-write barrier in baseline. r=jseward
https://hg.mozilla.org/integration/autoland/rev/00b99ec19d1e
wasm: Switch ion to use the precise post-write barrier. r=jseward
https://hg.mozilla.org/integration/autoland/rev/82ac83afeaea
wasm: Don't read a call site line in operations that AsmJS won't emit one for. r=lth

Comment 20

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/ce799cb566ac
https://hg.mozilla.org/mozilla-central/rev/aaf8c5eef48e
https://hg.mozilla.org/mozilla-central/rev/eabb888a0851
https://hg.mozilla.org/mozilla-central/rev/89db2f360b8f
https://hg.mozilla.org/mozilla-central/rev/00b99ec19d1e
https://hg.mozilla.org/mozilla-central/rev/82ac83afeaea