- Notifications
You must be signed in to change notification settings - Fork5.2k
Improve performance on virtual method processing#100897
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
- 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
sbomer left a comment
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.
It also exits early when possible in the IsInterfaceImplementationMarkedRecursively method.
Looks like you reverted that change, but there are still some left-over related diffs.
Uh oh!
There was an error while loading.Please reload this page.
sbomer commentedApr 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. |
sbomer left a comment
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.
Good find, thank you!
| { | ||
| _methods=newQueue<(MethodDefinition,DependencyInfo,MessageOrigin)>(); | ||
| _virtual_methods=newHashSet<(MethodDefinition,MarkScopeStack.Scope)>(); | ||
| _virtual_methods=newDictionary<MethodDefinition,MarkScopeStack.Scope>(); |
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.
Is the perf improvement due to the change of key? If so isMethodDefinition actually a good option for lookups? It doesn't implementIEquatable and doesn't have a customGetHashcode. Would a custom record using what makes aMethodDefinition identity better then?
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.
The trimmer only creates a singleMethodDefinition object for each method definition in metadata, soobject's implementation ofGetHashCode andEquals are fine here.
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.Fixesdotnet/sdk#40060.
sebastienros commentedJun 27, 2024
Quick update on this PR, this definitely fixed the regression
|
Uh oh!
There was an error while loading.Please reload this page.
Fixesdotnet/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.