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.

Sign up for GitHub

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

Closed
wants to merge 11 commits into from
+2 −3
Closed

perf in LogMessage #4453

Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4351b0a
perf in LogMessage
SimonCropp Dec 26, 2024
173b29c
Merge branch 'main' into perf-in-LogMessage
SimonCropp Dec 27, 2024
006b903
Update Logger.cs
SimonCropp Dec 27, 2024
5ef3b84
.
SimonCropp Dec 27, 2024
72c59c4
Merge branch 'main' into perf-in-LogMessage
SimonCropp Dec 28, 2024
aeaa33e
Update Directory.Packages.props
SimonCropp Dec 28, 2024
d74b740
Merge branch 'main' into perf-in-LogMessage
Evangelink Dec 28, 2024
e135fb6
Merge branch 'main' into perf-in-LogMessage
Evangelink Dec 28, 2024
3de4e5a
Merge branch 'main' into perf-in-LogMessage
SimonCropp Dec 29, 2024
ac28c8f
Merge branch 'main' into perf-in-LogMessage
Youssef1313 Dec 31, 2024
169a281
Merge branch 'main' into perf-in-LogMessage
SimonCropp Jan 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/TestFramework/TestFramework/Logger.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,12 @@ public static void LogMessage(string format, params object?[] args)
? format
: string.Format(CultureInfo.InvariantCulture, format, args);

object?[] parameters = [message];
// 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))
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Youssef1313 want me to burn this PR, and submit another that marks it as obsolete?

Copy link
Member

@Youssef1313 Youssef1313 Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'll let @Evangelink answer that on whether there are valid use cases for Logger.LogMessage or if people are using that API

{
try
{
invoker.GetMethodInfo()!.Invoke(invoker.Target, parameters);
invoker(message);
}
catch (Exception)
{
Expand Down
Loading