Stream: git-wasmtime

Topic: wasmtime / PR #8506 cranelift: Compress vcode range-lists

Wasmtime GitHub notifications bot (Apr 29 2024 at 20:20):

jameysharp opened PR #8506 from jameysharp:compress-range-lists to bytecodealliance:main:

These lists of ranges always cover contiguous ranges of an index space, meaning the start of one range is the same as the end of the previous range, so we can cut storage in half by only storing one endpoint of each range.

This in turn means we don't have to keep track of the other endpoint while building these lists, reducing the state we need to keep while building vcode and simplifying the various build steps.

Wasmtime GitHub notifications bot (Apr 29 2024 at 20:20):

jameysharp requested elliottt for a review on PR #8506.

Wasmtime GitHub notifications bot (Apr 29 2024 at 20:20):

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

Wasmtime GitHub notifications bot (Apr 29 2024 at 20:30):

jameysharp updated PR #8506.

Wasmtime GitHub notifications bot (Apr 29 2024 at 22:20):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 29 2024 at 22:20):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 29 2024 at 22:20):

fitzgen created PR review comment:

Why ignore?

Wasmtime GitHub notifications bot (Apr 29 2024 at 22:20):

fitzgen created PR review comment:

Can we add some typed indices for Ranges? Or reuse cranelift_entity::Entity?

I've always been slightly nervous about all of our raw indices in the backends.

Wasmtime GitHub notifications bot (Apr 29 2024 at 22:38):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Apr 29 2024 at 22:38):

jameysharp created PR review comment:

Because doc-tests don't work in non-public types, since they're compiled in a separate crate. I couldn't figure out a better way to document what these methods do than with code, so doc-test style seemed good, but didn't quite work how I wanted. I did build this with the module made public and the doc-tests un-ignored, and verified that they pass.

Wasmtime GitHub notifications bot (Apr 29 2024 at 22:38):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Apr 29 2024 at 22:38):

jameysharp created PR review comment:

I guess we could add some sort of type-system tagging, but what we have now is usize almost everywhere this is used, so it would be noisy.

That said, I had wanted to somehow associate each instance of Ranges with the Vec or Ranges that it indexes. I couldn't figure out a good way to do that, but putting a newtype wrapper around the indexes could work, with some helpers.

I'd prefer to land this without that change, since I believe it's easier to understand and no less safe than the _status quo_, but I do think this is worth thinking about more.

Wasmtime GitHub notifications bot (May 02 2024 at 00:39):

elliottt submitted PR review:

This is a really nice change!

Wasmtime GitHub notifications bot (May 02 2024 at 05:32):

jameysharp merged PR #8506.

Last updated: Nov 22 2024 at 16:03 UTC