Skip to content

Conversation

@snakefoot
Copy link
Contributor

No description provided.

@coderabbitai
Copy link

coderabbitai bot commented Nov 22, 2025

Walkthrough

Replaces 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

Cohort / File(s) Summary
Internal property handling refactor
src/NLog/Internal/PropertiesDictionary.cs
Replaced private property EventProperties with method GetEventProperties(bool prepareForInsert = false). Added InitializeEventPropertiesDictionary(IList<MessageTemplateParameter>?, bool) to create the internal dictionary with optional extra capacity when inserts are expected. Updated indexer, Add, Contains, Remove, TryGetValue, enumeration, and other internal call sites to use GetEventProperties(...). Capacity is increased (messageProperties.Count + 2) when prepareForInsert is true. Public API remains unchanged.

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)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Check thread-safe lazy initialization and potential races in GetEventProperties
  • Verify prepareForInsert is passed at all call sites that anticipate inserts
  • Review InitializeEventPropertiesDictionary capacity calculation and edge cases (null/empty message parameters)
  • Ensure TryGetValue/enumeration branches correctly reference the new path and preserve previous behavior

Poem

🐰 I swapped a property for a nimble call,
I sized the holes before the carrots fall,
Lazy hops save rehash and time,
Dictionary snug — the fields align.
🥕✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Description check ❓ Inconclusive No description was provided by the author, making it impossible to assess whether a description relates to the changeset. Add a pull request description explaining the motivation for increasing initial capacity and the performance benefits of this change.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: refactoring to increase initial capacity of the PropertiesDictionary when inserting properties.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a 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 nit

The new GetEventProperties(bool prepareForInsert = false) plus InitalizeEventPropertiesDictionary(...) wiring looks correct:

  • Interlocked.CompareExchange ensures _eventProperties is only initialized once, and discarded dictionaries from racing threads can’t overwrite an already-populated instance.
  • prepareForInsert is only used at real insert sites (this[key] setter and Add), so the Count + 2 capacity hint is applied exactly when extra headroom is needed.
  • Read-only paths (TryGetValue via TryLookupMessagePropertyValue when Count > 10) route through GetEventProperties() 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 of GetEventProperties preserves semantics; possible micro-optimization

The updated Contains(KeyValuePair<...>), Remove overloads, and ValueCollection.Contains all correctly route through GetEventProperties, keeping message-template and event properties unified in the backing dictionary while respecting IsMessageProperty flags. The guards using IsEmpty and (_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 _eventProperties is still null and _messageProperties is small (e.g., Count <= 10), you could scan _messageProperties directly instead of materializing the dictionary via GetEventProperties(). That would avoid creating the dictionary for rare Contains calls 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

📥 Commits

Reviewing files that changed from the base of the PR and between 88dece1 and 2b4b226.

📒 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)

Copy link

@coderabbitai coderabbitai bot left a 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 +2 buffer when prepareForInsert is 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

📥 Commits

Reviewing files that changed from the base of the PR and between 2b4b226 and f387482.

📒 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.CompareExchange ensures 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 Add method (line 216) properly call GetEventProperties(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, and TryLookupMessagePropertyValue appropriately use prepareForInsert=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.TryGetValue at 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.

@sonarqubecloud
Copy link

@snakefoot snakefoot merged commit ee0690e into NLog:dev Nov 22, 2025
5 of 6 checks passed
@snakefoot snakefoot added this to the 6.0.7 milestone Nov 30, 2025
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.

1 participant