-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 pipeline inputs when suppressed: #24928
Cache pipeline inputs when suppressed: #24928
Conversation
chsienki
commented
Apr 19, 2022
- When razor suppression is true, treat all pipeline inputs as cached
- Treat razor options as cached if only the supression flag has changed
- Update tests to include output code for regression testing
- When razor suppression is true, treat all pipeline inputs as cached - Treat razor options as cached if only the supression flag has changed - Update tests to include output code for regression testing
@RikkiGibson @cston for review please |
if (y.Right.SuppressRazorSourceGenerator) | ||
{ | ||
// If source generation is suppressed, we can always use previously cached results. | ||
return true; |
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.
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.
Yes, its actually designed for that to specifically be the case. Any time the options flip from unsuppressed -> suppressed we want to override and pretend everything is cached and do no work. When we switch back to unsuppressed we want to do the actual check. It's taking advantage of knowing how the generator internals work (https://sourceroslyn.io/#Microsoft.CodeAnalysis/SourceGeneration/Nodes/NodeStateTable.cs,386), so a a horrible hack frankly but necessary to support razors odd behavior caused by the design time / runtime split (something we're working on removing which would remove all these required hacks).
And does Equals(x, y) guarantee GetHashCode(x) == GetHashCode(y)
It does not, but again we know this will never actually be used in a hash table, so it's never called. I think I'll add some comments on this to make it clear its a very specialized piece of code that shouldn't be re-used generally.
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.
we know this will never actually be used in a hash table
In that case, let's change GetHashCode()
to throw ExceptionUtilities.Unreachable
, to catch cases early if the comparer is used for a hash table in the future.
} | ||
|
||
internal static IncrementalValuesProvider<T> AsCachedIfSuppressed<T>(this IncrementalValuesProvider<T> provider, IncrementalValueProvider<RazorSourceGenerationOptions> options) | ||
where T : notnull |
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.
@@ -49,11 +49,14 @@ internal class RazorSourceGenerationOptions : IEquatable<RazorSourceGenerationOp | |||
public bool SupportLocalizedComponentNames { get; set; } = false; | |||
|
|||
public bool Equals(RazorSourceGenerationOptions other) | |||
=> SuppressRazorSourceGenerator == other.SuppressRazorSourceGenerator && EqualsIgnoringSupression(other); | |||
|
|||
public bool EqualsIgnoringSupression(RazorSourceGenerationOptions other) |
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.
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.
It was no longer used, so have removed.
Please consider marking In reply to: 1115154902 Refers to: src/RazorSdk/SourceGenerators/RazorSourceGenerationOptions.cs:11 in 2d6359e. [](commit_id = 2d6359e, deletion_comment = False) |
var additionalTexts = context.AdditionalTextsProvider.AsCachedIfSuppressed(liveRazorSourceGeneratorOptions); | ||
var parseOptions = context.ParseOptionsProvider.AsCachedIfSuppressed(liveRazorSourceGeneratorOptions); | ||
var compilation = context.CompilationProvider.AsCachedIfSuppressed(liveRazorSourceGeneratorOptions); | ||
var razorSourceGeneratorOptions = liveRazorSourceGeneratorOptions.WithLambdaComparer((l, r) => l.EqualsIgnoringSupression(r), l => l.GetHashCode()); |
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.
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.
When we go from suppressed -> unsuppressed, we don't want to note the options as having been changed unless any of the other options actually have changed.
Thinking about it, I wonder if it might be more obvious if we break out the suppression from the rest of the options. I'll take a quick look what that might look like
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.
Ok, have broken the suppression out into its own input which simplifies the options significantly
In reply to: 1115160981 Refers to: src/RazorSdk/SourceGenerators/IncrementalValueProviderExtensions.cs:89 in 2d6359e. [](commit_id = 2d6359e, deletion_comment = False) |
var result = RunGenerator(compilation!, ref driver); | ||
var result = RunGenerator(compilation!, ref driver) | ||
.VerifyPageOutput( | ||
@"#pragma checksum ""Pages/Index.razor"" ""{ff1816ec-aa5e-4d10-87f7-6f4963833460}"" ""6b5db227a6aa2228c777b0771108b184b1fc5df3"" |
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.
FYI - Historically this was bothersome because the codegen would change for existing code and it would block insertion PRs and it was non trivial to get this string. The razor-compiler repo had tooling to regen it but it’s a bit of a pain to use and makes diffs harder to work with.
.Where(static (file) => file.Path.EndsWith(".razor", StringComparison.OrdinalIgnoreCase) || file.Path.EndsWith(".cshtml", StringComparison.OrdinalIgnoreCase)) | ||
.Combine(context.AnalyzerConfigOptionsProvider) | ||
.Select(ComputeProjectItems); | ||
var analyzerConfigOptions = context.AnalyzerConfigOptionsProvider.AsCachedIfSuppressed(isGeneratorSuppressed); |
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.
Very nicely done! 👍🏽
/// A highly specialized comparer that allows for treating an event source as cached if the razor options are set to suppress generation. | ||
/// </summary> | ||
/// <remarks> | ||
/// This should not be used outside of <see cref="IncrementalValuesProviderExtensions.AsCachedIfSuppressed{T}(IncrementalValueProvider{T}, IncrementalValueProvider{RazorSourceGenerationOptions})"/> |
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.
internal static class IncrementalValuesProviderExtensions | ||
{ | ||
/// <summary> | ||
/// Adds a comparer that will force the provider to be considered as cached if the razor options call for supression |
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.
/// The property is set by the SDK via an editor config. | ||
/// </para> | ||
/// </summary> | ||
private static bool GetSupressionStatus(AnalyzerConfigOptionsProvider optionsProvider, CancellationToken _) |
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.
/// to run without invoking the source generator to avoid duplicate types being produced. | ||
/// The property is set by the SDK via an editor config. | ||
/// </para> | ||
/// </summary> | ||
public bool SuppressRazorSourceGenerator { get; set; } = false; |
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.
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.
Removed. just an oversight.
In reply to: 1119997110 Refers to: src/RazorSdk/SourceGenerators/RazorSourceGenerator.cs:91 in 28b694b. [](commit_id = 28b694b, deletion_comment = False) |
@@ -223,19 +211,12 @@ public void Initialize(IncrementalGeneratorInitializationContext context) | |||
.Combine(importFiles.Collect()) | |||
.Combine(allTagHelpers) | |||
.Combine(razorSourceGeneratorOptions) | |||
.Combine(context.ParseOptionsProvider) |
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.
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.
Yes, it wasn't actually being used.
updatedOptionsProvider.TestGlobalOptions[option.Key] = option.Value; | ||
} | ||
// now run the generator with suppression | ||
var supressedOptions = optionsProvider.Clone(); |
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.
|
||
driver = driver.WithUpdatedAnalyzerConfigOptions(updatedOptionsProvider); | ||
result = RunGenerator(compilation!, ref driver); | ||
// now unsupress and re-run |
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.
catch (EqualException e) | ||
{ | ||
Assert.False(true, $"No diff supplied. But index {i} was:\r\n\r\n{e.Actual.Replace("\"", "\"\"")}"); | ||
} |
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.
What is the try/catch
for?
Could we use Assert.True(expectedText == actualText, $"... index {i} was ... {actualText}");
instead?
Or if (expectedText != actualText) Assert.False(true, $"... index {i} was ... {actualText}");
? #Closed
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.
Ah yeah, that was just from where I was iterating to get a decent mechanism for comparison. That path is only used if you don't supply a diff at all, so we can just assert if they don't match.
48246cc
to
8925dd3
Compare
/// For instance <c>dotnet msbuild /p:_RazorSourceGeneratorDebug=true</c> | ||
/// </para> | ||
/// </summary> | ||
public bool WaitForDebugger { get; set; } = false; |
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.
Is this removal related to the overall change? Is this just no longer needed, or perhaps there's a better way of debugging the generator that should be used instead?
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.
It was no longer used. You can't really do a 'wait' in the incremental world, and the flag check was removed in the transition, just not removed here.
if (y.IsSuppressed) | ||
{ | ||
// If source generation is suppressed, we can always use previously cached results. | ||
return true; |
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.
What's the meaning of x.IsSuppressed
in this code path? Will it always also be true
here?
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.
It can be both true
and false
, but for the purposes here, we don't care. We only want to suppress when going from unsuppressed -> suppressed, so X
has no effect.
.Select(static (pair, _) => | ||
{ | ||
var ((((sourceItem, imports), allTagHelpers), razorSourceGeneratorOptions), parserOptions) = pair; | ||
var (((sourceItem, imports), allTagHelpers), razorSourceGeneratorOptions) = pair; |
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.
It would be nice if Combine could build up a tuple more "gracefully" if the receiver of the Combine call is already a tuple. e.g. it would be ideal if we had var (sourceItem, imports, allTagHelpers, razorSourceGeneratorOptions) = pair;
here. I wonder if that ship has already sailed on the Roslyn side?
Alternatively, it seems like overloading .Combine()
so that it produces tuples of various sizes might be handy. But I imagined there might be combinatorial issues with use of IncrementalValueProvider
versus IncrementalValuesProvider
.
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.
Yeah, its something we should think about.
One API we considered was TOut Combine<TOut, TLeft, TRight>(IncrementalValueProvider<TLeft>, IncrementalValuesProvider<TRight>, Func<TOut, TLeft, TRight>);
Essentially allow you to select at the same time as combine, so you can build up a datamodel as you go. You can do it today via chaining, but it would be nice to make it easier.
@@ -2723,7 +2723,7 @@ public static GeneratorRunResult VerifyOutputsMatch(this GeneratorRunResult actu | |||
|
|||
private static string TrimChecksum(string text) | |||
{ | |||
var trimmed = text.Trim('\r', '\n').Replace("\r\n", "\n").Replace('\n', '\r'); | |||
var trimmed = text.Trim('\r', '\n').Replace("\r\n", "\r").Replace('\r', '\n'); |
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.
don't we have a .NormalizeWhitespace() or similar helper that can remove the guesswork here?
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.
We're in the SDK repo, so no, this is all new custom test stuff. We're going to move this over to the razor compiler repo, and I imagine i'll pull in some of the roslyn testing bits when we do.
The Razor generator is controlled by a flag that lives in an .editorconfig file; in the IDE we generally don't run the generator and instead use the design-time files added through the legacy IDynamicFileInfo API. When we're doing Hot Reload we then remove those legacy files and remove the .editorconfig file that is supposed to disable the generator; for the Hot Reload pass we then are running the generator. This is done in the CompileTimeSolutionProvider. dotnet/sdk#24928 introduced an issue where even though the Razor generator is being told to not run, it still runs anyways. As a tactical fix rather than reverting that PR, for Visual Studio 17.3 Preview 2 we are going to do a hack here which is to rip out generated files. In discussion, simply reverting the SDK change was problematic because it would reintroduce some notable performance issues that it was intended to fix in the first place. Fixing the generator directly is also possible (and we'll be doing a follow-up PR to try doing that), but is also a fairly risky change given the amount of testing that was required to test the PR in the first place. This we think is the least-risky option we have, even though it itself is touching a fairly sensitive area of our codebase. The intent is to remove this hack by Preview 3 with a properly tested fix in the generator.
The Razor generator is controlled by a flag that lives in an .editorconfig file; in the IDE we generally don't run the generator and instead use the design-time files added through the legacy IDynamicFileInfo API. When we're doing Hot Reload we then remove those legacy files and remove the .editorconfig file that is supposed to disable the generator; for the Hot Reload pass we then are running the generator. This is done in the CompileTimeSolutionProvider. dotnet/sdk#24928 introduced an issue where even though the Razor generator is being told to not run, it still runs anyways. As a tactical fix rather than reverting that PR, for Visual Studio 17.3 Preview 2 we are going to do a hack here which is to rip out generated files. In discussion, simply reverting the SDK change was problematic because it would reintroduce some notable performance issues that it was intended to fix in the first place. Fixing the generator directly is also possible (and we'll be doing a follow-up PR to try doing that), but is also a fairly risky change given the amount of testing that was required to test the PR in the first place. This we think is the least-risky option we have, even though it itself is touching a fairly sensitive area of our codebase. The intent is to remove this hack by Preview 3 with a properly tested fix in the generator.
The Razor generator is controlled by a flag that lives in an .editorconfig file; in the IDE we generally don't run the generator and instead use the design-time files added through the legacy IDynamicFileInfo API. When we're doing Hot Reload we then remove those legacy files and remove the .editorconfig file that is supposed to disable the generator; for the Hot Reload pass we then are running the generator. This is done in the CompileTimeSolutionProvider. dotnet/sdk#24928 introduced an issue where even though the Razor generator is being told to not run, it still runs anyways. As a tactical fix rather than reverting that PR, for Visual Studio 17.3 Preview 2 we are going to do a hack here which is to rip out generated files. In discussion, simply reverting the SDK change was problematic because it would reintroduce some notable performance issues that it was intended to fix in the first place. Fixing the generator directly is also possible (and we'll be doing a follow-up PR to try doing that), but is also a fairly risky change given the amount of testing that was required to test the PR in the first place. This we think is the least-risky option we have, even though it itself is touching a fairly sensitive area of our codebase. The intent is to remove this hack by Preview 3 with a properly tested fix in the generator.
dotnet#24928 made a change to the Razor generator to ensure that even if the generator is suppressed, it's still going to run and cache it's outputs so later runs aren't a from-scratch run on a performance-critical path. However, a bug meant that if the generator was suppressed, it'd still run the first time it's invoked, and will still output files even though it was supposed to be suppressed. The approach taken here is to suppress the addition of the files, but at the very end of the chain only -- the generation and walking in the middle of the incremental chain is left untouched, so that still has the existing caching behavior. This fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1545938 but we're still considering dotnet/roslyn#61675 as a tactical fix instead.
The Razor generator is controlled by a flag that lives in an .editorconfig file; in the IDE we generally don't run the generator and instead use the design-time files added through the legacy IDynamicFileInfo API. When we're doing Hot Reload we then remove those legacy files and remove the .editorconfig file that is supposed to disable the generator; for the Hot Reload pass we then are running the generator. This is done in the CompileTimeSolutionProvider. dotnet/sdk#24928 introduced an issue where even though the Razor generator is being told to not run, it still runs anyways. As a tactical fix rather than reverting that PR, for Visual Studio 17.3 Preview 2 we are going to do a hack here which is to rip out generated files. In discussion, simply reverting the SDK change was problematic because it would reintroduce some notable performance issues that it was intended to fix in the first place. Fixing the generator directly is also possible (and we'll be doing a follow-up PR to try doing that), but is also a fairly risky change given the amount of testing that was required to test the PR in the first place. This we think is the least-risky option we have, even though it itself is touching a fairly sensitive area of our codebase. The intent is to remove this hack by Preview 3 with a properly tested fix in the generator.
The Razor generator is controlled by a flag that lives in an .editorconfig file; in the IDE we generally don't run the generator and instead use the design-time files added through the legacy IDynamicFileInfo API. When we're doing Hot Reload we then remove those legacy files and remove the .editorconfig file that is supposed to disable the generator; for the Hot Reload pass we then are running the generator. This is done in the CompileTimeSolutionProvider. dotnet/sdk#24928 introduced an issue where even though the Razor generator is being told to not run, it still runs anyways. As a tactical fix rather than reverting that PR, for Visual Studio 17.3 Preview 2 we are going to do a hack here which is to rip out generated files. In discussion, simply reverting the SDK change was problematic because it would reintroduce some notable performance issues that it was intended to fix in the first place. Fixing the generator directly is also possible (and we'll be doing a follow-up PR to try doing that), but is also a fairly risky change given the amount of testing that was required to test the PR in the first place. This we think is the least-risky option we have, even though it itself is touching a fairly sensitive area of our codebase. The intent is to remove this hack by Preview 3 with a properly tested fix in the generator.
dotnet#24928 made a change to the Razor generator to ensure that even if the generator is suppressed, it's still going to run and cache it's outputs so later runs aren't a from-scratch run on a performance-critical path. However, a bug meant that if the generator was suppressed, it'd still run the first time it's invoked, and will still output files even though it was supposed to be suppressed. The approach taken here is to suppress the addition of the files, but at the very end of the chain only -- the generation and walking in the middle of the incremental chain is left untouched, so that still has the existing caching behavior. This fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1545938 but we're still considering dotnet/roslyn#61675 as a tactical fix instead.
dotnet/sdk#24928 made a change to the Razor generator to ensure that even if the generator is suppressed, it's still going to run and cache it's outputs so later runs aren't a from-scratch run on a performance-critical path. However, a bug meant that if the generator was suppressed, it'd still run the first time it's invoked, and will still output files even though it was supposed to be suppressed. The approach taken here is to suppress the addition of the files, but at the very end of the chain only -- the generation and walking in the middle of the incremental chain is left untouched, so that still has the existing caching behavior. This fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1545938 but we're still considering dotnet/roslyn#61675 as a tactical fix instead.
dotnet/sdk#24928 made a change to the Razor generator to ensure that even if the generator is suppressed, it's still going to run and cache it's outputs so later runs aren't a from-scratch run on a performance-critical path. However, a bug meant that if the generator was suppressed, it'd still run the first time it's invoked, and will still output files even though it was supposed to be suppressed. The approach taken here is to suppress the addition of the files, but at the very end of the chain only -- the generation and walking in the middle of the incremental chain is left untouched, so that still has the existing caching behavior. This fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1545938 properly; dotnet/roslyn#61675 was put in as a tactical fix first.
This reverts commit 033522e.
This reverts commit 033522e.