search

bytecodealliance / wasmtime-dotnet

.NET embedding of Wasmtime https://bytecodealliance.github.io/wasmtime-dotnet/
Apache License 2.0
409 stars 52 forks source link

Revert the changes to the cache of externs in `Store` #323

Closed kpreisser closed 2 months ago

kpreisser commented 2 months ago

Revert the changes to the cache of externs in Store from #318, as those would have negative performance impact.

When looking up a value in the dictionary, it would use the Equals(object) method on the struct which causes the struct to be boxed. Additionally, the default ValueType.GetHashCode() apparently doesn't incorporate the value of [U]IntPtr/n[u]int fields, so we would also need to implement GetHashCode() in the Extern... structs.

Instead, we use a ValueTuple as key consisting of the extern type and the the both struct fields, like it was done previously (ValueTuple<...> implements IEquatable<T> to avoid boxing, and uses all elements for calculating the hash code).

It should be Ok to access the __private field of the Extern... structs, because we don't interpret its value in any way, and just use it to compare the value to other struct instances.

jsturtevant commented 2 months ago

Revert the changes to the cache of externs in Store from https://github.com/bytecodealliance/wasmtime-dotnet/pull/318, as those would have negative performance impact.

Curious how you caught the perf impact? Is there anything we can add to the project that would catch this in CI?

kpreisser commented 2 months ago

Curious how you caught the perf impact?

After submitting the PR, I wasn't sure how the default GetHashCode() implementation on structs (ValueType) actually works, and found that e.g. with the ExternFunc type it would only take the first field (ulong store) into account, but not the second field (nuint __private), which would "destroy" performance of the Dictionary<TKey, TValue> as it depends on different hash codes for O(1) lookups. For this, we would need to override GetHashCode in the structs.

However, then I took a look at how the equality check is performed by default for value types, and found that EqualityComparer<T>.Default returns an ObjectEqualityComparer when T is ValueType (if it doesn't explicitly implement IEquatable<T>), which just calls the Equals(object) method. However this means the struct value may be boxed when lookup is performed, and we want to avoid struct boxing as much as possible in .NET, as this causes GC load that needs to track such objects (e.g., see #287, #202). For example, this can be a problem with performance-sensitive applications such as games (Unity).

Thanks!