Skip to content

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

Merged
jsturtevant merged 3 commits into
bytecodealliance:mainfrom
kpreisser:revert-extern-cache-changes
Jul 5, 2024
Merged

Revert the changes to the cache of externs in Store#323
jsturtevant merged 3 commits into
bytecodealliance:mainfrom
kpreisser:revert-extern-cache-changes

Conversation

@kpreisser
Copy link
Copy Markdown
Contributor

@kpreisser kpreisser commented Jul 1, 2024

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.

kpreisser added 3 commits July 1, 2024 09:10
…nd `Global` objects in the `Store`, which avoids having to explicitly accessing the `__private` field (because the whole struct is now compared)."

This reverts commit f4e6e57.
Comment thread src/Store.cs
@jsturtevant
Copy link
Copy Markdown
Contributor

Revert the changes to the cache of externs in Store from #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
Copy link
Copy Markdown
Contributor Author

kpreisser commented Jul 2, 2024

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!

@jsturtevant jsturtevant merged commit a55218a into bytecodealliance:main Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants