use a single table per instance for resources, waitables, etc.#11374
Conversation
github-actions
Bot
added
the
wasmtime:api
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
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.
| fn get_mut_by_rep(&mut self, rep: u32) -> Option<(u32, &mut Slot)> { | ||
| let index = *self.reps_to_indexes.get(usize::try_from(rep).unwrap())?; | ||
| if index > 0 { | ||
| let slot = self.get_mut(index).unwrap(); | ||
| Some((index, slot)) | ||
| } else { | ||
| None | ||
| } | ||
| } |
There was a problem hiding this 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:
- 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.
- 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.
There was a problem hiding this comment.
Sounds reasonable; I'll give it a shot.
There was a problem hiding this comment.
I just pushed an update; please let me know what you think.
LGTM, thanks. |
Per WebAssembly/component-model#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 bytecodealliance#11189 Signed-off-by: Joel Dice <joel.dice@fermyon.com> Re-internalize `ResourceKind` to `mod handle_table` Remove `ResourceKind` Start flattening the representation of `Slot` Internalize `get_mut_handle_by_index` Internalize implementation details such as the representation of slots to and make methods a bit more targeted in their functionality. Internalize more details of `HandleTable` Don't expose `HandleKind` and of per-function methods for operating on the various kinds of handles that reside in the table. Flatten the representation of `Slot` There's still some more work to do for host/guest resource handles, but this helps realize the goal of the previous refactorings in the meantime.
dicej
force-pushed
the
all-handles-in-same-table
branch
from
f2b787c to
b47ebfa
Compare
Per review feedback, we'd like to get rid of `HandleTable::reps_to_indexes` entirely. This commit doesn't go quite that far, but now we only use it for `error-context` handles. For waitables, which can only be referenced by at most one guest at a time, we now store the guest handle in `WaitableCommon::handle` and retrieve it from there when delivering an event for that waitable. 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. Going forward, we could consider either removing that "same handle" requirement from the spec or consider an alternative implementation (e.g. storing a `HashMap<RuntimeComponentIndex, usize>` in the `ErrorContext` host state for keeping track of the handles for each referencing instance. Signed-off-by: Joel Dice <joel.dice@fermyon.com>
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
👍
To clarify though, in the commit message you say:
For
error-contexthandles, the spec requirement that we always lower the same
handle for the sameerror-context, combined with the fact that an
error-contextmay 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 |
Turns out the spec no longer requires that guests receive the same handle for a given `error-context` as the one they already have, so we no longer need this field -- nor do we need to maintain a per-component-instance reference count. Signed-off-by: Joel Dice <joel.dice@fermyon.com>
…odealliance#11374) * use a single table per instance for resources, waitables, etc. Per WebAssembly/component-model#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 bytecodealliance#11189 Signed-off-by: Joel Dice <joel.dice@fermyon.com> Re-internalize `ResourceKind` to `mod handle_table` Remove `ResourceKind` Start flattening the representation of `Slot` Internalize `get_mut_handle_by_index` Internalize implementation details such as the representation of slots to and make methods a bit more targeted in their functionality. Internalize more details of `HandleTable` Don't expose `HandleKind` and of per-function methods for operating on the various kinds of handles that reside in the table. Flatten the representation of `Slot` There's still some more work to do for host/guest resource handles, but this helps realize the goal of the previous refactorings in the meantime. * stop using `HandleTable::reps_to_indexes` when delivering events Per review feedback, we'd like to get rid of `HandleTable::reps_to_indexes` entirely. This commit doesn't go quite that far, but now we only use it for `error-context` handles. For waitables, which can only be referenced by at most one guest at a time, we now store the guest handle in `WaitableCommon::handle` and retrieve it from there when delivering an event for that waitable. 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. Going forward, we could consider either removing that "same handle" requirement from the spec or consider an alternative implementation (e.g. storing a `HashMap<RuntimeComponentIndex, usize>` in the `ErrorContext` host state for keeping track of the handles for each referencing instance. Signed-off-by: Joel Dice <joel.dice@fermyon.com> * remove `HandleTable::reps_to_indexes` Turns out the spec no longer requires that guests receive the same handle for a given `error-context` as the one they already have, so we no longer need this field -- nor do we need to maintain a per-component-instance reference count. Signed-off-by: Joel Dice <joel.dice@fermyon.com> --------- Signed-off-by: Joel Dice <joel.dice@fermyon.com>
2 participants
Per WebAssembly/component-model#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
ResourceTableandStateTabledata structures into a singleHandleTablestructureRename
ComponentInstance::instance_resource_tablestoinstance_handle_tablesRemove
ConcurrentState::waitable_tablesanderror_context_tablesin favor of the aboveMove various associated functions from
ConcurrentStatetoComponentInstanceso they can accessinstance_resource_tablesWhile I was doing table-related things, I also updated
concurrent::Table::newto reserve the zero handle to mean "invalid". This won't affect what the guest sees in any way, but it allows us to useTableId::new(0)to invalidate host-owned handles in e.g.{Stream,Future}{Reader,Writer}::close.Fixes #11189