Skip to content
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

Adopt WellKnownTypeProvider for type lookup by name #8310

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

sharwell
Copy link
Member

@sharwell sharwell commented Feb 22, 2023

  • Store WellKnownTypeProvider (from dotnet/roslyn-analyzers) in TagHelperDescriptorProviderContext
  • Match by type instead of name

@sharwell sharwell changed the title Store WellKnownTypeProvider (from dotnet/roslyn-analyzers) in TagHelperDescriptorProviderContext Adopt WellKnownTypeProvider for type lookup by name Feb 22, 2023
@sharwell sharwell marked this pull request as ready for review February 23, 2023 15:45
@sharwell sharwell requested review from a team as code owners February 23, 2023 15:45
@chsienki
Copy link
Contributor

We'll need to run the benchmarks on this to make sure it doesn't regress. Currently the path that we changed name lookup for isn't being hit. So we'll need to incorporate the perf/generator PR on top of this to ensure it doesn't regress in the new scenario.

@chsienki
Copy link
Contributor

@sharwell I merged everything together in order to run the benchmarks, and it looks good.

Old:

|           Method |     Mean |   Error |  StdDev |      Gen0 |      Gen1 | Allocated |
|----------------- |---------:|--------:|--------:|----------:|----------:|----------:|
| Cold_Compilation | 391.4 ms | 4.08 ms | 3.82 ms | 6000.0000 | 2000.0000 | 166.92 MB |

|                        Method |     Mean |     Error |    StdDev |    Gen0 |    Gen1 |  Allocated |
|------------------------------ |---------:|----------:|----------:|--------:|--------:|-----------:|
|         Razor_Add_Independent | 5.140 ms | 0.0516 ms | 0.0483 ms | 78.1250 | 31.2500 | 2142.84 KB |
|        Razor_Edit_Independent | 4.143 ms | 0.0417 ms | 0.0370 ms | 85.9375 | 31.2500 |  2186.2 KB |
|      Razor_Remove_Independent | 1.590 ms | 0.0095 ms | 0.0084 ms | 31.2500 | 11.7188 |  818.17 KB |
| Razor_Edit_DependentIgnorable | 2.934 ms | 0.0206 ms | 0.0193 ms | 54.6875 | 15.6250 | 1362.03 KB |
|          Razor_Edit_Dependent | 6.258 ms | 0.1012 ms | 0.0947 ms | 93.7500 | 31.2500 | 2358.26 KB |
|        Razor_Remove_Dependent | 1.542 ms | 0.0308 ms | 0.0329 ms | 31.2500 | 15.6250 |  818.18 KB |


New:

|           Method |     Mean |    Error |   StdDev |   Median |      Gen0 |      Gen1 | Allocated |
|----------------- |---------:|---------:|---------:|---------:|----------:|----------:|----------:|
| Cold_Compilation | 516.2 ms | 10.79 ms | 31.66 ms | 504.8 ms | 5000.0000 | 2000.0000 | 132.29 MB |


|                        Method |     Mean |     Error |    StdDev |   Median |    Gen0 |    Gen1 |  Allocated |
|------------------------------ |---------:|----------:|----------:|---------:|--------:|--------:|-----------:|
|         Razor_Add_Independent | 5.692 ms | 0.1097 ms | 0.0916 ms | 5.718 ms | 46.8750 | 15.6250 | 1405.31 KB |
|        Razor_Edit_Independent | 5.190 ms | 0.1032 ms | 0.3044 ms | 5.109 ms | 54.6875 | 23.4375 | 1434.03 KB |
|      Razor_Remove_Independent | 1.716 ms | 0.0325 ms | 0.0348 ms | 1.713 ms | 15.6250 |  7.8125 |  390.03 KB |
| Razor_Edit_DependentIgnorable | 3.105 ms | 0.0326 ms | 0.0289 ms | 3.096 ms | 39.0625 | 15.6250 | 1023.87 KB |
|          Razor_Edit_Dependent | 7.545 ms | 0.2226 ms | 0.6565 ms | 7.536 ms | 62.5000 | 31.2500 | 1604.64 KB |
|        Razor_Remove_Dependent | 1.724 ms | 0.0329 ms | 0.0308 ms | 1.729 ms | 15.6250 |  3.9063 |  390.04 KB |

Note that the 'old' doesn't include #8297 so the allocations are a little higher, but generally looks like the perf impact is minimal (it was like 10x slower with GetTypeByMetadataName)


namespace Microsoft.CodeAnalysis.Razor;

internal static class ImmutableArrayExtensions
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

@sharwell sharwell Feb 24, 2023

Choose a reason for hiding this comment

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

➡️ Moving to new location

@@ -1 +1,2 @@
#nullable enable
*REMOVED*~static Microsoft.CodeAnalysis.Razor.TagHelperDescriptorProviderContextExtensions.SetCompilation(this Microsoft.AspNetCore.Razor.Language.TagHelperDescriptorProviderContext context, Microsoft.CodeAnalysis.Compilation compilation) -> void
Copy link
Member

Choose a reason for hiding this comment

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

We can't break the public API yet.

Copy link
Member Author

@sharwell sharwell Feb 24, 2023

Choose a reason for hiding this comment

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

➡️ Restoring the method, marking obsolete, and implementing by creating a new WellKnownTypeProvider and forwarding the call to SetTypeProvider

var compilation = context.GetCompilation();
if (compilation == null)
var typeProvider = context.GetTypeProvider();
if (typeProvider == null || !ComponentSymbols.TryCreate(typeProvider, out var symbols))
Copy link
Member

Choose a reason for hiding this comment

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

Feels like it might be cleaner to move the typeProvider == null into TryCreate.

Copy link
Member Author

@sharwell sharwell Feb 24, 2023

Choose a reason for hiding this comment

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

➡️ Added the null check to TryCreate

switch (symbol.Kind)
{
case SymbolKind.Alias:
// Aliases are uber private. They're only visible in the same file that they
Copy link
Member

Choose a reason for hiding this comment

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

I just know this is a Cyrus comment.

Public = 0,
Internal = 1,
Private = 2,
Friend = Internal,
Copy link
Member

Choose a reason for hiding this comment

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

We don't deal with VB, this alias isn't required.

Copy link
Member Author

@sharwell sharwell Feb 24, 2023

Choose a reason for hiding this comment

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

➡️ Removing

/// <summary>
/// Provides and caches well known types in a compilation.
/// </summary>
public class WellKnownTypeProvider
Copy link
Member

Choose a reason for hiding this comment

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

Feels like maybe we should pull this from roslyn-analyzers into a shared package, like we do for pooled objects. I'm somewhat concerned about trying to keep this up to date: razor already has plenty of things copied over that have received significant changes from the original code.

Copy link
Member Author

@sharwell sharwell Feb 24, 2023

Choose a reason for hiding this comment

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

It's certainly possible. I'm not happy with the current API shape so I'd be hoping for a significant rewrite.

Ideally this would be moved to the compiler as part of a source generator helper API, and instead of operating on Compilation it would operate on IAssemblySymbol in a manner than allows symbols from metadata references to be carried forward in an incremental source generator without changing every time the compilation changes.

➡️ Not planning on making any changes here as part of this pull request.

@333fred
Copy link
Member

333fred commented Feb 24, 2023

I merged everything together in order to run the benchmarks, and it looks good.

@chsienki correct me if I'm reading this wrong, but those benchmarks don't look good? Cold compilation, in particular, looks bad. The minor memory savings don't seem to be paying off with wall-clock time.

@jaredpar jaredpar added this to the Backlog milestone Sep 25, 2023
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.

4 participants