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

Improved The Performance Of Logger #66895

Closed
wants to merge 3 commits into from
Closed

Improved The Performance Of Logger #66895

wants to merge 3 commits into from

Conversation

ProgrammingFire
Copy link

Updated LoggerExtensions.cs To Only Even Try To Log If The Log Level Is Enable To Improve Performance Of Logger By Saving Memory Allocation

Updated LoggerExtensions.cs To Only Even Try To Log If The Log Level If Enable To Improve Performance Of Logger By Saving Memory Allocation
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Mar 20, 2022
@ghost
Copy link

ghost commented Mar 20, 2022

Tagging subscribers to this area: @dotnet/area-extensions-logging
See info in area-owners.md if you want to be subscribed.

Issue Details

Updated LoggerExtensions.cs To Only Even Try To Log If The Log Level Is Enable To Improve Performance Of Logger By Saving Memory Allocation

Author: ProgrammingFire
Assignees: -
Labels:

area-Extensions-Logging

Milestone: -

@dnfadmin
Copy link

dnfadmin commented Mar 20, 2022

CLA assistant check
All CLA requirements met.

Added A Space After The `if` Keyword On-Line `28` Inside `LoggerExtensions.cs`
@@ -25,7 +25,10 @@ public static class LoggerExtensions
/// <example>logger.LogDebug(0, exception, "Error while processing request from {Address}", address)</example>
public static void LogDebug(this ILogger logger, EventId eventId, Exception? exception, string? message, params object?[] args)
{
logger.Log(LogLevel.Debug, eventId, exception, message, args);
if (logger.IsEnabled(LogLevel.Debug))
Copy link
Member

Choose a reason for hiding this comment

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

Which allocations should be saved by this pattern?

The logger.IsEnabled-check should be done before the call to LogDebug in user code.
Thus the allocation of args (the array) and potentially the creation of message can be saved. These objects are already existent when reaching the check you introduced here.

Or even better use LoggerMessage.Define where that behavior can be set / use the (new) logging source generator.

@tarekgh
Copy link
Member

tarekgh commented Mar 20, 2022

@ProgrammingFire please provide the benchmark numbers for the allocation and the code you are using to measure it.

@stephentoub stephentoub added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 20, 2022
Updated `LoggerExtensions.cs` To Only Even Try To Log If The Log Level Is Enabled And Make Sure The First Three Arguments For The Formatted Message Is Passed As Generics To Make Sure They Are Not Boxed And Unboxed To Improve Performance Of Logger By Saving Memory Allocation
@ProgrammingFire
Copy link
Author

ProgrammingFire commented Mar 21, 2022

Check this new commit which actually improves the memory allocation by not storing the first three arguments into objects so that they are not boxed and unboxed that actually saves memory allocation: cc87f19

@ProgrammingFire
Copy link
Author

I think the improvements in the logger depends on the user that's why I am closing this pull request.

@ghost ghost locked as resolved and limited conversation to collaborators May 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Logging community-contribution Indicates that the PR has been added by a community member NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants