Stream: git-wasmtime

Topic: wasmtime / PR #11547 merge `concurrent::table::Table` bac...

Wasmtime GitHub notifications bot (Aug 26 2025 at 22:56):

dicej requested alexcrichton for a review on PR #11547.

Wasmtime GitHub notifications bot (Aug 26 2025 at 22:56):

dicej opened PR #11547 from dicej:deduplicate-resource-table to bytecodealliance:main:

When I started implementing component model async support, I copied resource_table.rs to concurrent/table.rs and made some modifications. Now I'm unifying the implementations to reduce duplication.

• concurrent/table.rs now only contains the TableId type and the TableDebug trait; the Table type has been merged into ResourceTable.

• TableId can be thought of as a lightweight variation of Resource representing handles which can't be borrowed. It also implements Copy, Hash, Debug, etc. for convenience, and can be converted to and from Resource.

• ResourceTable now has enable_debug, is_empty, add_child, remove_child, and iter_mut methods.

• ConcurrentState::table is now of type Mutex<ResourceTable>; the Mutex wrapper is needed to ensure the field is Sync, but we never lock it.

Fixes #11190

<!--
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 26 2025 at 22:56):

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

Wasmtime GitHub notifications bot (Aug 27 2025 at 13:10):

dicej updated PR #11547.

Wasmtime GitHub notifications bot (Aug 27 2025 at 13:16):

dicej updated PR #11547.

Wasmtime GitHub notifications bot (Aug 27 2025 at 14:41):

alexcrichton created PR review comment:

I'd probably say go ahead and remove this and only have get_mut for now

Wasmtime GitHub notifications bot (Aug 27 2025 at 14:41):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 27 2025 at 14:41):

alexcrichton created PR review comment:

Could this replace Mutex with AlwaysMut added in https://github.com/bytecodealliance/wasmtime/pull/11453?

Wasmtime GitHub notifications bot (Aug 27 2025 at 14:41):

alexcrichton created PR review comment:

While this is true of indices we hand out to wasm itself, I think this can be removed now? I think that this was required before merging the wasm-facing tables together but nowadays I think this should be unnecessary

Wasmtime GitHub notifications bot (Aug 27 2025 at 14:41):

alexcrichton created PR review comment:

Have you used per-table debugging in the past? If not how about moving this to a const in this module? That way it can still be easily flipped but won't take up space at runtime

Wasmtime GitHub notifications bot (Aug 27 2025 at 14:47):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Aug 27 2025 at 14:47):

dicej created PR review comment:

You're right that this was originally a hack because I was handing these handles directly to guests in an early draft. Once I stopped doing that, I removed the hack, but later I added it back in -- not because we were handing these handles to guest, but because I found it useful e.g. here.

In other words, it's not strictly necessary, but it could help us catch our own mistakes more easily.

Wasmtime GitHub notifications bot (Aug 27 2025 at 14:49):

alexcrichton created PR review comment:

Could u32::MAX be used there instead? It's effectively a nice property of container-like data structures that creation doesn't require allocation, so that's what I'd like to shoot for here

Wasmtime GitHub notifications bot (Aug 27 2025 at 14:49):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 27 2025 at 14:52):

dicej created PR review comment:

I'm using it in some parts of concurrent-async-tests and did it this way because I didn't want a static or const to bleed over into other tests where it wasn't meant to be enabled, but I guess it's not that important. Maybe I'll just change the self.debug checks to cfg!(debug_assertions).

Wasmtime GitHub notifications bot (Aug 27 2025 at 14:52):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Aug 27 2025 at 14:56):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 27 2025 at 14:56):

alexcrichton created PR review comment:

That seems reasonable to me as well yeah :+1:

Wasmtime GitHub notifications bot (Aug 27 2025 at 15:20):

dicej updated PR #11547.

Wasmtime GitHub notifications bot (Aug 27 2025 at 15:21):

dicej has enabled auto merge for PR #11547.

Wasmtime GitHub notifications bot (Aug 27 2025 at 15:49):

dicej updated PR #11547.

Wasmtime GitHub notifications bot (Aug 27 2025 at 16:10):

dicej has disabled auto merge for PR #11547.

Wasmtime GitHub notifications bot (Aug 27 2025 at 16:45):

dicej merged PR #11547.

Last updated: Dec 06 2025 at 07:03 UTC