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

BeginScope: support for non-serializable objects + performance improvement (20-50% cpu, 35% times less allocations) #232

Merged
merged 13 commits into from
Aug 3, 2018

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Jul 25, 2018

Improves the support of custom objects together with BeginScope, when using AppDomains (Using CallContext that requires serializable objects)

Ex. ActionLogScope. Attempt to resolve NLog/NLog#2812

Includes a small performance boost in handling of Microsofts internal ActionLogScope + HostingLogScope + FormattedLogValue by using IReadOnlyList in the fast-path of BeginScope. (Still supports Arrays and Lists), so it doesn't allocate yield-enumerator or uses keyvaluepair-property-extractor.

Includes a small performance boost by reusing a shared allocation of NLogBeginScopeParser (lives on the NLogLoggerProvider) for all NLogLogger-objects.

Includes a small performance boost in handling of log-message with no message-properties, by skipping allocation of message-property parser.

Includes a new IncludeScopes option (similar to the one for the Microsoft ConsoleLogger), that allows one to disable the overhead of the BeginScope handling (Skips parsing state and maintaining NDLC + MDLC)

Includes a small performance boost when using CaptureMessageProperties = true, but with CaptureMessageTemplates = false. Then it skips extra parsing of message properties to see if making use of message-template-syntax (or positional format args)

Improves the handling of BeginScope("Hello {world}", "Earth") so it only includes the actually property (and not the original beginscope-message-template).

Improves the behavior of BeginScope so the returned IDisposable has ToString() that returns input-state ToString().

@codecov
Copy link

codecov bot commented Jul 25, 2018

Codecov Report

Merging #232 into master will increase coverage by 0.43%.
The diff coverage is 69.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #232      +/-   ##
==========================================
+ Coverage   71.39%   71.82%   +0.43%     
==========================================
  Files           6        7       +1     
  Lines         374      433      +59     
  Branches       88      106      +18     
==========================================
+ Hits          267      311      +44     
- Misses         72       80       +8     
- Partials       35       42       +7
Impacted Files Coverage Δ
...og.Extensions.Logging/Logging/NLogLoggerFactory.cs 0% <ø> (ø) ⬆️
...g.Extensions.Logging/Logging/NLogLoggerProvider.cs 82.5% <33.33%> (-11.79%) ⬇️
...tensions.Logging/Extensions/ConfigureExtensions.cs 18.18% <50%> (-8.49%) ⬇️
....Extensions.Logging/Logging/NLogProviderOptions.cs 93.33% <66.66%> (+1.02%) ⬆️
...Extensions.Logging/Logging/NLogBeginScopeParser.cs 67% <67%> (ø)
...nsions.Logging/Logging/NLogMessageParameterList.cs 73.49% <75%> (+0.89%) ⬆️
src/NLog.Extensions.Logging/Logging/NLogLogger.cs 77.45% <76.11%> (+4.76%) ⬆️

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 7c08a84...c2c50c9. Read the comment docs.

…lection across all NLogLogger-objects (Reduce allocation and better cache reuse).
@snakefoot
Copy link
Contributor Author

snakefoot commented Jul 26, 2018

Simple test with the following code (Pure NetCore-console, no Asp.Net.Core)

static readonly _messageDefine = LoggerMessage.Define<int, int>(LogLevel.Information, default(EventId), "Log message {number} / {total}");
using (_logger.BeginScope("Hello {World}", "Earth"))
{
   _messageDefine(_logger, id, totalCount, null);
}
Logger Name Messages Size Args Threads Loggers
Default Layout 5.000.000 12 2 2 1
Test Name Msgs/sec CPU (ms) Alloc (MB)
This PR IncludeScopes=false + CaptureMessageTemplates=false 393944 21,6 4.616,1
This PR IncludeScopes=false 383808 24,1 4.780,9
This PR IncludeScopes=true 358684 26,5 8.575,6
Master 309062 31,5 11.556,1
Serilog 2.7.1 185459 50,7 20.889

@304NotModified 304NotModified changed the title BeginScope with support for non-serializable objects BeginScope: support for non-serializable objects + performance improvement (20-50% cpu, 35% times less allocations) Aug 1, 2018
@304NotModified
Copy link
Member

@snakefoot could you please check the better code hub results? (don;t know if you could reed the results, here as image:)

image

image

Thanks!

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.

(please check better code hub)

@snakefoot
Copy link
Contributor Author

@304NotModified Think you need to post some more screenshots. Or commit some changes to this PR that fixes the "problems".

@304NotModified
Copy link
Member

only "NLogLogger.CreateLogEventInfo" needs to be fixed, it has 11 branch points, and the it should be <10.

So we need a helper method for that

@snakefoot
Copy link
Contributor Author

snakefoot commented Aug 2, 2018 via email

@304NotModified
Copy link
Member

You are very welcome implement such a method.

I can't push on your master AFAIK. (it works on other branches)

@304NotModified
Copy link
Member

304NotModified commented Aug 2, 2018

! [remote rejected] snakefoot-master -> snakefoot-master (permission denied)
error: failed to push some refs to 'https://github.com/snakefoot/NLog.Extensions.Logging.git'

I think your master is protected. Any way, will make a PR then

@304NotModified
Copy link
Member

-- > snakefoot#5

@snakefoot snakefoot force-pushed the master branch 2 times, most recently from b86da07 to 6ace912 Compare August 2, 2018 21:49
@snakefoot
Copy link
Contributor Author

@304NotModified Have now tried to reduce the complexity even further. But still it complains. Where is the pain?

@304NotModified
Copy link
Member

in at least TryParseMessageTemplate, see snakefoot#6

@304NotModified
Copy link
Member

304NotModified commented Aug 2, 2018

I think we have to move some code in the future, e.g. to c# extensions

I like refactoring, so I have no problem is doing that

@304NotModified
Copy link
Member

but you can't check the report at all? That's strange.

@304NotModified
Copy link
Member

image

🥇

@304NotModified 304NotModified merged commit fda6a7a into NLog:master Aug 3, 2018
@304NotModified
Copy link
Member

merged, thanks also for refactoring!

@snakefoot
Copy link
Contributor Author

Found some stuff to cleanup: #233

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.

NLog +asp.net core + net 472 + rdlc: error generating a report
2 participants