Wasmtime GitHub notifications bot (Jul 02 2025 at 20:09): Wasmtime GitHub notifications bot (Jul 02 2025 at 20:09):fitzgen opened PR #11175 from fitzgen:issue-11162-no-hash-sets-in-drc to bytecodealliance:main:
This removes the explicit
HashSets used to represent the over-approximated-stack-roots and precise-stack-roots sets and replaces them with an intrusive, singly-linked list and a mark bit in the object headers respectively. The new list implementation also subsumes the old bump chunk that sat in front of the old over-approximated-stack-roots hash set.This shaves off about 25% of the time it takes to run the test case in https://github.com/bytecodealliance/wasmtime/issues/11141 for me locally.
This also ended up being a nice simplification of the DRC collector, which in turn allowed us to further simplify the
GcHeaptrait, since we no longer ever need to GC before passing GC refs into Wasm.Fixes https://github.com/bytecodealliance/wasmtime/issues/11162
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Jul 02 2025 at 20:09):fitzgen requested cfallin for a review on PR #11175.
Wasmtime GitHub notifications bot (Jul 02 2025 at 20:09):fitzgen requested wasmtime-compiler-reviewers for a review on PR #11175.
Wasmtime GitHub notifications bot (Jul 02 2025 at 20:09):fitzgen requested wasmtime-core-reviewers for a review on PR #11175.
Wasmtime GitHub notifications bot (Jul 02 2025 at 20:09):fitzgen requested alexcrichton for a review on PR #11175.
Wasmtime GitHub notifications bot (Jul 02 2025 at 20:09):fitzgen updated PR #11175.
Wasmtime GitHub notifications bot (Jul 02 2025 at 20:10):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Jul 02 2025 at 20:10):fitzgen created PR review comment:
This was such a silly bug but it made me feel like I was losing my mind :facepalm:
Wasmtime GitHub notifications bot (Jul 02 2025 at 20:31):cfallin created PR review comment:
s/table/list/
Wasmtime GitHub notifications bot (Jul 02 2025 at 20:31):cfallin submitted PR review:
This looks generally good to me -- nice speedups too! Just a few thoughts below.
Wasmtime GitHub notifications bot (Jul 02 2025 at 20:31):cfallin created PR review comment:
Do we want to defer this list-head update until after the code below sets up the object at the head? Otherwise there's a risk the intermediate state is visible -- perhaps fine (we're not a concurrent mutator) but if e.g. a trap occurs or we add some kind of observability mechanism it might present an edge case, especially as we delegate the setup to helpers. Basically it seems cleaner to me to keep the "making it live is the last store" pattern here.
Wasmtime GitHub notifications bot (Jul 02 2025 at 20:31):cfallin created PR review comment:
s/approximized/approximated/
Wasmtime GitHub notifications bot (Jul 02 2025 at 20:31):cfallin created PR review comment:
s/logivally/logically/
Wasmtime GitHub notifications bot (Jul 02 2025 at 20:31):cfallin created PR review comment:
s/table/list/ ?
Wasmtime GitHub notifications bot (Jul 02 2025 at 20:31):cfallin created PR review comment:
The function name here says
u26but the comment, and the function above as well, sayu27/27-bit-- they should match I think?
Wasmtime GitHub notifications bot (Jul 02 2025 at 20:31):cfallin created PR review comment:
I'm not sure I see why we load this value twice -- can we use
nextbelow when we patch the prev link?
Wasmtime GitHub notifications bot (Jul 02 2025 at 20:32):alexcrichton created PR review comment:
Could this method be removed entirely in favor taking a
bit: boolparameter inset_in_over_approximated_stack_rootsabove? That way the callers would inherit the comment of "no need to update the over approximated set" which I think would make more sense in the context of callers than here.
Wasmtime GitHub notifications bot (Jul 02 2025 at 20:32):alexcrichton submitted PR review:
Really like how this worked out, especially how it simplifies passing GC references into wasm
Wasmtime GitHub notifications bot (Jul 02 2025 at 20:32):alexcrichton created PR review comment:
Could this expand on what's being marked?
Wasmtime GitHub notifications bot (Jul 02 2025 at 21:55):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Jul 02 2025 at 21:55):fitzgen created PR review comment:
It shouldn't matter either way -- as you mention, there isn't any concurrency here -- but happy to swap things around.
Wasmtime GitHub notifications bot (Jul 02 2025 at 22:17):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Jul 02 2025 at 22:17):fitzgen created PR review comment:
VMGcRefdoesn't implementClonesince cloning aVMGcRefpotentially needs to run barriers, so we have dedicated methods to clone with those barriers if necessary. However, inside the GC implementation that isn't really a concern so we haveunchecked_copyfor these cases that don't need barriers. That said, when the GC ref is wrapped in anOptionlike it is here, it can be a bit of a mouthful, and we would have to donext.as_ref().map(|r| r.unchecked_copy()). So I figured I'd just grab it from the header twice and let LLVM clean things up.
Wasmtime GitHub notifications bot (Jul 02 2025 at 22:26):fitzgen updated PR #11175.
Wasmtime GitHub notifications bot (Jul 02 2025 at 22:30):fitzgen has enabled auto merge for PR #11175.
Wasmtime GitHub notifications bot (Jul 03 2025 at 01:05):github-actions[bot] commented on PR #11175:
Subscribe to Label Action
cc @fitzgen
<details>
This issue or pull request has been labeled: "wasmtime:api", "wasmtime:ref-types"Thus the following users have been cc'd because of the following labels:
- fitzgen: wasmtime:ref-types
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
Wasmtime GitHub notifications bot (Jul 03 2025 at 15:28):fitzgen updated PR #11175.
Wasmtime GitHub notifications bot (Jul 03 2025 at 15:29):fitzgen has enabled auto merge for PR #11175.
Wasmtime GitHub notifications bot (Jul 03 2025 at 17:07):fitzgen updated PR #11175.
Wasmtime GitHub notifications bot (Jul 03 2025 at 17:43):fitzgen merged PR #11175.
Last updated: Dec 06 2025 at 06:05 UTC