LogEventInfo - Minor optimization of HasImmutableProperties #6047

snakefoot · 2025-11-22T13:30:58Z

No description provided.

coderabbitai · 2025-11-22T13:31:08Z

Warning

Rate limit exceeded

@snakefoot has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 2 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 8fd8029 and b079a12.

📒 Files selected for processing (3)

src/NLog/LogEventInfo.cs (6 hunks)
src/NLog/MessageTemplates/MessageTemplateParameters.cs (1 hunks)
tests/NLog.UnitTests/LoggerTests.cs (4 hunks)

Walkthrough

The changes refactor message template parameter handling by removing an internal MessageTemplateParameters constructor overload that accepted message and parameters, simplifying it to accept only template parameters. LogEventInfo is updated to align with this change by passing only properties when constructing MessageTemplateParameters, alongside variable renames and code clarity improvements.

Changes

Cohort / File(s)	Summary
LogEventInfo Message Template Handling `src/NLog/LogEventInfo.cs`	Renamed local variables for parameter parsing (`rebuildMessageTemplateParameters` → `parseMessageTemplateParameters`). Modified MessageTemplateParameters getter to construct with only properties when present instead of including message and parameters. Refactored HasImmutableProperties with local messageProperties variable for consistent access. Updated ResetFormattedMessage parameter naming. Added clarifying comment about parsing message-template when constructor input didn't provide it.
MessageTemplateParameters Constructor Simplification `src/NLog/MessageTemplates/MessageTemplateParameters.cs`	Removed constructor overload accepting `(templateParameters, message, parameters)`. Single-parameter constructor now accepts only `(templateParameters)`. IsValidTemplate forced to true regardless of parameter consistency; validation logic comparing lengths eliminated. IsPositional initialization removed from empty-parameters constructor path.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Constructor signature removal requires verification of all call sites in LogEventInfo
Changes to validation logic (forcing IsValidTemplate to true) and removed parameter consistency checks warrant careful examination
Impact of no longer initializing IsPositional in empty-parameters constructor path should be confirmed
Changes span internal APIs but affect core message template handling

Poem

🐰 A rabbit hops through template trails,
Simplifying paths where complexity fails,
One parameter now, no more, no less,
Cleaner constructors, a templated finesse! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	No pull request description was provided by the author, making it impossible to evaluate whether the description relates to the changeset.	Add a description explaining the purpose of these changes, including why the optimizations were made and any potential impacts on functionality or performance.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'LogEventInfo - Minor optimization of HasImmutableProperties' accurately describes the main change: optimizing the HasImmutableProperties method in LogEventInfo.cs, which is a key refactoring in this changeset.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/NLog/LogEventInfo.cs (1)

819-826: ResetFormattedMessage / ResetMessageTemplateParameters behavior and minor nit

Gating the HasMessageTemplateParameters call behind the parseMessageTemplateParameters flag in ResetFormattedMessage keeps the expensive “do we have template parameters?” check conditional and ensures _formattedMessage is cleared first when using an ILogMessageFormatter.

ResetMessageTemplateParameters’s final return (_properties.MessageProperties.Count == 0) nicely distinguishes between events that already carry message-template properties (where you don’t want to re-parse from the message) and those that only have non-template properties but should still be eligible for template parsing later.

If you expect LogEventInfo instances to have Message or Parameters mutated after both formatted message and message-properties have been generated, it might be worth re-validating that this flagging still gives the desired re-parse behavior in those edge cases; otherwise this looks fine for the common, one-shot logging flow. Also, the comment at Line 843 has a minor typo (“contructor-input” → “constructor-input”).

Also applies to: 832-845

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1b90d2a and 8fd8029.

📒 Files selected for processing (2)

src/NLog/LogEventInfo.cs (6 hunks)
src/NLog/MessageTemplates/MessageTemplateParameters.cs (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

src/NLog/LogEventInfo.cs (1)

src/NLog/MessageTemplates/MessageTemplateParameters.cs (3)

MessageTemplateParameters (45-198)

MessageTemplateParameters (81-94)

MessageTemplateParameters (99-103)

src/NLog/MessageTemplates/MessageTemplateParameters.cs (2)

src/NLog/MessageTemplates/MessageTemplateParameter.cs (2)

MessageTemplateParameter (105-111)

MessageTemplateParameter (120-126)

src/NLog/Internal/ArrayHelper.cs (1)

ArrayHelper (36-53)

🔇 Additional comments (4)

src/NLog/MessageTemplates/MessageTemplateParameters.cs (1)

99-103: Pre-parsed-parameters constructor behavior looks consistent

Using the provided IList<MessageTemplateParameter> directly and unconditionally marking IsValidTemplate = true matches the “already parsed” contract and avoids redundant validation; no issues from this change.

src/NLog/LogEventInfo.cs (3)

318-320: Message/Parameters setters: reset & parse flag usage is coherent

Both setters now compute a parseMessageTemplateParameters flag, then update the backing field and call ResetFormattedMessage(parseMessageTemplateParameters). This keeps message-template parsing and property reset in sync for both Message and Parameters without changing behavior.

Also applies to: 332-335

470-475: Using pre-existing MessageProperties to build MessageTemplateParameters is appropriate

Constructing MessageTemplateParameters directly from _properties.MessageProperties avoids re-parsing based on _message/_parameters when you already have pre-parsed parameters, which is both cheaper and semantically correct. The corresponding constructor setting IsValidTemplate = true for pre-parsed data aligns with this usage.

720-747: HasImmutableProperties micro-optimization preserves semantics

Caching properties.MessageProperties in a local and iterating it by index when properties.Count == messageProperties.Count eliminates an extra property lookup and the dictionary enumerator allocation while still checking every value with IsSafeToDeferFormatting. The fallback branch for mixed properties remains unchanged, so behavior is preserved with a minor perf win.

sonarqubecloud · 2025-11-22T13:57:06Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

pull-request-size bot added the size/M label Nov 22, 2025

coderabbitai bot reviewed Nov 22, 2025

View reviewed changes

LogEventInfo - Minor optimization of HasImmutableProperties

b079a12

snakefoot force-pushed the property_cleanup branch from 8fd8029 to b079a12 Compare November 22, 2025 13:49

snakefoot merged commit 88dece1 into NLog:dev Nov 22, 2025
6 checks passed

snakefoot added the performance label Nov 22, 2025

snakefoot added this to the 6.0.7 milestone Nov 30, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

LogEventInfo - Minor optimization of HasImmutableProperties #6047

LogEventInfo - Minor optimization of HasImmutableProperties #6047

snakefoot commented Nov 22, 2025

Uh oh!

coderabbitai bot commented Nov 22, 2025 •

edited

Loading

Rate limit exceeded

Uh oh!

coderabbitai bot left a comment

Uh oh!

sonarqubecloud bot commented Nov 22, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

LogEventInfo - Minor optimization of HasImmutableProperties #6047

LogEventInfo - Minor optimization of HasImmutableProperties #6047

Conversation

snakefoot commented Nov 22, 2025

Uh oh!

coderabbitai bot commented Nov 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rate limit exceeded

Walkthrough

Changes

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Nov 22, 2025

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai bot commented Nov 22, 2025 •

edited

Loading