Skip to content
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

NLogProviderOptions with support for CaptureEventId #538

Merged
merged 1 commit into from
Oct 16, 2021

Conversation

snakefoot
Copy link
Contributor

Follow up to #319. Better granularity of how to capture EventId. See also #537

@snakefoot
Copy link
Contributor Author

Tempted to merge EventId-property-capture into NLogMessageParameterList, as it would reduce memory-allocation even more.

@codecov-commenter
Copy link

codecov-commenter commented Oct 16, 2021

Codecov Report

Merging #538 (45c0509) into master (1d8f06a) will decrease coverage by 0.20%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #538      +/-   ##
==========================================
- Coverage   80.87%   80.66%   -0.21%     
==========================================
  Files          17       17              
  Lines        1443     1438       -5     
  Branches      236      237       +1     
==========================================
- Hits         1167     1160       -7     
  Misses        171      171              
- Partials      105      107       +2     
Impacted Files Coverage Δ
...nsions.Logging/Logging/NLogMessageParameterList.cs 91.30% <0.00%> (-0.07%) ⬇️
src/NLog.Extensions.Logging/Logging/NLogLogger.cs 80.82% <50.00%> (-0.94%) ⬇️
....Extensions.Logging/Logging/NLogProviderOptions.cs 94.44% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d8f06a...45c0509. Read the comment docs.

@304NotModified
Copy link
Member

Tempted to merge EventId-property-capture into NLogMessageParameterList, as it would reduce memory-allocation even more.

Sounds good :)

@snakefoot snakefoot merged commit e006c89 into NLog:master Oct 16, 2021
@STeeL835
Copy link

STeeL835 commented Oct 17, 2021

If this capture selection logic exists for performance, is it really faster to do all this than just to capture EventId struct?

if (@event != default)
    properties["EventId"] = @event;

I've run a benchmark and seems like simple capturing could be faster. It consistently allocates 32B for a single eventId but is also consistently faster, while current logic can either save allocation completely in case of separate properties and pre-generated eventId, or allocate even more if user chooses to log eventId as struct

@snakefoot
Copy link
Contributor Author

In my world, then memory-allocation / boxing is always slow. And you can performs lots cpu-instructions for the price of a single allocation.

@STeeL835
Copy link

I agree that boxing is an expensive operation, Microsoft says that it can take up to 20 times longer than a reference assignment. My concern is that the optimization code here can also be expensive relative to a boxing operation.

Benchmark, (if it is correct), confirms it - 23ns boxing vs 30-40ns optimization. This difference could also be a result of compile-time/JIT optimization though, which is why I'm not sure about my benchmark, you know more about the code and can write a correct one to check this theory.

@snakefoot
Copy link
Contributor Author

snakefoot commented Oct 26, 2021

Have tried to reduce the code-complexity with #539 and #541 to reduce cpu-branching. You are ofcourse very welcome to create pull-requests and contribute with your own suggestions.

@snakefoot
Copy link
Contributor Author

snakefoot commented Oct 29, 2021

I'm guessing your benchmark is comparing "time to insert single eventid" vs. "time to insert two properties". And your benchmark shows that string-hashcode and dictionary-insert is the most expensive part (Notice that the longer "hundred" is more expensive than the shorter "two")

Happy that the change to enum, now allows you to perform single insert (or completely skip it)

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.

None yet

4 participants