wasmtime / PR #12985 Only const-eval passive element segm... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #12985 Only const-eval passive element segm...

Wasmtime GitHub notifications bot (Apr 07 2026 at 19:34):

fitzgen requested wasmtime-core-reviewers for a review on PR #12985.

Wasmtime GitHub notifications bot (Apr 07 2026 at 19:34):

fitzgen opened PR #12985 from fitzgen:const-eval-once to bytecodealliance:main:

Not each time they are referenced by e.g. an array.new_elem instruction.

Wasmtime GitHub notifications bot (Apr 07 2026 at 19:34):

fitzgen requested cfallin for a review on PR #12985.

Wasmtime GitHub notifications bot (Apr 07 2026 at 19:41):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 07 2026 at 19:41):

fitzgen created PR review comment:

Ignore this, I just need to remove it. Forgot that ValRaw's raw GC refs are raw GC refs.

Wasmtime GitHub notifications bot (Apr 07 2026 at 19:42):

cfallin submitted PR review:

LGTM!

Wasmtime GitHub notifications bot (Apr 07 2026 at 19:42):

cfallin created PR review comment:

would this make sense as a helper ty.needs_gc_rooting() ?

Wasmtime GitHub notifications bot (Apr 07 2026 at 20:35):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 07 2026 at 20:35):

fitzgen created PR review comment:

That's essentially what is_vmgcref_type_and_not_i31 is, but this allows us to avoid the type traversal at runtime, having done it at Wasm compile time already and saved the result in needs_gc_rooting. I just didn't want to save bool because it makes use-sites hard to understand what is happening.

Wasmtime GitHub notifications bot (Apr 07 2026 at 20:43):

fitzgen updated PR #12985.

Wasmtime GitHub notifications bot (Apr 07 2026 at 20:43):

fitzgen has enabled auto merge for PR #12985.

Wasmtime GitHub notifications bot (Apr 07 2026 at 20:57):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Apr 07 2026 at 20:57):

cfallin created PR review comment:

Sure, I guess I was thinking it'd be nice to package up the link from "is VMGCRef and not an i31" to "needs GC rooting" since you have the enum naming that concept (or maybe a ctor on NeedsGcRooting otherwise that takes a ty) but no big deal either way.

Wasmtime GitHub notifications bot (Apr 07 2026 at 21:13):

fitzgen updated PR #12985.

Wasmtime GitHub notifications bot (Apr 07 2026 at 21:26):

fitzgen added PR #12985 Only const-eval passive element segments once to the merge queue.

Wasmtime GitHub notifications bot (Apr 07 2026 at 21:57):

fitzgen merged PR #12985.

Wasmtime GitHub notifications bot (Apr 07 2026 at 21:57):

fitzgen removed PR #12985 Only const-eval passive element segments once from the merge queue.

Wasmtime GitHub notifications bot (Apr 07 2026 at 23:37):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 07 2026 at 23:37):

alexcrichton created PR review comment:

Or, put another way, should the tracing of instance-owned items move into here now? If not, could this be renamed to make it clear it's not a general purpose tracing function?

Wasmtime GitHub notifications bot (Apr 07 2026 at 23:37):

alexcrichton created PR review comment:

Could this be a TryPrimaryMap<PassiveIndex, Option<...>>?

Wasmtime GitHub notifications bot (Apr 07 2026 at 23:37):

alexcrichton created PR review comment:

Could this be a method on *const ValRaw or something like that? It seems a bit sketchy to just cast away the ValRaw w.r.t endianness and such, as opposed to having a specific conversion which does something like &raw mut ... or similar.

Wasmtime GitHub notifications bot (Apr 07 2026 at 23:37):

alexcrichton created PR review comment:

Can this have a passive_elements_mut helper like other field accessors in this file?

Wasmtime GitHub notifications bot (Apr 07 2026 at 23:37):

alexcrichton created PR review comment:

Perhaps a naive question: how come this isn't tracing tables or globals?

Wasmtime GitHub notifications bot (Apr 07 2026 at 23:37):

alexcrichton created PR review comment:

Although, actually, this seems pretty suspect. Here this is not Pin<&mut ...> but below there's an accessor for Pin<&mut ...>. Below the Pin isn't actually doing anything I think because the internal type doesn't require pinning, and additionally I don't think that the type needs to be pinned? I'm a bit confused now...

Wasmtime GitHub notifications bot (Apr 09 2026 at 19:06):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 09 2026 at 19:07):

fitzgen created PR review comment:

Even better would be rebasing and landing #12621 and then using TrySecondaryMap here. Doesn't really seem better to me to replace one poor-man's TrySecondaryMap with a different poor-man's TrySecondaryMap. Do you have objections to that?

Wasmtime GitHub notifications bot (Apr 09 2026 at 23:08):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 09 2026 at 23:08):

alexcrichton created PR review comment:

Well #12621 is caught up, in my opinion at least, about the usage of secondary maps in the type registry where the use case isn't clear. Here, however, the usefulness is clear, but I don't think the map-like behavior of SecondaryMap is useful here. Here for example it seems like this is created/mutated/accessed exactly like a PrimaryMap where there's always an entry-per-key.

Wasmtime GitHub notifications bot (Apr 10 2026 at 15:38):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 10 2026 at 15:38):

fitzgen created PR review comment:

Well #12621 is caught up, in my opinion at least, about the usage of secondary maps in the type registry where the use case isn't clear.

There is no reason that merging #12621 means we can't continue to evolve the TypeRegistry representation in #12662 IMO: they don't have to be bundled together as one all-or-nothing thing.

Here for example it seems like this is created/mutated/accessed exactly like a PrimaryMap where there's always an entry-per-key.

I think this is a misunderstanding of SecondaryMap: primary vs secondary isn't really about density per se (you can always do PrimaryMap<K, Option<V>>) but about defining an index space versus reusing an existing index space.

In this example, the index space is defined by wasmtime_environ::Module::passive_elements: PrimaryMap<PassiveElementIndex, _>, and in vm::Instance::passive_elements we are creating a side table to associate new data with that existing, already-defined index space. We aren't defining a new index space.

By using PrimaryMap here again, we would be defining another index space but with the invariant that it actually must map 1:1 with the first index space (and in practice that invariant requires that we must insert into the new PrimaryMap in the exact same order as the first PrimaryMap, for example). Although that invariant is relatively easy to maintain in this case, it is additional complexity and it is fairly subtle if you aren't familiar with all the moving parts already. On the other hand, SecondaryMap completely avoids that invariant, doesn't require inserting in order, and removes the complexity/subtlety.

Wasmtime GitHub notifications bot (Apr 10 2026 at 15:48):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 10 2026 at 15:48):

fitzgen created PR review comment:

If your reservations about #12621 are primarily about how we aren't "just" wrapping an inner type any more but implementing it "from scratch" on top of Vec (which should probably be TryVec actually) then we could experiment a little bit and see if implementing it on top of TryPrimaryMap instead of [Try]Vec cuts down on lines of code, something like this:

pub struct TrySecondaryMap<K, V>
where
    K: EntityRef,
{
    inner: TryPrimaryMap<K, V>,
    default_value: V,
    phantom: PhantomData<fn(K) -> V>,
}

impl<K, V> Index<K> for SecondaryMap<K, V>
where
    K: EntityRef,
{
    type Output = V;

    fn index(&self, k: K) -> &V {
        self.get(k).unwrap_or(&self.default_value)
    }
}

impl<K, V> TrySecondaryMap<K, V>
where
    K: EntityRef,
{
    pub fn insert(&mut self, k: K, v: V) -> Result<Option<V>, OutOfMemory>
    where
        K: EntityRef,
        V: TryClone,
    {
        if let Some(old) = self.inner.get_mut(k) {
            Ok(Some(mem::replace(old, v)))
        } else {
            self.resize(k.index() + 1)?;
            self.elems[k] = v;
            Ok(None)
        }
    }

    // etc...
}

Last updated: Apr 12 2026 at 23:10 UTC