Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Move logging to new style (remove allocs) #326

Merged
merged 1 commit into from
Nov 13, 2015

Conversation

benaadams
Copy link
Contributor

and additionally boxing param issue shown below:

img

Takes allocations for logging calls to zero

@rynowak
Copy link
Member

rynowak commented Nov 3, 2015

@benaadams
Copy link
Contributor Author

That's too weird for me :)

Raised issue #327 for it instead

@benaadams
Copy link
Contributor Author

Actually, quite like it, updated.

Resolves #327

@rynowak
Copy link
Member

rynowak commented Nov 3, 2015

The pattern for this resolves a few perf issues by doing things like avoiding params object[] on both paths, and pre-parsing and caching the format string for you.

If you need to do significant work before calling the delegate, do a manual IsEnabled check.

We've measured this and verified that it's allocation free when logging is off, and allocates one object when logging is on. We might take it further in the next milestone to get rid of that allocation when logging is on as well.

@benaadams benaadams changed the title Check logging is enabled before creating params array Move logging to new style Nov 3, 2015
_connectionWriteFin = LoggerMessage.Define<long>(LogLevel.Debug, 7, @"Connection id ""{ConnectionId}"" sending FIN.");
_connectionWroteFin = LoggerMessage.Define<long, int>(LogLevel.Debug, 8, @"Connection id ""{ConnectionId}"" sent FIN with status ""{Status}"".");
_connectionKeepAlive = LoggerMessage.Define<long>(LogLevel.Debug, 9, @"Connection id ""{ConnectionId}"" completed keep alive response.");
_connectionDisconnect = LoggerMessage.Define<long>(LogLevel.Error, 10, @"Connection id ""{ConnectionId}"" disconnected.");
Copy link
Member

Choose a reason for hiding this comment

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

Should ApplicationError also use this patern?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd imagine so; but would need the following overload (as Exception is is a built in param)?

static Action<ILogger, Exception> Define(LogLevel logLevel, int eventId, string formatString)

in Microsoft.Extensions.Logging.Abstractions/LoggerMessage.cs which doesn't currently exist.

@benaadams benaadams force-pushed the reduce-log-boxing branch 16 times, most recently from 6d792bb to 98dcd71 Compare November 8, 2015 16:20
@benaadams benaadams changed the title Move logging to new style Move logging to new style (remove allocs) Nov 8, 2015
@benaadams benaadams force-pushed the reduce-log-boxing branch 3 times, most recently from e482ddf to 69e15c7 Compare November 10, 2015 08:28
@benaadams benaadams force-pushed the reduce-log-boxing branch 2 times, most recently from d1954f4 to c0f8276 Compare November 10, 2015 10:43
@halter73 halter73 merged commit c0f8276 into aspnet:dev Nov 13, 2015
@benaadams benaadams deleted the reduce-log-boxing branch November 13, 2015 06:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants