[Wasmtime] Component model: Quenstion about floating point canonicalization

[kaivol]

I noticed a significant performance penalty when returning list<f32> from WebAssembly components.
By overriding/specializing Lift::load_list for Vec<f32> (similar to the implementation for integers), I was able to achieve significantly better performance (I will make an issue or PR about this when I find the time).

While doing so I wondered whether it is necessary to canonicalize floating point numbers lifted/loaded from the WebAssembly runtime, as seen here:

wasmtime/crates/wasmtime/src/runtime/component/func/typed.rs

Lines 932 to 945 in 128decd

	unsafe impl Lift for $float {
	#[inline]
	fn lift(_cx: &mut LiftContext<'_>, ty: InterfaceType, src: &Self::Lower) -> Result<Self> {
	debug_assert!(matches!(ty, InterfaceType::$ty));
	Ok(canonicalize($float::from_bits(src.$get_float())))
	}
	#[inline]
	fn load(_cx: &mut LiftContext<'_>, ty: InterfaceType, bytes: &[u8]) -> Result<Self> {
	debug_assert!(matches!(ty, InterfaceType::$ty));
	debug_assert!((bytes.as_ptr() as usize) % Self::SIZE32 == 0);
	Ok(canonicalize($float::from_le_bytes(bytes.try_into().unwrap())))
	}
	}

I understand that (in the Component Model) f32 and f64 logically only have a single NaN value.
Is it correct that for this reason we must ensure that we only pass the canonical NaN to components?

Anyways I don't quite understand why this should be necessary for values passed from components to the host, as Rust doesn't really make any promises about the bit patterns of NaN.
Also, shouldn't all floating point numbers coming from a component already be provided in canonical form, making another canonicalization redundant?

I would be delighted if someone could enlighten me about this.

[tschneidereit]

Good catch! This canonicalization used to be required by the specification, but that requirement has since been removed—precisely over the performance concerns you raise here. Seems like we never updated Wasmtime to match the spec change.

@alexcrichton, I assume removing the canonicalization here is fairly straight-forward?

[kaivol]

Thanks for the explanation!

[alexcrichton]

Ah yes this is an artifact of the original implementation. @kaivol would you be interested in sending a PR for this? It in theory is as easy as "just remove canonicalize" I think