PropertiesDictionary - Faster TryGetValue without IsEmpty #6052
Conversation
WalkthroughRefactoring of the TryGetValue method in PropertiesDictionary.cs to simplify control flow. The change replaces an IsEmpty guard with a null check on _eventProperties, consolidating lookup logic into two distinct paths: one delegating to TryLookupMessagePropertyValue when null, and another performing direct dictionary lookup when available. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (1)
src/NLog/Internal/PropertiesDictionary.cs (1)
315-330: TryGetValue refactor looks correct and aligns with dictionary invariantsThe new branching on
_eventPropertiesbeing null cleanly separates the “message-properties-only” path from the dictionary-backed path and keeps semantics consistent with howGetEventProperties,Remove, and the enumerator treat the dictionary as canonical once it exists. I don’t see any functional regressions; this should also avoid redundant checks and prevent ever falling back to message-property scanning after a key has been removed from the dictionary.If you want a tiny micro-optimization, you could cache
_eventPropertiesinto a local (var eventProperties = _eventProperties;) and use that for the null-check andTryGetValueto avoid a double field read, but that’s purely optional.
|
No description provided.