-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Don't use IEnumerable and yield return for GetDefaultInterfaceImplementations #98183
Don't use IEnumerable and yield return for GetDefaultInterfaceImplementations #98183
Conversation
Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr, @marek-safar Issue Details
|
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.
Nice find, thanks!
Co-authored-by: Sven Boemer <sbomer@gmail.com>
@@ -313,8 +311,7 @@ IEnumerable<InterfaceImplementation> GetDefaultInterfaceImplementations (TypeDef | |||
// We haven't found a MethodImpl on the current interface, but one of the interfaces | |||
// this interface requires could still provide it. | |||
if (!foundImpl) { | |||
foreach (var impl in GetDefaultInterfaceImplementations (potentialImplInterface, interfaceMethod)) | |||
yield return impl; | |||
FindAndAddDefaultInterfaceImplementations (potentialImplInterface, interfaceMethod); |
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.
This is not producing equivalent results.
Previously, we always called AddDefaultInterfaceImplementation (resolvedInterfaceMethod, type, {ThisComesFromTheAlgorithm});
Now we are calling AddDefaultInterfaceImplementation (resolvedInterfaceMethod, {ThisComesFromTheAlgorithm}, {ThisComesFromTheAlgorithm});
because at this spot type
is swapped for potentialImplInterface
when this recursively calls itself at this spot.
Is that a problem? Or the second parameter doesn't matter for anything? If it doesn't matter, the perf improvement in #98436 can be even better - we don't have to look for default implementation per each class - we can only look for default implementations when MapType
is called for an interface and don't worry about default implementations when looking at classes at all.
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.
Oh, good catch. We do need the correct 'ImplementingType' for this, so we'll have to make sure the original type
is passed to AddDefaultInterfaceImplementation
.
…eImplementations (dotnet#98183)" This reverts commit 8af0b00.
GetDefaultInterfaceImplementations
takes the most time of anything in the linker in a hello world app, and allocates ~200mb total. If we refactor to not use yield return and directly add the DIMs to the cache in the method, we speed up by about 20% on a hello world app and use much less memory.