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

NLogLogger - Reduce allocation by not boxing EventId when CaptureEntireEventId = false #319

Merged
merged 1 commit into from
Aug 26, 2021

Conversation

snakefoot
Copy link
Contributor

Resolves #318

@codecov-io
Copy link

codecov-io commented Aug 20, 2019

Codecov Report

Merging #319 into master will increase coverage by 0.07%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #319      +/-   ##
=========================================
+ Coverage   81.82%   81.9%   +0.07%     
=========================================
  Files          12      12              
  Lines        1150    1166      +16     
  Branches      195     198       +3     
=========================================
+ Hits          941     955      +14     
- Misses        140     141       +1     
- Partials       69      70       +1
Impacted Files Coverage Δ
....Extensions.Logging/Logging/NLogProviderOptions.cs 100% <100%> (ø) ⬆️
src/NLog.Extensions.Logging/Logging/NLogLogger.cs 80.55% <72.22%> (+0.26%) ⬆️

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 f7ee149...548570c. Read the comment docs.

@snakefoot snakefoot force-pushed the master branch 2 times, most recently from 952ba47 to e22a191 Compare August 20, 2019 22:40
@304NotModified
Copy link
Member

this is a behavior change, isn't?

@snakefoot
Copy link
Contributor Author

snakefoot commented Aug 24, 2019

this is a behavior change, isn't?

Yes. Instead of having 3 EventId-properties to store Id + Name, then it now only uses 2.

I guess we could wait with this PR until NLog 5.0 has been released, then we can bump this to ver. 2.0

@304NotModified
Copy link
Member

Good plan!

@304NotModified 304NotModified added this to the 2.0 milestone Aug 26, 2019
@304NotModified 304NotModified changed the title NLogLogger - Reduce allocation by not boxing EventId when CaptureEntireEventId = false NLogLogger - Reduce allocation by not boxing EventId when CaptureEntireEventId = false (WIP) Sep 21, 2019
@304NotModified
Copy link
Member

(WIP) for 2.0 release

Copy link
Member

@304NotModified 304NotModified left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@codecov-commenter
Copy link

Codecov Report

Merging #319 into master will decrease coverage by 0.31%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #319      +/-   ##
==========================================
- Coverage   82.22%   81.90%   -0.32%     
==========================================
  Files          14       12       -2     
  Lines        1198     1166      -32     
  Branches      200      198       -2     
==========================================
- Hits          985      955      -30     
  Misses        141      141              
+ Partials       72       70       -2     
Impacted Files Coverage Δ
src/NLog.Extensions.Logging/Logging/NLogLogger.cs 80.55% <72.22%> (+0.05%) ⬆️
....Extensions.Logging/Logging/NLogProviderOptions.cs 100.00% <100.00%> (ø)
...og.Extensions.Logging/Logging/NLogLoggerFactory.cs 84.61% <0.00%> (-3.62%) ⬇️
...tensions.Logging/Extensions/ConfigureExtensions.cs 54.71% <0.00%> (-1.65%) ⬇️
...ensions.Logging/Config/NLogLoggingConfiguration.cs 91.98% <0.00%> (-0.37%) ⬇️
...xtensions.Logging/Config/SetupBuilderExtensions.cs
...Logging/Config/SetupExtensionsBuilderExtensions.cs
...g.Extensions.Logging/Logging/NLogLoggerProvider.cs 97.18% <0.00%> (+1.94%) ⬆️

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 06574d4...b565946. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Jan 1, 2021

Codecov Report

Merging #319 (d4c23c0) into master (6defbc2) will increase coverage by 0.06%.
The diff coverage is 73.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #319      +/-   ##
==========================================
+ Coverage   81.41%   81.48%   +0.06%     
==========================================
  Files          14       14              
  Lines        1227     1242      +15     
  Branches      204      207       +3     
==========================================
+ Hits          999     1012      +13     
- Misses        154      155       +1     
- Partials       74       75       +1     
Impacted Files Coverage Δ
src/NLog.Extensions.Logging/Logging/NLogLogger.cs 80.75% <72.22%> (+0.25%) ⬆️
....Extensions.Logging/Logging/NLogProviderOptions.cs 100.00% <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 6defbc2...d4c23c0. Read the comment docs.

@snakefoot snakefoot force-pushed the master branch 2 times, most recently from 3c7531a to d4c23c0 Compare January 2, 2021 01:41
@snakefoot snakefoot changed the title NLogLogger - Reduce allocation by not boxing EventId when CaptureEntireEventId = false (WIP) NLogLogger - Reduce allocation by not boxing EventId when CaptureEntireEventId = false Aug 25, 2021
@snakefoot snakefoot marked this pull request as ready for review August 26, 2021 17:18
@snakefoot snakefoot merged commit 996fc65 into NLog:master Aug 26, 2021
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.

Remove capture of EventId to reduce constant boxing
4 participants