-
Notifications
You must be signed in to change notification settings - Fork 215
Cache ISymbol.ToDisplayString results #12220
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
Cache ISymbol.ToDisplayString results #12220
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.
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 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 theGetFullName
extension method. Calling into the cache should be encapsulated byGetFullName
. - A new
GetGloballyQualifiedFullName
extension 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?
...ler/Microsoft.CodeAnalysis.Razor.Compiler/src/CSharp/ComponentTagHelperDescriptorProvider.cs
Outdated
Show resolved
Hide resolved
...oft.CodeAnalysis.Razor.Compiler/src/Language/SymbolCache.SymbolData.ToDisplayStringResult.cs
Outdated
Show resolved
Hide resolved
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.
src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/TagHelperCollector.cs
Show resolved
Hide resolved
src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/SymbolCache.cs
Outdated
Show resolved
Hide resolved
...Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/CSharp/BindTagHelperDescriptorProvider.cs
Outdated
Show resolved
Hide resolved
.../Microsoft.CodeAnalysis.Razor.Compiler/src/CSharp/EventHandlerTagHelperDescriptorProvider.cs
Outdated
Show resolved
Hide resolved
src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/CSharp/WellKnownSymbolDisplayFormats.cs
Show resolved
Hide resolved
.../Microsoft.CodeAnalysis.Razor.Compiler/src/Language/Extensions/INamedTypeSymbolExtensions.cs
Outdated
Show resolved
Hide resolved
...oft.CodeAnalysis.Razor.Compiler/src/Language/SymbolCache.SymbolData.ToDisplayStringResult.cs
Outdated
Show resolved
Hide resolved
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.
🚀
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