Stream: git-wasmtime

Topic: wasmtime / PR #5756 Move default blocks into jump tables

Wasmtime GitHub notifications bot (Feb 09 2023 at 22:44):

elliottt opened PR #5756 from trevor/default-in-jump-table to main:

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Feb 09 2023 at 22:51):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Feb 09 2023 at 22:51):

bjorn3 created PR review comment:

Maybe keep it as first element instead?

Wasmtime GitHub notifications bot (Feb 09 2023 at 22:51):

elliottt edited PR #5756 from trevor/default-in-jump-table to main:

While working through #5731, adapting branch_destination to return an iterator over multiple slices of BlockCall values was a little awkward, and yielded more complicated indexing for heavily used operations like inst_values. As the root of the problem is the jump table default target and data not living in contiguous memory, there was a somewhat obvious solution: inline the default block into the JumpTableData, and remove it from the BranchTable instruction data.

The resulting refactoring is implemented in this PR: jump tables require a default block when they are created, and maintain an invariant that the last element of their internal vector is always the default label. Additionally, they now export slices for both the vector and just the jump table, to make it easy to adapt existing uses of as_slice.

For a little bit of additional motivation, consider this godbolt link. There are three indexing functions implemented: one that indexes into two slices using nth on a chained iterator; one that checks which slice the index falls on; and one that indexes into a single slice. By far the best option is indexing into a single slice, followed by choosing the slice to index, followed by using nth. Without this refactoring, we would need to pick the approach in either index1 or index2 to finish transitioning br_table to using BlockCall arguments, and both feel like bad options.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Feb 09 2023 at 22:52):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Feb 09 2023 at 22:52):

bjorn3 created PR review comment:

*excluding

here and below?

Wasmtime GitHub notifications bot (Feb 09 2023 at 22:52):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 09 2023 at 22:52):

elliottt created PR review comment:

Thanks for catching that!

Wasmtime GitHub notifications bot (Feb 09 2023 at 22:53):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 09 2023 at 22:53):

elliottt created PR review comment:

The only downside there is that we would break the invariant that the table data is 0-indexed. However, as we can always return a slice to the table data, maybe that's fine?

Wasmtime GitHub notifications bot (Feb 09 2023 at 22:53):

elliottt updated PR #5756 from trevor/default-in-jump-table to main.

Wasmtime GitHub notifications bot (Feb 09 2023 at 22:56):

elliottt has marked PR #5756 as ready for review.

Wasmtime GitHub notifications bot (Feb 09 2023 at 23:09):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Feb 09 2023 at 23:09):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Feb 09 2023 at 23:09):

bjorn3 created PR review comment:

This sounds like define or definition to me. Default is a keyword, and default_ isn't the nicest either. Unless someone has a better suggestion I guess leaving it the way it is right now is fine.

Wasmtime GitHub notifications bot (Feb 09 2023 at 23:09):

bjorn3 created PR review comment:

This is still needed, right?

Wasmtime GitHub notifications bot (Feb 09 2023 at 23:09):

bjorn3 created PR review comment:

Iter skips the default block, right?

Wasmtime GitHub notifications bot (Feb 09 2023 at 23:09):

bjorn3 created PR review comment:

Maybe cleanup for a followup PR, but is a separate targets argument still necessary?

Wasmtime GitHub notifications bot (Feb 09 2023 at 23:09):

bjorn3 created PR review comment:

I think so.

Wasmtime GitHub notifications bot (Feb 09 2023 at 23:22):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 09 2023 at 23:22):

elliottt created PR review comment:

As it's using iter_mut, the default label is included.

Wasmtime GitHub notifications bot (Feb 09 2023 at 23:24):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 09 2023 at 23:24):

elliottt created PR review comment:

Right now it is, but I'd like to change that as well. It would be great to have a function for translating the blocks in the branch instructions instead of passing the labels as an extra argument.

Wasmtime GitHub notifications bot (Feb 09 2023 at 23:25):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 09 2023 at 23:25):

elliottt created PR review comment:

iter includes the default block. I decided that as_slice, as_slice_mut, iter, and iter_mut would iterate the whole vector, and that access to just the table or default block would be through table_slice and default_block.

Wasmtime GitHub notifications bot (Feb 09 2023 at 23:25):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Feb 09 2023 at 23:25):

bjorn3 created PR review comment:

I see

Wasmtime GitHub notifications bot (Feb 09 2023 at 23:26):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 09 2023 at 23:26):

elliottt created PR review comment:

Perhaps it would be better to go the other way, and introduce a special function for accessing a slice for the whole vector, keeping the existing functions as excluding the default?

Wasmtime GitHub notifications bot (Feb 09 2023 at 23:28):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Feb 09 2023 at 23:28):

bjorn3 created PR review comment:

Another option would be to not use the regular names for conversion to a slice or iterator at all, but instead explicitly call out which part is returned for each function, both those returning everything and those returning only a part.

Wasmtime GitHub notifications bot (Feb 09 2023 at 23:31):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 09 2023 at 23:31):

elliottt created PR review comment:

I think I'm going to rework this to keep the existing behavior consistent, and introduce new functions to iterate the whole range. Thanks for pointing this out :)

Wasmtime GitHub notifications bot (Feb 09 2023 at 23:56):

elliottt updated PR #5756 from trevor/default-in-jump-table to main.

Wasmtime GitHub notifications bot (Feb 09 2023 at 23:58):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 09 2023 at 23:58):

elliottt created PR review comment:

I switched it to iterate over all_branches_mut instead, which reads a bit better and doesn't change the meaning of the old function.

Wasmtime GitHub notifications bot (Feb 10 2023 at 00:00):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Feb 10 2023 at 00:00):

bjorn3 created PR review comment:

7.0.0

Wasmtime GitHub notifications bot (Feb 10 2023 at 00:01):

elliottt updated PR #5756 from trevor/default-in-jump-table to main.

Wasmtime GitHub notifications bot (Feb 10 2023 at 00:08):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 10 2023 at 00:08):

elliottt created PR review comment:

Whoops :sweat_smile:

Wasmtime GitHub notifications bot (Feb 10 2023 at 00:08):

elliottt updated PR #5756 from trevor/default-in-jump-table to main.

Wasmtime GitHub notifications bot (Feb 10 2023 at 00:55):

elliottt updated PR #5756 from trevor/default-in-jump-table to main.

Wasmtime GitHub notifications bot (Feb 10 2023 at 00:55):

elliottt updated PR #5756 from trevor/default-in-jump-table to main.

Wasmtime GitHub notifications bot (Feb 10 2023 at 00:56):

elliottt updated PR #5756 from trevor/default-in-jump-table to main.

Wasmtime GitHub notifications bot (Feb 10 2023 at 01:35):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Feb 10 2023 at 01:35):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Feb 10 2023 at 01:35):

jameysharp created PR review comment:

This comment should be updated to refer to all_branches instead of iter.

Wasmtime GitHub notifications bot (Feb 10 2023 at 01:35):

jameysharp created PR review comment:

I was going to say that this should just use table.all_branches() but then I remembered that would change the visit order. Instead, would you update the comment above to note that some callers depend on the default block being visited first?

Wasmtime GitHub notifications bot (Feb 10 2023 at 02:30):

elliottt updated PR #5756 from trevor/default-in-jump-table to main.

Wasmtime GitHub notifications bot (Feb 10 2023 at 16:53):

elliottt merged PR #5756.

Wasmtime GitHub notifications bot (Feb 13 2023 at 21:50):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 13 2023 at 21:50):

elliottt created PR review comment:

I implemented this change in #5770.

Last updated: Dec 23 2024 at 12:05 UTC