-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Use Delegate.EnumerateInvocationList instead of Delegate.GetInvocationList #9246
base: main
Are you sure you want to change the base?
Use Delegate.EnumerateInvocationList instead of Delegate.GetInvocationList #9246
Conversation
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.
LGMT
@@ -170,12 +170,12 @@ public void RemoveThreadPreprocessMessageHandlerFirst(ThreadMessageEventHandler | |||
{ | |||
ThreadMessageEventHandler newHandler = null; | |||
|
|||
foreach (ThreadMessageEventHandler testHandler in _threadPreprocessMessage.GetInvocationList()) | |||
foreach (ThreadMessageEventHandler testHandler in Delegate.EnumerateInvocationList(_threadPreprocessMessage)) |
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.
I'd remove the outer null checks as EnumerateInvocationList
accepts null and returns an empty enumerator (false). Same in PropertyMetadata
. Otherwise LGTM.
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.
I'd rather keep it that way since it would make cases when _threadPreprocessMessage is null slower while maybe making a tiny perf gain by skipping a null check. Your suggestion would require knowing how often it is called with a null delegate vs a non null delegate, which is more complex.
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.
Yes, that is a fine argument. Maybe I'll feel bored someday but probably not.
Just out of curiosity, does subscribing/unsubscribing in the handler not make the enumerator unhappy? |
Delegates are immutable (subscribing/unsubscribing creates a new delegate instance) so I don't believe we can make the enumerator unhappy: https://learn.microsoft.com/en-us/dotnet/api/system.delegate?view=net-8.0#:~:text=Delegates%20are%20immutable%3B%20once%20created%2C%20the%20invocation%20list%20of%20a%20delegate%20does%20not%20change. |
Description
Use
Delegate.EnumerateInvocationList
instead ofDelegate.GetInvocationList
.Delegate.GetInvocationList
allocates an array on every call whileDelegate.EnumerateInvocationList
doesn't allocate which makes the code faster and use less memory.I also used
Delegate.HasSingleTarget
for places where we only wanted to check if there was only one entry inDelegate.GetInvocationList
, which makes it even faster.Delegate.EnumerateInvocationList
andDelegate.HasSingleTarget
were added in .Net 9.0 Preview 4 with dotnet/runtime#97683.Benchmark results for
Delegate.EnumerateInvocationList
:I used this benchmark:
Code
Benchmark results for
Delegate.HasSingleTarget
:I used this benchmark:
Code
Customer Impact
Improved performance and reduced allocations.
Regression
No.
Testing
Local testing
Risk
Low.
Microsoft Reviewers: Open in CodeFlow