Stream: git-wasmtime

Topic: wasmtime / PR #8137 wasmtime-cranelift: Reorder table GC ...

Wasmtime GitHub notifications bot (Mar 14 2024 at 19:28):

jameysharp opened PR #8137 from jameysharp:reorder-gc to bytecodealliance:main:

We have some flexibility in what order we do steps in the GC write barrier for table.set on an externref table. We must:

• bounds-check the table index before incrementing the new refcount
• load the old ref before storing the new ref
• increment the new refcount before decrementing the old refcount
• store the new ref before calling drop_externref, which might GC

But that leaves several valid orders for these steps.

This commit moves the load and store as early as possible, so they're right after bounds-checking.

I believe this should have no effect on the correctness of this write barrier. It enables an upcoming change I'm working on to let table bounds-checks trap in the memory access instruction, rather than during address computation. The invariants here are tricky though so I wanted to get this change reviewed in isolation first.

For my purposes, I only need to move the load earlier; the store could stay where it was, right after incrementing the new refcount. However, moving both memory accesses shortens the range where the table entry address needs to be in a register, without lengthening the live range of any other value. Moving the load does lengthen the live range of the old ref, but as long as we move the store with it, register pressure should be unchanged.

Moving the store before the corresponding refcount increment should be safe for two reasons. One is that there's no opportunity for GC to happen in between. The other is that the new value should already have a non-zero refcount.

Wasmtime GitHub notifications bot (Mar 14 2024 at 19:28):

jameysharp requested alexcrichton for a review on PR #8137.

Wasmtime GitHub notifications bot (Mar 14 2024 at 19:28):

jameysharp requested wasmtime-core-reviewers for a review on PR #8137.

Wasmtime GitHub notifications bot (Mar 14 2024 at 19:48):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 14 2024 at 20:17):

alexcrichton merged PR #8137.

Last updated: Nov 22 2024 at 16:03 UTC