Stream: git-wasmtime

Topic: wasmtime / PR #11411 Minimize lazy allocation of the GC s...

Wasmtime GitHub notifications bot (Aug 09 2025 at 01:41):

alexcrichton opened PR #11411 from alexcrichton:less-lazy-gc-store to bytecodealliance:main:

This commit is an effort to minimize the number of entrypoints which might lazily allocate a GC store. The is currently done through StoreOpaque::gc_store_mut but this method is very commonly used meaning that there are many many places to audit for lazily allocating a GC store. The reason that this needs an audit is that lazy allocation is an async operation right now that must be on a fiber and is something I'm looking to fix as part of #11262.

This commit performs a few refactorings to achieve this:

• gc_store_mut is renamed to ensure_gc_store. This is intended to be an async function in the future and clearly demarcates where lazy allocation of a GC store is occurring.

• require_gc_store{,_mut} is now added which is a pure accessor of the GC store with no lazy allocation. Most locations previously using gc_store_mut are updated to use this instead.

Documentation is added to store methods to clearly indicate which ones are allocating and which ones should only be called in a context where allocation should already have happened.

<!--
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.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Aug 09 2025 at 01:41):

alexcrichton requested pchickey for a review on PR #11411.

Wasmtime GitHub notifications bot (Aug 09 2025 at 01:41):

alexcrichton requested wasmtime-core-reviewers for a review on PR #11411.

Wasmtime GitHub notifications bot (Aug 09 2025 at 03:57):

github-actions[bot] commented on PR #11411:

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 (Aug 11 2025 at 14:11):

alexcrichton commented on PR #11411:

@fitzgen ok so turns out there were more places than expected that really do lazily allocate a GC store, notably dealing with i31ref and null references. Best I can think of for handling this is to either:

1. Support allocating a GC store without a GC heap so things like write_gc_ref if the reference is i31ref or null. We'd then have ensure_gc_store (sync, just allocates some data structures) and ensure_gc_heap (async, allocates a heap) where both return &mut GcStore. Just ensure_gc_store wouldn't allocate a heap so we'd still skip it with i31ref and null references.
2. Change callers of write_gc_ref to internally check for null references and i31ref an directly perform writes. This would basically require that all GC implementations don't need barriers for i31ref and null and would require more runtime checking in the host when performing those writes.

Do you have other ideas and/or a preference about how might be best to resolve this?

Wasmtime GitHub notifications bot (Aug 11 2025 at 17:11):

fitzgen commented on PR #11411:

Change callers of write_gc_ref to internally check for null references and i31ref an directly perform writes. This would basically require that all GC implementations don't need barriers for i31ref and null and would require more runtime checking in the host when performing those writes.

We already bake in the assumption that i31refs don't need barriers in various places, because they aren't actually GC heap objects, so they aren't managed by the GC runtime. Same for null pointers. Although writing an i31ref or null reference into a field of a non-null object does need to call GC barriers.

Anyways, we have some of these checks in the GcStore::* barrier methods, and we could pull them out to being StoreOpaque::* barrier methods perhaps.

Also, if we have a non-i31ref, non-null reference, then we must have already allocated the GC store. And if we don't have one of those, then we don't need barriers, so we don't need to allocate a GcStore lazily to run barriers.

So I think that the StoreOpaque::* barrier methods shouldn't ever need to lazily allocate a GcStore. Basically something like

impl StoreOpaque {
    pub(crate) fn clone_gc_ref(&mut self, gc_ref: &GcRef) -> GcRef {
        if gc_ref.is_i31() {
            gc_ref.copy_i31()
        } else {
            self.gc_store
                .as_mut()
                .expect("non-null, non-i31 gc ref means we must have a gc store")
                .clone_gc_ref(gc_ref)
    }
}

Wasmtime GitHub notifications bot (Aug 11 2025 at 17:20):

pchickey requested fitzgen for a review on PR #11411.

Wasmtime GitHub notifications bot (Aug 11 2025 at 17:30):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Aug 11 2025 at 17:30):

fitzgen created PR review comment:

Reminder to finish this doc comment

Wasmtime GitHub notifications bot (Aug 11 2025 at 17:30):

fitzgen created PR review comment:

It isn't clear to me why these can require rather than ensure? Wouldn't we need to ensure in Global::new or somewhere in order for this require to be okay?

Wasmtime GitHub notifications bot (Aug 11 2025 at 17:30):

fitzgen created PR review comment:

FWIW, if these lazy-allocation sites in initialize_tables are problematic, we could do require_gc_store here and require that callers check and initialize the gc heap if necessary. For instances, they should already be allocated by instantiation. For host tables, we would need to add a new check, but that should be straightforward enough to add.

Wasmtime GitHub notifications bot (Aug 11 2025 at 17:30):

fitzgen created PR review comment:

Perhaps provide some examples of when it should have already happened?

• We have a non-null, non-i31ref gc ref: in order to have allocated this reference's object, we must have allocated a gc heap and associated GcStore
• We are working with an instance whose associated wasmtime_environ::Module::needs_gc_heap flag is true: we always allocate the gc store during instantiation for these moduels.
• etc

Wasmtime GitHub notifications bot (Aug 11 2025 at 18:57):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 11 2025 at 18:57):

alexcrichton created PR review comment:

These are the problematic writes yeah and a leading cause of failure in the tests below. There's no such ensure in Global::new right now which is why these are failing.

Wasmtime GitHub notifications bot (Aug 11 2025 at 19:42):

alexcrichton updated PR #11411.

Wasmtime GitHub notifications bot (Aug 11 2025 at 19:51):

alexcrichton updated PR #11411.

Wasmtime GitHub notifications bot (Aug 11 2025 at 19:52):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 11 2025 at 19:52):

alexcrichton created PR review comment:

Agreed yeah, I updated compilation to look at the top type of tables and flag needs_gc_heap from that as appropriate.

Wasmtime GitHub notifications bot (Aug 11 2025 at 19:53):

alexcrichton commented on PR #11411:

Ok I think everything should be handled now. Mind taking another look @fitzgen?

Wasmtime GitHub notifications bot (Aug 11 2025 at 19:55):

alexcrichton updated PR #11411.

Wasmtime GitHub notifications bot (Aug 11 2025 at 20:08):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Aug 11 2025 at 20:08):

fitzgen created PR review comment:

LOL how did this sneak by????

Wasmtime GitHub notifications bot (Aug 11 2025 at 20:08):

fitzgen created PR review comment:

                    self.result.module.needs_gc_heap |= table
                        .ref_type
                        .is_vmgcref_type_and_points_to_object();

Wasmtime GitHub notifications bot (Aug 11 2025 at 20:19):

alexcrichton updated PR #11411.

Wasmtime GitHub notifications bot (Aug 11 2025 at 20:21):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 11 2025 at 20:21):

alexcrichton created PR review comment:

I ended up going with is_vmgcref_type since the method here is on wasmtime_environ types and not wasmtime types (therefore is_vmgcref_type_and_points_to_object not naively available). I tried using is_vmgcref_type_not_i31 but due to the way table initialization works it requires the GC store to be present right now so I switched it to is_vmgcref_type. Should be easy enough to fix in the future if we really want, but I figure it's not too important to be too optimal here and keeping the same code between the externref/exnref/anyref clauses is nice

Wasmtime GitHub notifications bot (Aug 11 2025 at 20:34):

alexcrichton updated PR #11411.

Wasmtime GitHub notifications bot (Aug 11 2025 at 20:39):

alexcrichton has enabled auto merge for PR #11411.

Wasmtime GitHub notifications bot (Aug 11 2025 at 21:09):

alexcrichton merged PR #11411.

Last updated: Dec 06 2025 at 06:05 UTC