Improve performance on virtual method processing #100897
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Make _virtual_methods a dictionary to prevent duplicates due to the Scope object in the tuple - Exit early when possible in IsInterfaceImplementationMarkedRecursively - Return a list from TypeMapInfo.GetRecursiveInterfaces and iterate over with a regular for loop - Iterate over _typesWithInterfaces with a for loop instead of copying to an array
dotnet-issue-labeler
bot
added
the
area-Tools-ILLink
dotnet-policy-service
bot
added
the
linkable-framework
jtschuster
removed
the
linkable-framework
dotnet-policy-service
bot
added
the
linkable-framework
sbomer
reviewed
Apr 11, 2024
|
I'm good with the changes to how we iterate over overrides. I haven't thought of a case where we would modify the set while iterating over it, but the for loop doesn't hurt. |
| @@ -220,7 +220,7 @@ public partial class MarkStep : IStep | |||
| public MarkStep () | |||
| { | |||
| _methods = new Queue<(MethodDefinition, DependencyInfo, MessageOrigin)> (); | |||
| _virtual_methods = new HashSet<(MethodDefinition, MarkScopeStack.Scope)> (); | |||
| _virtual_methods = new Dictionary<MethodDefinition, MarkScopeStack.Scope> (); | |||
There was a problem hiding this comment.
Is the perf improvement due to the change of key? If so is MethodDefinition actually a good option for lookups? It doesn't implement IEquatable and doesn't have a custom GetHashcode. Would a custom record using what makes a MethodDefinition identity better then?
There was a problem hiding this comment.
The trimmer only creates a single MethodDefinition object for each method definition in metadata, so object's implementation of GetHashCode and Equals are fine here.
matouskozak
pushed a commit
to matouskozak/runtime
that referenced
this pull request
Apr 30, 2024
The _virtual_methods cache had duplicates due to the Scope item in the tuple, which made ProcessVirtualMethods do significantly more work. This PR makes the cache a dictionary, with TryAdd's instead of Add to only add each method once. These changes made caused the overrides cache in TypeMapInfo to be modified during iteration, so a for loop is used instead of a foreach loop. This should be find since the list will always be append-only. I noticed we copy _typesWithInterfaces to an array for the same reason, though since that's also append-only we can iterate with a for loop there, too. Fixes dotnet/sdk#40060.
|
Quick update on this PR, this definitely fixed the regression
|
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes dotnet/sdk#40060.
The _virtual_methods cache had duplicates due to the
Scopeitem in the tuple, which made ProcessVirtualMethods do significantly more work. This PR makes the cache a dictionary, with TryAdd's instead of Add to only add each method once.It also exits early when possible in the IsInterfaceImplementationMarkedRecursively method.These changes made caused theoverridescache in TypeMapInfo to be modified during iteration, so aforloop is used instead of a foreach loop. This should be find since the list will always be append-only. I noticed we copy _typesWithInterfaces to an array for the same reason, though since that's also append-only we can iterate with aforloop there, too.On my machine, this reduces the trim time of the aspnetcore benchmarks from ~80 seconds to ~35 seconds.