Skip to content

Conversation

ToddGrun
Copy link
Contributor

@ToddGrun ToddGrun commented Sep 11, 2025

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

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.
@ToddGrun ToddGrun requested a review from a team as a code owner September 11, 2025 20:32
Copy link
Member

@DustinCampbell DustinCampbell left a 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...

  1. Every caller of ToDisplayString(SymbolExtensions.FullNameTypeDisplayFormat) should be updated to call the GetFullName extension method. Calling into the cache should be encapsulated by GetFullName.
  2. A new GetGloballyQualifiedFullName extension method should be added and every caller of ToDisplayString(GloballyQualifedFullNameTypeDisplayFormat) should be updated to call GetGloballyQualifiedFullName.
  3. A new method to replace all calls to ToDisplayString() with no SymbolDisplayFormat. (GetSimpleDisplayString()? GetDefaultDisplayString()?
  4. 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.
Copy link
Member

@DustinCampbell DustinCampbell left a comment

Choose a reason for hiding this comment

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

🚀

@ToddGrun ToddGrun merged commit c3114ee into dotnet:main Sep 17, 2025
11 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Sep 17, 2025
@akhera99 akhera99 modified the milestones: Next, 18.0 P1, 18.0 P2 Sep 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants