-
Notifications
You must be signed in to change notification settings - Fork 1.4k
PropertiesDictionary - Increase initial capacity when insert #6048
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
Conversation
WalkthroughReplaces the private EventProperties property with GetEventProperties(bool prepareForInsert = false), adds InitializeEventPropertiesDictionary(IList?, bool) to lazily create the internal dictionary with an optional capacity hint, and updates all internal reads/writes to use the new method path. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Props as PropertiesDictionary
participant Init as InitializeEventPropertiesDictionary
Note over Caller,Props: Read or write operation on event properties
Caller->>Props: GetEventProperties(prepareForInsert?)
alt _eventProperties == null
Props->>Init: InitializeEventPropertiesDictionary(messageParams?, prepareForInsert)
Init-->>Props: new Dictionary (capacity = base or messageParams.Count + 2)
end
Props-->>Caller: IDictionary reference
Caller->>Props: perform read/write (indexer/Add/Contains/Remove/TryGetValue)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/NLog/Internal/PropertiesDictionary.cs (2)
109-116: Lazy init + insert-aware capacity look solid; minor naming nitThe new
GetEventProperties(bool prepareForInsert = false)plusInitalizeEventPropertiesDictionary(...)wiring looks correct:
Interlocked.CompareExchangeensures_eventPropertiesis only initialized once, and discarded dictionaries from racing threads can’t overwrite an already-populated instance.prepareForInsertis only used at real insert sites (this[key]setter andAdd), so theCount + 2capacity hint is applied exactly when extra headroom is needed.- Read-only paths (
TryGetValueviaTryLookupMessagePropertyValuewhenCount > 10) route throughGetEventProperties()without the insert hint, which keeps capacity tight for lookups.Only nit: the helper name is misspelled (
InitalizeEventPropertiesDictionary), which makes it harder to search for and slightly hurts readability. Consider renaming while this code is fresh:- private static Dictionary<object, PropertyValue> InitalizeEventPropertiesDictionary(IList<MessageTemplateParameter>? messageProperties, bool prepareForInsert) + private static Dictionary<object, PropertyValue> InitializeEventPropertiesDictionary(IList<MessageTemplateParameter>? messageProperties, bool prepareForInsert) @@ - System.Threading.Interlocked.CompareExchange(ref _eventProperties, InitalizeEventPropertiesDictionary(_messageProperties, prepareForInsert), null); + System.Threading.Interlocked.CompareExchange(ref _eventProperties, InitializeEventPropertiesDictionary(_messageProperties, prepareForInsert), null);Also applies to: 167-180, 194-217, 324-325, 345-349
235-247: Contains/Remove/ValueCollection usage ofGetEventPropertiespreserves semantics; possible micro-optimizationThe updated
Contains(KeyValuePair<...>),Removeoverloads, andValueCollection.Containsall correctly route throughGetEventProperties, keeping message-template and event properties unified in the backing dictionary while respectingIsMessagePropertyflags. The guards usingIsEmptyand(_eventProperties != null || ContainsKey(...))avoid unnecessary work when there are truly no properties.If you ever want to shave a few allocations, a small optional optimization would be:
- In places like
ValueCollection.Contains, when_eventPropertiesis stillnulland_messagePropertiesis small (e.g.,Count <= 10), you could scan_messagePropertiesdirectly instead of materializing the dictionary viaGetEventProperties(). That would avoid creating the dictionary for rareContainscalls in tiny message-only cases.Not required for correctness; current behavior is consistent and clear.
Also applies to: 289-312, 604-616
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/NLog/Internal/PropertiesDictionary.cs(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/NLog/Internal/PropertiesDictionary.cs (2)
src/NLog/Internal/ObjectReflectionCache.cs (3)
PropertyValue(294-299)PropertyValue(301-313)PropertyValue(315-327)src/NLog/MessageTemplates/MessageTemplateParameter.cs (2)
MessageTemplateParameter(105-111)MessageTemplateParameter(120-126)
2b4b226 to
f387482
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/NLog/Internal/PropertiesDictionary.cs (1)
167-180: Consider documenting the capacity buffer.The
+2buffer whenprepareForInsertis true reduces rehashing on subsequent inserts. However, this magic number lacks explanation.Consider adding a comment or named constant to clarify the rationale:
private static Dictionary<object, PropertyValue> InitializeEventPropertiesDictionary(IList<MessageTemplateParameter>? messageProperties, bool prepareForInsert) { if (messageProperties?.Count > 0) { - var dictionaryCapacity = prepareForInsert ? (messageProperties.Count + 2) : messageProperties.Count; + // Reserve extra capacity to reduce rehashing when adding event properties alongside message properties + const int ExtraCapacityForInserts = 2; + var dictionaryCapacity = prepareForInsert ? (messageProperties.Count + ExtraCapacityForInserts) : messageProperties.Count; var eventProperties = new Dictionary<object, PropertyValue>(dictionaryCapacity, PropertyKeyComparer.Default);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/NLog/Internal/PropertiesDictionary.cs(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/NLog/Internal/PropertiesDictionary.cs (3)
src/NLog/Internal/ObjectReflectionCache.cs (4)
Dictionary(558-570)PropertyValue(294-299)PropertyValue(301-313)PropertyValue(315-327)src/NLog/Internal/ThreadSafeDictionary.cs (8)
Dictionary(205-216)Add(88-94)Add(96-102)IDictionary(218-238)Contains(112-115)Remove(140-146)Remove(148-154)TryGetValue(156-159)src/NLog/MessageTemplates/MessageTemplateParameter.cs (2)
MessageTemplateParameter(105-111)MessageTemplateParameter(120-126)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: task-list-completed
🔇 Additional comments (4)
src/NLog/Internal/PropertiesDictionary.cs (4)
109-116: LGTM! Thread-safe lazy initialization is correctly implemented.The use of
Interlocked.CompareExchangeensures thread-safe initialization of_eventProperties. Multiple threads may create competing dictionary instances, but only one will be retained, which is an acceptable trade-off for lock-free lazy initialization.
194-194: LGTM! Write operations correctly use capacity optimization.Both the indexer setter (line 194) and
Addmethod (line 216) properly callGetEventProperties(true)to pre-allocate extra capacity, reducing rehashing when properties are added.Also applies to: 216-216
239-244: LGTM! Read operations correctly use default (non-optimized) initialization.Read-only operations like
Contains,Remove, andTryLookupMessagePropertyValueappropriately useprepareForInsert=false(default), avoiding over-allocation when the dictionary is lazily initialized for lookup purposes rather than bulk inserts.Also applies to: 293-293, 303-308, 345-345, 608-613
319-328: LGTM! Direct field access is safe and efficient.The direct field access
_eventProperties.TryGetValueat line 324 is correct because it's guarded by the null check at line 319. This avoids unnecessary method call overhead in the read hot path.
|



No description provided.