elliottt opened PR #7596 from elliottt:trevor/wasi-table
to bytecodealliance:main
:
Fix the TODO in preview2's table implementation about performance of key generation once the index number space has wrapped; instead of incrementing a u32, store resources in a vector and keep a free queue in the empty slots.
<!--
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
-->
elliottt edited PR #7596:
Fix the TODO in preview2's table implementation about performance of key generation once the index space has wrapped; instead of incrementing a u32, store resources in a vector and keep a free queue in the empty slots.
<!--
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
-->
elliottt updated PR #7596.
elliottt updated PR #7596.
elliottt updated PR #7596.
elliottt has marked PR #7596 as ready for review.
elliottt requested alexcrichton for a review on PR #7596.
elliottt requested wasmtime-core-reviewers for a review on PR #7596.
elliottt requested pchickey for a review on PR #7596.
elliottt updated PR #7596.
pchickey submitted PR review:
Looks good to me. Thank you
pchickey submitted PR review:
Looks good to me. Thank you
pchickey created PR review comment:
I don't have an intuition for what a good magic number for capacity is. @alexcrichton any guesses?
alexcrichton submitted PR review:
I've got some thoughts on possibly simplifying this a bit, but overall looks good!
alexcrichton submitted PR review:
I've got some thoughts on possibly simplifying this a bit, but overall looks good!
alexcrichton created PR review comment:
Lots of methods here have
_
as a suffix, and while that was used for some prior that was only done when there's apub
method with the same name and the_
-version erases generics effectively. Could the_
be removed when that's not necessary?
alexcrichton created PR review comment:
Could
with_capacity
be removed and this start as an empty vec? I'm not sure if there's much to gain fromwith_capacity
really other than letting default heuristics take over.
alexcrichton created PR review comment:
Could this perhaps become just
free_head
? Currently this looks like a FIFO list but changing it to LIFO I think is ok as well and removes the amount of state to manage here.
alexcrichton created PR review comment:
I was naively expecting a slab-like allocator when taking a look at this, which wouldn't have this branch. Could this perhaps be removed to help simplify this a bit further?
elliottt submitted PR review.
elliottt created PR review comment:
My thought here was that it would be good to allow a bit of control so that embedders could make that decision depending on their workloads. Perhaps that's over-estimating the cost of reallocating the vector though.
elliottt submitted PR review.
elliottt created PR review comment:
Yep, that's fine. I made it a LIFO after discussing this refactoring with @jameysharp. He had a good suggestion, which is that when debugging code that's using resources, it would probably be helpful to have the generated ids look as distinct as possible. Using a LIFO means that we'd see the oldest free'd id reused first when logging, rather than potentially seeing a lot of churn on the most recent one.
elliottt submitted PR review.
elliottt created PR review comment:
Absolutely, I was just trying to follow the conventions already present.
elliottt submitted PR review.
elliottt created PR review comment:
The intent here was to prefer generating new ids, in service of making more debuggable logs. As we've already allocated the space, why not use it and fall back on the free list when it's full?
elliottt submitted PR review.
elliottt created PR review comment:
What do you think about removing
push_child_
? it's called only bypush_child
and would inline without issues.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Sounds good to me! Many of these are historical and the artifact of a number of refactorings, so it's definitely ripe for taking the liberty to restructure things while you're here.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Ok keeping
with_capacity
makes sense to me, but I think thatnew
should create with the default 0 capacity ofVec
and thenwith_capacity
sets an initial capacity.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Trevor and I talked a bit on Zulip, and I agree that this would be useful for that form of debugging but I'd personally prefer to stick to a simpler FIFO queue since I think it's not clear whether this form of debugging would be relied on all that much. As an isolated implementation detail it's easy to swap this out in the future, and I'd prefer personally to see the need for such debugging come up first before we go with the slightly fancier LIFO implementation.
It's sort of 6-to-one-half-dozen-or-the-other though so I don't feel too strongly either way, but I do have a preference for the FIFO version. I'm probably biased though b/c that's what I was assuming this was before I read it.
elliottt submitted PR review.
elliottt created PR review comment:
I mixed up my acronyms. Just for the record, I'm going to remove the tail pointer and make this a LIFO, removing the existing FIFO behavior. Sorry about the confusion there :)
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Oops, I also am appparently forgetting what I was just saying yesterday. Anyway sounds good!
elliottt updated PR #7596.
elliottt updated PR #7596.
elliottt updated PR #7596.
elliottt updated PR #7596.
alexcrichton submitted PR review.
elliottt merged PR #7596.
Last updated: Dec 23 2024 at 12:05 UTC