-
Notifications
You must be signed in to change notification settings - Fork 781
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
Wrap EventSource calls with IsEnabled #2052
Conversation
GrpcEventSource.Log.CallFailed(_status.StatusCode); | ||
} | ||
} | ||
if (GrpcEventSource.Log.IsEnabled()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since you're always going to test enabled here, is it worth pulling this into a local around L297, to avoid multiple checks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IsEnabled implementation directly returns a field: https://github.com/dotnet/runtime/blob/463954dbc728470fb21a097188fc3dae91f6cbe4/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs#L256-L263
IsEnabled is so cheap (probably inlined) that I don't think it's worth it.
_deadline = DateTime.MaxValue; | ||
} | ||
} | ||
} | ||
|
||
GrpcCallLog.GrpcStatusError(Logger, status.StatusCode, status.Detail); | ||
GrpcEventSource.Log.CallFailed(status.StatusCode); | ||
if (GrpcEventSource.Log.IsEnabled()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto hoist covering 3 in a row here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nits re avoidable repeated call; 👍 in principle, and I'll let you decide whether that change is needed
@@ -23,7 +23,7 @@ | |||
|
|||
namespace Grpc.AspNetCore.Server.Internal; | |||
|
|||
internal class GrpcEventSource : EventSource | |||
internal sealed class GrpcEventSource : EventSource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 , although with the static readonly Log
I would hope that the JIT devirtualizes anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 , although with the
static readonly Log
I would hope that the JIT devirtualizes anyway
Yes, jit does that for static readonly
Wrap event source calls with IsEnabled to avoid
Interlocked.Increment
calls inside the methods.Small perf improvement:
The perf improvement will be much bigger with AOT + ARM: dotnet/runtime#73246