Cache ISymbol.ToDisplayString results#12220
Merged
ToddGrun merged 5 commits intodotnet:mainfrom Sep 17, 2025
Merged
Conversation
Instead of adding another CWT, went ahead and merged the CWTs that we already had for IAssemblySymbol/INamedTypeSymbol into one that I needed for ISymbol. Will add more information once a test insertion comes through indicating whether this helps.
Member
DustinCampbell
left a comment
There was a problem hiding this comment.
This is really promising! I have some meta suggestions, since you said this is still a work-in-progress.
I'd rather see a more prescriptive approach rather than adding a ToCachedDisplayString(SymbolDisplayFormat?) method that calling code has to remember to call. I think it'd also be good to hide SymbolDisplayFormats altogether Instead, I'd rather see...
- Every caller of
ToDisplayString(SymbolExtensions.FullNameTypeDisplayFormat)should be updated to call theGetFullNameextension method. Calling into the cache should be encapsulated byGetFullName. - A new
GetGloballyQualifiedFullNameextension method should be added and every caller ofToDisplayString(GloballyQualifedFullNameTypeDisplayFormat)should be updated to callGetGloballyQualifiedFullName. - A new method to replace all calls to
ToDisplayString()with noSymbolDisplayFormat. (GetSimpleDisplayString()?GetDefaultDisplayString()? - An analyzer that flags calls to
ToDisplayString(...)with a message to use one of the caching APIs.
With all of that, it should be harder to introduce new code that doesn't cache.
Thoughts?
2) Fix issue where the CWT in TagHelperCollector.Cache wasn't being removed properly. Just abandoned getting rid of that CWT as it's outside the scope of what this PR is attempting to achieve.
ToddGrun
commented
Sep 12, 2025
DustinCampbell
approved these changes
Sep 15, 2025
davidwengier
approved these changes
Sep 17, 2025
chsienki
approved these changes
Sep 17, 2025
DustinCampbell
approved these changes
Sep 17, 2025
This was referenced Sep 25, 2025
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Instead of adding another CWT, went ahead and merged the CWT that we already had for INamedTypeSymbol into one that I needed for ISymbol/IAssemblySymbol.
Looked at one trace from speedometer runs with/without this change and filtered to callstacks containing ToDisplayString.
CPU went from 188 ms -> 54 ms. Allocations went from 9 MB -> 300 KB.
Commit 3 test insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/670197