Cache StringNames & NodePaths#1175
Conversation
WalkthroughBoth NodePath and StringName classes are enhanced with static ConcurrentDictionary-based caching to store weak references, enabling reuse of instances for identical input strings. Changes include: readonly modification to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@modules/mono/glue/GodotSharp/GodotSharp/Core/NodePath.cs`:
- Around line 177-192: Cached weak references in _nodePathCache for NodePath can
become dead and remain in the dictionary, causing repeated cache misses; modify
the lookup in the NodePath creation code: when _nodePathCache.TryGetValue(from,
out var cachedWeak) returns true but cachedWeak.TryGetTarget(out _) returns
false, remove the stale entry (e.g., _nodePathCache.TryRemove(from, out _))
before creating a new NodePath, and then add the new
nodePath._weakReferenceToSelf via _nodePathCache.TryAdd(from,
nodePath._weakReferenceToSelf); update the code paths that reference
TryGetValue, TryGetTarget, TryRemove/TryAdd, and _weakReferenceToSelf
accordingly to ensure thread-safe replacement of dead weak refs.
In `@modules/mono/glue/GodotSharp/GodotSharp/Core/StringName.cs`:
- Around line 118-134: The cache path in StringName.Create (where
_stringNameCache.TryGetValue is used) leaves dead WeakReference entries in
_stringNameCache when cachedStringNameWeakReference.TryGetTarget returns false;
before creating and TryAdd'ing the new StringName(from) instance, remove the
stale entry from _stringNameCache (e.g., call _stringNameCache.TryRemove(from,
out _)) so the subsequent TryAdd using stringName._weakReferenceToSelf can
succeed; ensure this change touches the block around
_stringNameCache.TryGetValue, TryGetTarget, the creation of new StringName, and
the TryAdd call.
Arctis-Fireblight
left a comment
There was a problem hiding this comment.
Thank you for your work on this!
I have gone ahead and marked this as approved for merge.
I'll be cutting our 26.1-stable this week, and this will be merged shortly afterwards for inclusion in 26.2.
Cherry-picked from my pull request in godotengine/godot at the request of @Arctis-Fireblight.
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.