Skip to content

Optimize IsRecord for non-record fast paths#48935

Merged
1 commit merged intodotnet:masterfrom
333fred:isrecord-optimize
Oct 27, 2020
Merged

Optimize IsRecord for non-record fast paths#48935
1 commit merged intodotnet:masterfrom
333fred:isrecord-optimize

Conversation

@333fred
Copy link
Member

@333fred 333fred commented Oct 27, 2020

We noticed some perf regressions when SymbolDisplayVisitor was calling IsRecord for a lot of types. This attempts to reduce the memory impact by having PEMethodSymbol.IsRecord first check to see if any member is named <Clone>$, and not loading the full signatures of all members.

We noticed some perf regressions when SymbolDisplayVisitor was calling IsRecord for a lot of types. This attempts to reduce the memory impact by having PEMethodSymbol.IsRecord first check to see if any member is named <Clone>$, and not loading the full signatures of all members.
@333fred 333fred changed the title Optimize FindValidCloneMethod for non-record fast paths Optimize IsRecord for non-record fast paths Oct 27, 2020
@333fred
Copy link
Member Author

333fred commented Oct 27, 2020

@sharwell
Copy link
Contributor

sharwell commented Oct 27, 2020

I have a fix for this in SymbolDisplayVisitor in progress, which will not hurt to have both. If this one works though, it's certainly cleaner than mine.

@333fred
Copy link
Member Author

333fred commented Oct 27, 2020

I have a fix for this in SymbolDisplayVisitor in progress, which will not hurt to have both. If this one works though, it's certainly cleaner than mine.

@sharwell I don't believe a fix for this in SymbolDisplayVisitor is the right solution. It's not the layer that needs to care about this.

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@RikkiGibson
Copy link
Member

It appears this fixes the BytesAllocated scenario that we were concerned about but I noticed a new RefSet_Image_Delta Regression instead in a different scenario. I don't see how it could be related to this change, but I'm not extremely experienced with the tools.

@333fred 333fred marked this pull request as ready for review October 27, 2020 16:41
@333fred 333fred requested a review from a team as a code owner October 27, 2020 16:41
@333fred
Copy link
Member Author

333fred commented Oct 27, 2020

@RikkiGibson the refset regression is likely due to the CLR methods jitting regressions.

@ghost
Copy link

ghost commented Oct 27, 2020

Hello @333fred!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-approval

@ghost ghost merged commit df72206 into dotnet:master Oct 27, 2020
@ghost ghost added this to the Next milestone Oct 27, 2020
333fred added a commit to 333fred/roslyn that referenced this pull request Oct 27, 2020
dotnet#48935 optimizes one path that hits FindValidCloneMethod, but there are others that could do with the same fast-path optimization. In order to accomplish that, I introduced a new HasPossibleCloneMethod on NamedTypeSymbol, that returns whether the type _could_ have a method on it that is a valid clone method. For source types, this is a quick syntax check to see if it's a record. For metadata types, it's a check on just the member names for something with the right name. This should prevent us from unnecessarily loading all method signatures from metadata unnecessarily whenever we check for a clone method.
@333fred 333fred deleted the isrecord-optimize branch October 27, 2020 19:20
@allisonchou allisonchou modified the milestones: Next, 16.9.P2 Nov 24, 2020
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants