Skip to content

[.NET] Avoid heap alloc when using StringNames as key in a Dictionary #106133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fLindahl
Copy link

@fLindahl fLindahl commented May 6, 2025

StringNames in Godot C# are IEquatable, and overrides GetHashCode(), which calls godot_string_name.movable's version of that method, which was not overridden. This led to heap allocations when e.g. calling the indexer in a Dictionary with StringName type as Key.

This PR just adds the (assuming) missing method, which seems to fix the issue.

@fLindahl fLindahl requested a review from a team as a code owner May 6, 2025 19:45
@clayjohn clayjohn added this to the 4.5 milestone May 6, 2025
@clayjohn clayjohn requested a review from raulsntos May 6, 2025 20:56
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contributing to the .NET module!

The movable types are only meant to be used to store them in classes (like StringName) where we can't store ref structs. None of them implement any API other than for conversion. So I think it was intentional to not implement GetHashCode.

However, as you noticed, we likely meant to call the godot_string_name.GetHashCode method (which is implemented) from the StringName.GetHashCode implementation. So the fix should be something like this:

diff --git a/modules/mono/glue/GodotSharp/GodotSharp/Core/StringName.cs b/modules/mono/glue/GodotSharp/GodotSharp/Core/StringName.cs
index 21d9ada127..a28230636f 100644
--- a/modules/mono/glue/GodotSharp/GodotSharp/Core/StringName.cs
+++ b/modules/mono/glue/GodotSharp/GodotSharp/Core/StringName.cs
@@ -161,7 +161,7 @@ namespace Godot
 
         public override int GetHashCode()
         {
-            return NativeValue.GetHashCode();
+            return NativeValue.DangerousSelfRef.GetHashCode();
         }
     }
 }

@fLindahl
Copy link
Author

fLindahl commented May 7, 2025

The movable types are only meant to be used to store them in classes (like StringName) where we can't store ref structs. None of them implement any API other than for conversion. So I think it was intentional to not implement GetHashCode.

That makes sense! I removed the GetHashCode method from movable and changed StringName to use the existing version in godot_string_name instead. Thanks!

@fLindahl fLindahl requested a review from raulsntos May 7, 2025 16:31
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks!

@clayjohn
Copy link
Member

clayjohn commented May 7, 2025

Looks great! The final step before merging is to squash all the commits together so that the whole PR only contains 1 big commit with all your changes. We like to merge one commit at a time to keep the git history clean and navigable.

If you don't know how to do that, we have a helpful tutorial in the official documentation https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#the-interactive-rebase

…ction.Dictionary.

Changed StringName GetHashCode to call godot_string_name.GetHashCode instead of godot_string_name's (which was not overridden) as this otherwise leads to heap allocations when e.g. calling the indexer in a Dictionary with `StringName` type as Key.
@fLindahl fLindahl force-pushed the stringname_movable_dict_fix branch from 8e3946c to 01056f3 Compare May 8, 2025 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants