Stream: git-wasmtime

Topic: wasmtime / PR #7596 Replace the preview2 table's HashMap ...

Wasmtime GitHub notifications bot (Nov 28 2023 at 17:49):

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.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 (Nov 28 2023 at 17:59):

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.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 (Nov 28 2023 at 18:01):

elliottt updated PR #7596.

Wasmtime GitHub notifications bot (Nov 28 2023 at 18:02):

elliottt updated PR #7596.

Wasmtime GitHub notifications bot (Nov 28 2023 at 18:06):

elliottt updated PR #7596.

Wasmtime GitHub notifications bot (Nov 28 2023 at 18:42):

elliottt has marked PR #7596 as ready for review.

Wasmtime GitHub notifications bot (Nov 28 2023 at 18:42):

elliottt requested alexcrichton for a review on PR #7596.

Wasmtime GitHub notifications bot (Nov 28 2023 at 18:42):

elliottt requested wasmtime-core-reviewers for a review on PR #7596.

Wasmtime GitHub notifications bot (Nov 28 2023 at 18:42):

elliottt requested pchickey for a review on PR #7596.

Wasmtime GitHub notifications bot (Nov 28 2023 at 19:11):

elliottt updated PR #7596.

Wasmtime GitHub notifications bot (Nov 28 2023 at 20:51):

pchickey submitted PR review:

Looks good to me. Thank you

Wasmtime GitHub notifications bot (Nov 28 2023 at 20:51):

pchickey submitted PR review:

Looks good to me. Thank you

Wasmtime GitHub notifications bot (Nov 28 2023 at 20:51):

pchickey created PR review comment:

I don't have an intuition for what a good magic number for capacity is. @alexcrichton any guesses?

Wasmtime GitHub notifications bot (Nov 28 2023 at 21:52):

alexcrichton submitted PR review:

I've got some thoughts on possibly simplifying this a bit, but overall looks good!

Wasmtime GitHub notifications bot (Nov 28 2023 at 21:52):

alexcrichton submitted PR review:

I've got some thoughts on possibly simplifying this a bit, but overall looks good!

Wasmtime GitHub notifications bot (Nov 28 2023 at 21:52):

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 a pub method with the same name and the _-version erases generics effectively. Could the _ be removed when that's not necessary?

Wasmtime GitHub notifications bot (Nov 28 2023 at 21:52):

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 from with_capacity really other than letting default heuristics take over.

Wasmtime GitHub notifications bot (Nov 28 2023 at 21:52):

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.

Wasmtime GitHub notifications bot (Nov 28 2023 at 21:52):

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?

Wasmtime GitHub notifications bot (Nov 29 2023 at 16:42):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2023 at 16:42):

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.

Wasmtime GitHub notifications bot (Nov 29 2023 at 16:44):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2023 at 16:44):

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.

Wasmtime GitHub notifications bot (Nov 29 2023 at 16:44):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2023 at 16:44):

elliottt created PR review comment:

Absolutely, I was just trying to follow the conventions already present.

Wasmtime GitHub notifications bot (Nov 29 2023 at 16:46):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2023 at 16:46):

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?

Wasmtime GitHub notifications bot (Nov 29 2023 at 16:57):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2023 at 16:57):

elliottt created PR review comment:

What do you think about removing push_child_? it's called only by push_child and would inline without issues.

Wasmtime GitHub notifications bot (Nov 29 2023 at 17:03):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2023 at 17:03):

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.

Wasmtime GitHub notifications bot (Nov 29 2023 at 17:04):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2023 at 17:04):

alexcrichton created PR review comment:

Ok keeping with_capacity makes sense to me, but I think that new should create with the default 0 capacity of Vec and then with_capacity sets an initial capacity.

Wasmtime GitHub notifications bot (Nov 29 2023 at 17:07):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2023 at 17:07):

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.

Wasmtime GitHub notifications bot (Nov 29 2023 at 17:17):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2023 at 17:17):

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 :)

Wasmtime GitHub notifications bot (Nov 29 2023 at 17:19):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2023 at 17:19):

alexcrichton created PR review comment:

Oops, I also am appparently forgetting what I was just saying yesterday. Anyway sounds good!

Wasmtime GitHub notifications bot (Nov 29 2023 at 17:47):

elliottt updated PR #7596.

Wasmtime GitHub notifications bot (Nov 29 2023 at 17:50):

elliottt updated PR #7596.

Wasmtime GitHub notifications bot (Nov 29 2023 at 18:03):

elliottt updated PR #7596.

Wasmtime GitHub notifications bot (Nov 29 2023 at 18:12):

elliottt updated PR #7596.

Wasmtime GitHub notifications bot (Nov 29 2023 at 18:20):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2023 at 19:07):

elliottt merged PR #7596.

Last updated: Nov 22 2024 at 16:03 UTC