Avoid excessive WellKnownType cache misses in RDG #50669
Conversation
captainsafia
requested review from
BrennanConroy,
Tratcher and
halter73
as code owners
ghost
added
the
area-networking
|
with this change, do we meet VS/Roslyn perf requirements? (just asking as I know perf involved some iterations last cycle when we were learning to do source generators) |
To follow up on this, there's no concrete standards to meet when it comes to VS/Roslyn perf. The expectation is largely around making sure we do the due diligence to benchmark things, address gaps before shipping, and be flexible with issues that arise. We've got benchmarks for RDG in the repo that we can hook up to our automated infrastructure for monitoring. Locally profiling helps identify expensive codepaths, like the one resolved here, so we can nip things in the bud before shipping. Also, from benchmarking VS with the GeneratorTrace tool, we can observe that when there are no endpoints in an application, the generator's run time is inline with others (e.g. users not using it don't pay the cost of it running). 
|
|
Sounds great thanks for checking. |
captainsafia
added
feature-rdg
area-minimal
| private static bool ImplementsIEndpointMetadataProvider(ITypeSymbol? responseType, WellKnownTypes wellKnownTypes) | ||
| => responseType == null ? false : responseType.Implements(wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Http_Metadata_IEndpointMetadataProvider)); | ||
| private static bool ImplementsIEndpointMetadataProvider(ITypeSymbol? responseType) | ||
| => responseType == null ? false : responseType.Implements(["Microsoft", "AspNetCore", "Http", "Metadata", "IEndpointMetadataProvider"]); |
There was a problem hiding this comment.
I'm not familiar with the new initialization syntax. Does this array get cached?
| methodSymbol.Parameters.Length == 2 && | ||
| SymbolEqualityComparer.Default.Equals(methodSymbol.Parameters[0].Type, wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Http_HttpContext)) && | ||
| SymbolEqualityComparer.Default.Equals(methodSymbol.Parameters[1].Type, wellKnownTypes.Get(WellKnownType.System_Reflection_ParameterInfo)) && | ||
| methodSymbol.Parameters[0].Type.EqualsByName(["Microsoft", "AspNetCore", "Http", "HttpContext"]) && |
There was a problem hiding this comment.
What happens when there are duplicate arrays? For example, ["Microsoft", "AspNetCore", "Http", "HttpContext"] is used twice. Does it exist twice in memory?
Rather than passing arrays directly, what about having a static class with known values already defined?
There was a problem hiding this comment.
Does it exist twice in memory?
Looks like this is the case, although the implementation for collection expressions is changing.
I think having a static class with the known values is better for readability in either case.
|
Remove the unused values from KnownTypeData. Have we discussed this with Roslyn folks? I think it is worth asking them what they recommended. |
|
Should similar changes be applied to route tooling? |
| break; | ||
| } | ||
| if (IsBindAsync(methodSymbol, typeSymbol, wellKnownTypes)) | ||
| if (IsBindAsync(methodSymbol, typeSymbol)) |
There was a problem hiding this comment.
probably not worth the bother, but IsBindAsync and IsBindAsyncWithParameter are similar and could be one method that has an out bool returning whether there's a parameter.
| return false; | ||
| } | ||
|
|
||
| public static bool Implements(this ITypeSymbol type, params string[] interfaceName) |
There was a problem hiding this comment.
do you need the params on this and the method below since you seem to only pass in single arrays? not sure whether it affects generated code.
(I'm assuming you copied this from someplace where they need a more general version)
There was a problem hiding this comment.
by the way, it seems surprising that these useful methods aren't exposed in code analysis itself?
There was a problem hiding this comment.
Roslyn usually uses pattern matching when testing for types instead of arrays. For example, https://github.com/dotnet/roslyn/blob/dd7c29de3e172a3abf8768a493d925d54f9a15a9/src/Features/Core/Portable/EmbeddedLanguages/EmbeddedLanguageDetector.cs#L338-L357
I imagine it is faster, but it would be a method per type which would be very verbose.
Yes, this change came out of a discussion around some of the results I was seeing when measuring the run time of RDG in VS using the generator trace tool.
I thought about this but didn't want to jump the gun on making changes without having some numbers/profiles for route tooling itself. From my understanding, we should be able to measure perf for those analyzers by enabling |
I haven't done any profiling. Some Roslyn folks reviewed the route tooling code, which would include finding performance traps, but otherwise, I've been waiting to see if any users report perf issues. Apart from one stack overflow, no one has reported problems or perf issues with it. |
I think this is a totally sensible strategy given what I outlined in this comment.
It might be helpful to figure out how to setup microbenchmarks for analyzers though just so we have baselines in case extreme regressions do occur. |
ghost
added
the
pending-ci-rerun
|
Holding this to do more benchmarking and assess impact across different app sizes (e.g. various amounts of endpoints). |
|
@captainsafia I see your comment on benchmarking... just making sure, is this still something we want for RC2? |
|
@adityamandaleeka No, we can pause on this. |
wtgodbe
removed
the
pending-ci-rerun
dotnet-policy-service
bot
added
the
area-infrastructure
wtgodbe
removed
the
area-infrastructure
RDG currently relies on a WellKnownType cache to resolve type symbols for framework types that we want to compare against. The WellKnownType cache is keyed by compilation. VS invokes generators on each key press with a new compilation, which means that the cache is invalidated and we end up having to do expensive metadata look ups on the type with each keypress. To avoid this, we replace the type symbol comparisons with fuzzier checks that compare the fully-qualified names of the target types.