Stream: git-wasmtime

Topic: wasmtime / PR #9892 Override component model `Lower::stor...

Wasmtime GitHub notifications bot (Dec 23 2024 at 00:02):

kaivol opened PR #9892 from kaivol:optimize-component-model-fp-performance to bytecodealliance:main:

Added overrides for Lower::store_list and Lift::load_list for floating point numbers to reduce overhead when passing or returning list<f32> and list<f64> to/from components.

I based this on the respective implementation for integers.

<!--
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 (Jan 02 2025 at 17:06):

dicej assigned dicej to PR #9892.

Wasmtime GitHub notifications bot (Jan 02 2025 at 17:33):

dicej submitted PR review:

Thanks for this! Looks reasonable to me overall; see my comments inline regarding the details.

Wasmtime GitHub notifications bot (Jan 02 2025 at 17:33):

dicej created PR review comment:

Again, these comments should be updated to discuss floating point types rather than integer types.

Wasmtime GitHub notifications bot (Jan 02 2025 at 17:33):

dicej created PR review comment:

This paragraph (and the code that follows) refers to $integer, but we really care about $float, right? We should rewrite this to talk about floating point numbers instead of integers, e.g. "all u8 patterns are valid $float patterns since $float is a IEEE 754 floating point type."

Wasmtime GitHub notifications bot (Jan 02 2025 at 17:33):

dicej created PR review comment:

                let (before, middle, end) = unsafe { dst.align_to_mut::<$float>() };

Presumably the alignment of e.g. f32 and u32 should be the same, but we might as well be precise here (and in similar code below).

Wasmtime GitHub notifications bot (Jan 12 2025 at 22:07):

kaivol commented on PR #9892:

Thanks for the review!
I'm a bit busy at the moment but I'll look into it when i find some time.

Wasmtime GitHub notifications bot (Mar 26 2025 at 13:33):

kaivol submitted PR review.

Wasmtime GitHub notifications bot (Mar 26 2025 at 13:33):

kaivol created PR review comment:

The reason I used integer types and align_to[_mut]::<$integer> was to handle endianness correctly.
I felt like using align_to[_mut]::<$float> and thereby transmuting the component's memory to $float is incorrect, because (in my understanding) $float represents floating point numbers in native endianness, while dst is a slice of floating point numbers with little-endian byte-order.

Therefore, I thought using $float for floating point numbers with different endianess would also cause confusion, in particular when converting endianess.

I also considered using only accordingly aligned bytes (e.g. [u8; 4]) for the memory of the component, but I was unsure if that would enable the same optimizations.
Is there any easy (built-in) way to benchmark Wasmtime in specific scenarios?

Let me know what you think.

Wasmtime GitHub notifications bot (Mar 26 2025 at 13:43):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Mar 26 2025 at 13:43):

dicej created PR review comment:

Good point; I hadn't considered what align_to_mut was actually doing. I agree we should leave it as you have it.

Is there any easy (built-in) way to benchmark Wasmtime in specific scenarios?

You can look under the benches directory in this repo to see if the scenario you're interested in is already covered; if not, you'd need to add a new benchmark to that suite.

Wasmtime GitHub notifications bot (Mar 27 2025 at 14:19):

kaivol updated PR #9892 (assigned to dicej).

Wasmtime GitHub notifications bot (Mar 27 2025 at 14:19):

kaivol updated PR #9892 (assigned to dicej).

Wasmtime GitHub notifications bot (Mar 27 2025 at 14:34):

kaivol commented on PR #9892:

I modified the code to work with byte slices ([u8]) when referring to the component's memory. I think this is the best way to show which variables are host and which are component memory.

I also got rid of the unsafe blocks, without compromising on performance (at least in my experiments).

Let me know if the code looks good, and if further comments would help clarify it.

Wasmtime GitHub notifications bot (Mar 27 2025 at 14:34):

kaivol has marked PR #9892 as ready for review.

Wasmtime GitHub notifications bot (Mar 27 2025 at 14:34):

kaivol requested fitzgen for a review on PR #9892.

Wasmtime GitHub notifications bot (Mar 27 2025 at 14:34):

kaivol requested wasmtime-core-reviewers for a review on PR #9892.

Wasmtime GitHub notifications bot (Mar 27 2025 at 16:47):

dicej submitted PR review:

Thanks!

Wasmtime GitHub notifications bot (Mar 27 2025 at 16:56):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Mar 27 2025 at 16:56):

dicej created PR review comment:

use core::iter;

That should fix https://github.com/bytecodealliance/wasmtime/actions/runs/14112166505/job/39533533593

Wasmtime GitHub notifications bot (Mar 27 2025 at 21:27):

kaivol updated PR #9892 (assigned to dicej).

Wasmtime GitHub notifications bot (Mar 27 2025 at 21:37):

dicej commented on PR #9892:

One last thing to make CI happy (I hope): please run cargo fmt and push the result.

Wasmtime GitHub notifications bot (Mar 27 2025 at 21:40):

kaivol updated PR #9892 (assigned to dicej).

Wasmtime GitHub notifications bot (Mar 27 2025 at 22:45):

dicej merged PR #9892.

Last updated: Apr 18 2025 at 01:31 UTC