Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wasmtime-cranelift: Reorder table GC write barrier #8137

Merged
merged 1 commit into from
Mar 14, 2024

Conversation

jameysharp
Copy link
Contributor

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.

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.
@jameysharp jameysharp requested a review from a team as a code owner March 14, 2024 19:28
@jameysharp jameysharp requested review from alexcrichton and removed request for a team March 14, 2024 19:28
@alexcrichton alexcrichton added this pull request to the merge queue Mar 14, 2024
Merged via the queue into bytecodealliance:main with commit d1cd698 Mar 14, 2024
19 checks passed
@jameysharp jameysharp deleted the reorder-gc branch March 14, 2024 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants