Stream: git-wasmtime

Topic: wasmtime / PR #8105 Stop threading Cranelift tables throu...

Wasmtime GitHub notifications bot (Mar 12 2024 at 18:30):

jameysharp requested fitzgen for a review on PR #8105.

Wasmtime GitHub notifications bot (Mar 12 2024 at 18:30):

jameysharp opened PR #8105 from jameysharp:dedup-table-indexes to bytecodealliance:main:

Previously, every table-related method in the FuncEnvironment trait took two IDs to identify a table: the TableIndex from the WebAssembly module, and the Cranelift Table we generated associated with it.

This is redundant and most implementations only used one or the other. The TableIndex is a perfectly good identifier and we can use it in a SecondaryMap if we need to associate additional data with the table, such as a Cranelift Table ID.

This is a step toward #5532, where we don't want to use Cranelift's table support at all, so the associated data that an implementation needs will be different.

<!--
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 (Mar 12 2024 at 18:30):

jameysharp requested wasmtime-compiler-reviewers for a review on PR #8105.

Wasmtime GitHub notifications bot (Mar 12 2024 at 18:30):

jameysharp requested wasmtime-core-reviewers for a review on PR #8105.

Wasmtime GitHub notifications bot (Mar 13 2024 at 13:20):

fitzgen submitted PR review:

Nice!

Wasmtime GitHub notifications bot (Mar 13 2024 at 13:20):

fitzgen submitted PR review:

Nice!

Wasmtime GitHub notifications bot (Mar 13 2024 at 13:20):

fitzgen created PR review comment:

It seems like every time we call ensure_table_exists we then do let table = self.tables[table_index];

Might be worth returning that value from ensure_table_exists to clean up call sites a bit.

Wasmtime GitHub notifications bot (Mar 13 2024 at 15:32):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Mar 13 2024 at 15:32):

jameysharp created PR review comment:

I wish I could! In this PR it would be fine, since the values in self.tables are Copy, but later I'm going to have to extend it to a full struct that I'd want to return a borrow of. Unfortunately, if you have a fn f(&'_ mut T) -> &'_ T, the shared borrow that's returned acts like a mutable borrow, for reasons I don't entirely understand; the mutability is apparently attached to the lifetime. So I'll have to introduce this pattern later anyway.

I've also considered eagerly creating all the associated table data so that ensure_table_exists can go away, but that seemed like a larger change I didn't want to think through.

Wasmtime GitHub notifications bot (Mar 13 2024 at 16:43):

jameysharp updated PR #8105.

Wasmtime GitHub notifications bot (Mar 13 2024 at 16:43):

jameysharp has enabled auto merge for PR #8105.

Wasmtime GitHub notifications bot (Mar 13 2024 at 17:28):

jameysharp merged PR #8105.

Last updated: Dec 23 2024 at 12:05 UTC