-
Notifications
You must be signed in to change notification settings - Fork 262
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
perf in LogMessage #4453
base: main
Are you sure you want to change the base?
perf in LogMessage #4453
Conversation
I have synced the Polyfill package. |
@Evangelink We need to add the updated version to https://github.com/dotnet/source-build-reference-packages/tree/main/src/textOnlyPackages/src/polyfill/ I'll take care of that. |
I opened dotnet/source-build-reference-packages#1105 |
// Making sure all event handlers are called in sync on same thread. | ||
foreach (Delegate invoker in OnLogMessage.GetInvocationList()) | ||
foreach (LogMessageHandler invoker in DelegatePolyfill.EnumerateInvocationList(OnLogMessage)) |
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.
Note: we don't call Logger.LogMessage at all. I don't know how much consumers will be calling this, but it doesn't seem to be a hot path.
@Evangelink I'm not sure if there are "valid" use cases for this Logger class or not. If not, we should consider breaking in v4.
I assume logging is in the hot path. so a few more lines for better oerf is justified
move over to using EnumerateInvocationList