Wasmtime GitHub notifications bot (Jul 05 2023 at 20:00): Wasmtime GitHub notifications bot (Jul 05 2023 at 20:00):alexcrichton opened PR #6693 from alexcrichton:retain-only-one-pointer to bytecodealliance:main:
This commit updates how instantiation arguments are rooted within a component instance. Previously a list of import arguments was "unzipped" into a list of modules and a list of functions. This is becoming a bit more cumbersome in an upcoming change for resources where a third style of import is being added, so instead of this unzipping operation this commit instead replaces the list of imports with an
Arc. This way instantiation is actually a bit cheaper since only oneArcneeds to be retained instead of each individual argument imported into a module.Additionally this refactors the way that exported modules are handled by avoiding translating everything into one list and instead continuing to store modules in two separate lists: those in the component and those imported.
<!--
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.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Jul 05 2023 at 20:00):alexcrichton requested jameysharp for a review on PR #6693.
Wasmtime GitHub notifications bot (Jul 05 2023 at 20:00):alexcrichton requested wasmtime-core-reviewers for a review on PR #6693.
Wasmtime GitHub notifications bot (Jul 05 2023 at 20:43):jameysharp submitted PR review:
I think this makes sense! I'm not absolutely sure I understand the surrounding context but this does appear to do what you said it should.
Wasmtime GitHub notifications bot (Jul 05 2023 at 20:43):jameysharp submitted PR review:
I think this makes sense! I'm not absolutely sure I understand the surrounding context but this does appear to do what you said it should.
Wasmtime GitHub notifications bot (Jul 05 2023 at 20:43):jameysharp created PR review comment:
Minor typo:
/// Strong references are stored to these arguments since pointers are saved
Wasmtime GitHub notifications bot (Jul 05 2023 at 20:43):jameysharp created PR review comment:
Maybe grab a little more of this comment to add to the documentation for the
importsfield above? Assuming this explanation is still accurate with regard to what's being retained and why, I don't see anything else describing those details.
Wasmtime GitHub notifications bot (Jul 05 2023 at 23:02):alexcrichton updated PR #6693.
Wasmtime GitHub notifications bot (Jul 05 2023 at 23:04):alexcrichton created PR review comment:
Good point yeah, in general the component model docs, even on the internals, are long due for an update. This is a pattern used commonly throughout the core wasm implementation as well though (which also isn't necessarily thoroughly documented).
One difficulty is that it's not easy to tie the data structure's types to "oh yes here's the point where the
unsafeblock is ok" since the unsafety is so pervasive. Or at least right now with our current organization I'm not aware of a "single spot" but rather the whole codebase assumes a store lives forever and everything needed by anything in a store must be rooted in the store somehow.
Wasmtime GitHub notifications bot (Jul 05 2023 at 23:04):alexcrichton has enabled auto merge for PR #6693.
Wasmtime GitHub notifications bot (Jul 06 2023 at 00:08):alexcrichton merged PR #6693.
Last updated: Oct 23 2024 at 20:03 UTC