Stream: git-wasmtime

Topic: wasmtime / PR #11374 use a single table per instance for ...

Wasmtime GitHub notifications bot (Aug 01 2025 at 22:18):

dicej requested alexcrichton for a review on PR #11374.

Wasmtime GitHub notifications bot (Aug 01 2025 at 22:18):

dicej opened PR #11374 from dicej:all-handles-in-same-table to bytecodealliance:main:

Per https://github.com/WebAssembly/component-model/pull/513, the spec now puts resources, waitables, waitable sets, subtasks, and error contexts in the same table per instance. This updates the implementation to match.

• Combine the ResourceTable and StateTable data structures into a single HandleTable structure

• Rename ComponentInstance::instance_resource_tables to instance_handle_tables

• Remove ConcurrentState::waitable_tables and error_context_tables in favor of the above

• Move various associated functions from ConcurrentState to ComponentInstance so they can access instance_resource_tables

While I was doing table-related things, I also updated concurrent::Table::new to reserve the zero handle to mean "invalid". This won't affect what the guest sees in any way, but it allows us to use TableId::new(0) to invalidate host-owned handles in e.g. {Stream,Future}{Reader,Writer}::close.

Fixes #11189

<!--
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 01 2025 at 22:18):

dicej requested wasmtime-core-reviewers for a review on PR #11374.

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

alexcrichton updated PR #11374.

Wasmtime GitHub notifications bot (Aug 05 2025 at 20:00):

alexcrichton submitted PR review:

Thanks! I left one comment below and I also pushed up some refactorings I did as well. Mostly I wanted to move away from "resources over there async things over here" in the sense that they were still very distinctly separated and I found it difficult how to use a HandleTable as a result due to having to remember which method to use for which object. Instead now I've refactored it so Slot has a "more flat" representation.

This brings a minor size benefit (slot is 24 bytes, not 32, more is possible with some minor refactoring) but additionally helps encapsulate all the low-level details of handle management. I personally feel like this cleaned up code in concurrent.rs and futures_and_streams.rs. I'd naturally like to double-check you're ok with these refactorings though so lemme know if you feel they don't fit well.

Wasmtime GitHub notifications bot (Aug 05 2025 at 20:00):

alexcrichton created PR review comment:

This method, and the reps_to_indexes field, makes me feel uncomfortable and is something that I would prefer to remove entirely unless we otherwise have good motivation for it. Tracing this backwards I found two uses for this:

1. For error-context this is used as the "make sure we always return the same error-context handle" logic to reuse the same handle. This is, IMO, a debatable part of the specification and also not something we're shipping with WASIp3. I'd prefer to remove the logic entirely and have an implementation-specific detail that lowers new error-context handles each time. That would remove the error-context dependency on this, keep programs working. In the future if we want this error-context behavior we can reevaluate.
2. For delivering an event this is used to do a reverse-lookup from the "rep" in the host to the handle that the guest has for the relevant waitable. Would it be possible to store the guest handle inside of the host's "rep" (or, rather, the host-state object for this) instead? I'm under the impression that guest-state handles can't be removed while they're being waited on which would mean that it should be safe to store the index on the host temporarily.

I mostly want to double-check with (2) since I'm less familiar with the code. If you agree with both of the above that would remove the need for this entirely (and reps_to_indexes) which I think would be a good cleanup.

Wasmtime GitHub notifications bot (Aug 12 2025 at 14:30):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Aug 12 2025 at 14:30):

dicej created PR review comment:

Sounds reasonable; I'll give it a shot.

Wasmtime GitHub notifications bot (Aug 12 2025 at 15:29):

dicej commented on PR #11374:

I'd naturally like to double-check you're ok with these refactorings though so lemme know if you feel they don't fit well.

LGTM, thanks.

Wasmtime GitHub notifications bot (Aug 12 2025 at 15:47):

dicej updated PR #11374.

Wasmtime GitHub notifications bot (Aug 12 2025 at 17:35):

dicej updated PR #11374.

Wasmtime GitHub notifications bot (Aug 12 2025 at 17:35):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Aug 12 2025 at 17:36):

dicej created PR review comment:

I just pushed an update; please let me know what you think.

Wasmtime GitHub notifications bot (Aug 12 2025 at 17:53):

alexcrichton submitted PR review:

:+1:

To clarify though, in the commit message you say:

For error-context handles, the spec requirement that we always lower the same
handle for the same error-context, combined with the fact that an
error-context may be referenced by more than one component instance at a time,
means we still need some general way to convert a host rep plus component index
into a handle.

Is the latter part here still a motivation for this "local" reference count? For example if reps_to_indexes were entirely removed then error_context_insert would always create a fresh new handle (a violation of the current spec) but there's still a "global" reference count for the cross-component reference count for an error-context. Given that would it be possible to delete reps_to_indexes and have a temporary spec violation while the upstream spec is being sorted out?

Wasmtime GitHub notifications bot (Aug 12 2025 at 19:54):

dicej commented on PR #11374:

:+1:

To clarify though, in the commit message you say:

For error-context handles, the spec requirement that we always lower the same
handle for the same error-context, combined with the fact that an
error-context may be referenced by more than one component instance at a time,
means we still need some general way to convert a host rep plus component index
into a handle.

Is the latter part here still a motivation for this "local" reference count? For example if reps_to_indexes were entirely removed then error_context_insert would always create a fresh new handle (a violation of the current spec) but there's still a "global" reference count for the cross-component reference count for an error-context. Given that would it be possible to delete reps_to_indexes and have a temporary spec violation while the upstream spec is being sorted out?

Looks like the error-context "same handle" requirement is no longer part of the spec anyway, so I'll go ahead and remove that.

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

dicej updated PR #11374.

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

dicej has enabled auto merge for PR #11374.

Wasmtime GitHub notifications bot (Aug 12 2025 at 20:53):

dicej merged PR #11374.

Last updated: Dec 06 2025 at 06:05 UTC