-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Delete useless delegate virtual methods #97959
Conversation
The split between `Delegate` and `MulticastDelegate` is a wart that likely has some history behind it. Types that inherit from `Delegate` directly would not be considered delegates by the runtime. They need to inherit `MulticastDelegate. I can't find a reason why we'd need some useless base implementation of these methods that immediately gets overriden in `MulticastDelegate`. This deletes the useless base implementation and moves the useful implementation from `MulticastDelegate` to `Delegate`. This along with dotnet#97951 saves ~40 bytes per delegate `MethodTable` because the virtual methods can now be devirtualized or placed into the sealed vtable. We might be able to do even more since technically sealed virtuals could be reshuffled after the codegen phase and slots eliminated then.
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue DetailsThe split between This along with #97951 saves ~40 bytes per delegate Cc @dotnet/ilc-contrib
|
src/coreclr/nativeaot/System.Private.CoreLib/src/System/Delegate.cs
Outdated
Show resolved
Hide resolved
src/coreclr/nativeaot/System.Private.CoreLib/src/System/MulticastDelegate.cs
Outdated
Show resolved
Hide resolved
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.
LGTM otherwise
@@ -1,18 +1,13 @@ | |||
// Licensed to the .NET Foundation under one or more agreements. | |||
// The .NET Foundation licenses this file to you under the MIT license. |
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.
Can this file be moved under libraries and shared between runtimes now?
if (d2 is null) | ||
{ | ||
return d1 is null; | ||
} | ||
|
||
return ReferenceEquals(d2, d1) ? true : d2.Equals((object?)d1); |
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.
if (d2 is null) | |
{ | |
return d1 is null; | |
} | |
return ReferenceEquals(d2, d1) ? true : d2.Equals((object?)d1); | |
if (ReferenceEquals(d1, d2)) | |
{ | |
return true; | |
} | |
return d2 is not null && d2.Equals((object?)d1); |
Wouldn't this be better?
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.
"Test d2 first to allow branch elimination when inlined for null checks (== null) so it can become a simple test" comment above explains why the code is written the way it is written.
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.
"Test d2 first to allow branch elimination when inlined for null checks (== null) so it can become a simple test" comment above explains why the code is written the way it is written.
ReferenceEquals
lets it become a simple test too though with a constant null?
G_M4196_IG01: ;; offset=0x0000
;; size=0 bbWeight=1 PerfScore 0.00
G_M4196_IG02: ;; offset=0x0000
33C0 xor eax, eax
4885C9 test rcx, rcx
0F94C0 sete al
;; size=8 bbWeight=1 PerfScore 1.50
G_M4196_IG03: ;; offset=0x0008
C3 ret
;; size=1 bbWeight=1 PerfScore 1.00
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, this looks better. Would you like to submit a PR with this improvement?
This pattern came from dotnet/coreclr#21765 that applied this pattern in multiple places (the pattern was later modified in dotnet/coreclr#21829 to avoid a potential breaking change). All these places can use the same improvement.
The split between
Delegate
andMulticastDelegate
is a wart that likely has some history behind it. Types that inherit fromDelegate
directly would not be considered delegates by the runtime. They need to inheritMulticastDelegate
. I can't find a reason why we'd need some useless base implementation of these methods that immediately gets overriden inMulticastDelegate
. This deletes the useless base implementation and moves the useful implementation fromMulticastDelegate
toDelegate
.This along with #97951 saves ~40 bytes per delegate
MethodTable
because the virtual methods can now be devirtualized or placed into the sealed vtable. We might be able to do even more since technically sealed virtuals could be reshuffled after the codegen phase and slots eliminated then.Cc @dotnet/ilc-contrib