Cache pipeline inputs when suppressed: by chsienki · Pull Request #24928 · dotnet/sdk

chsienki · 2022-04-19T21:07:24Z

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

chsienki · 2022-04-27T20:57:06Z

@RikkiGibson @cston for review please

cston · 2022-04-29T17:23:11Z

src/RazorSdk/SourceGenerators/IncrementalValueProviderExtensions.cs

+            if (y.Right.SuppressRazorSourceGenerator)
+            {
+                // If source generation is suppressed, we can always use previously cached results.
+                return true;


return true;

Does this mean RazorSourceGeneratorComparer<T> is not symmetric? That is Equals(x, y) does not necessarily imply Equals(y, x)?

And does Equals(x, y) guarantee GetHashCode(x) == GetHashCode(y)? #Closed

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.

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.

cston · 2022-05-02T16:59:57Z

src/RazorSdk/SourceGenerators/IncrementalValueProviderExtensions.cs

+        }
+
+        internal static IncrementalValuesProvider<T> AsCachedIfSuppressed<T>(this IncrementalValuesProvider<T> provider, IncrementalValueProvider<RazorSourceGenerationOptions> options)
+    where T : notnull


where

Indent? #Closed

cston · 2022-05-02T17:30:47Z

src/RazorSdk/SourceGenerators/RazorSourceGenerationOptions.cs

        public bool Equals(RazorSourceGenerationOptions other)
+                   => SuppressRazorSourceGenerator == other.SuppressRazorSourceGenerator && EqualsIgnoringSupression(other);
+
+        public bool EqualsIgnoringSupression(RazorSourceGenerationOptions other)


EqualsIgnoringSupression

Should this consider WaitForDebugger as well? #Closed

It was no longer used, so have removed.

cston · 2022-05-02T17:31:23Z

internal class RazorSourceGenerationOptions : IEquatable<RazorSourceGenerationOptions>

Please consider marking sealed to simplify reasoning about Equals().

In reply to: 1115154902

Refers to: src/RazorSdk/SourceGenerators/RazorSourceGenerationOptions.cs:11 in 2d6359e. [](commit_id = 2d6359e, deletion_comment = False)

cston · 2022-05-02T17:36:07Z

src/RazorSdk/SourceGenerators/RazorSourceGenerator.cs

+            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());


l.EqualsIgnoringSupression(r)

Why are we ignoring suppression here rather than changing the RazorSourceGeneratorOptions? When do the options change?

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

Ok, have broken the suppression out into its own input which simplifies the options significantly

cston · 2022-05-02T17:37:39Z

internal class LambdaComparer<T> : IEqualityComparer<T>

sealed

In reply to: 1115160981

Refers to: src/RazorSdk/SourceGenerators/IncrementalValueProviderExtensions.cs:89 in 2d6359e. [](commit_id = 2d6359e, deletion_comment = False)

pranavkm · 2022-05-06T15:05:45Z

src/Tests/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/RazorSourceGeneratorTests.cs

-            var result = RunGenerator(compilation!, ref driver);
+            var result = RunGenerator(compilation!, ref driver)
+                            .VerifyPageOutput(
+@"#pragma checksum ""Pages/Index.razor"" ""{ff1816ec-aa5e-4d10-87f7-6f4963833460}"" ""6b5db227a6aa2228c777b0771108b184b1fc5df3""


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.

pranavkm · 2022-05-06T15:05:53Z

src/RazorSdk/SourceGenerators/RazorSourceGenerator.cs

-                .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);


Very nicely done! 👍🏽

cston · 2022-05-06T20:28:19Z

src/RazorSdk/SourceGenerators/IncrementalValueProviderExtensions.cs

+    /// 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})"/>


IncrementalValuesProviderExtensions

Consider making RazorSourceGeneratorComparer<T> a private nested class in IncrementalValuesProviderExtensions, to reduce the chance this will be used elsewhere. #Closed

cston · 2022-05-06T20:29:57Z

src/RazorSdk/SourceGenerators/IncrementalValueProviderExtensions.cs

    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


supression

Typo, here and below. #Closed

cston · 2022-05-06T20:31:35Z

src/RazorSdk/SourceGenerators/RazorSourceGenerator.RazorProviders.cs

+        /// The property is set by the SDK via an editor config.
+        /// </para>
+        /// </summary>
+        private static bool GetSupressionStatus(AnalyzerConfigOptionsProvider optionsProvider, CancellationToken _)


Supression

Typo. #Closed

cston · 2022-05-06T20:39:16Z

src/RazorSdk/SourceGenerators/RazorSourceGenerationOptions.cs

-        /// The property is set by the SDK via an editor config.
-        /// </para>
-        /// </summary>
        public bool SuppressRazorSourceGenerator { get; set; } = false;


SuppressRazorSourceGenerator

Is this SuppressRazorSourceGenerator field still used? If so, why are we able to ignore the value in Equals() below? (Consider adding a comment, either here or in Equals().) #Closed

Removed. just an oversight.

cston · 2022-05-06T20:47:20Z

        var tagHelpersFromCompilation = context.CompilationProvider

compilation?

In reply to: 1119997110

Refers to: src/RazorSdk/SourceGenerators/RazorSourceGenerator.cs:91 in 28b694b. [](commit_id = 28b694b, deletion_comment = False)

cston · 2022-05-06T20:50:03Z

src/RazorSdk/SourceGenerators/RazorSourceGenerator.cs

                .Combine(importFiles.Collect())
                .Combine(allTagHelpers)
                .Combine(razorSourceGeneratorOptions)
-                .Combine(context.ParseOptionsProvider)


.Combine(context.ParseOptionsProvider)

Was context.ParseOptionsProvider unnecessary? (It looks like it was unnecessary, given that parserOptions was not used inside the Select() lambda expression, but just confirming my understanding.)

Yes, it wasn't actually being used.

cston · 2022-05-06T21:17:46Z

src/Tests/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/RazorSourceGeneratorTests.cs

-                updatedOptionsProvider.TestGlobalOptions[option.Key] = option.Value;
-            }
+            // now run the generator with suppression
+            var supressedOptions = optionsProvider.Clone();


supressed

Typo. #Closed

cston · 2022-05-06T21:18:29Z

src/Tests/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/RazorSourceGeneratorTests.cs


-            driver = driver.WithUpdatedAnalyzerConfigOptions(updatedOptionsProvider);
-            result = RunGenerator(compilation!, ref driver);
+            // now unsupress and re-run


unsupress

Typo. #Closed

cston · 2022-05-06T21:31:49Z

src/Tests/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/RazorSourceGeneratorTests.cs

+                    catch (EqualException e)
+                    {
+                        Assert.False(true, $"No diff supplied. But index {i} was:\r\n\r\n{e.Actual.Replace("\"", "\"\"")}");
+                    }


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

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.

RikkiGibson · 2022-05-09T22:03:56Z

src/RazorSdk/SourceGenerators/RazorSourceGenerationOptions.cs

-        /// For instance <c>dotnet msbuild /p:_RazorSourceGeneratorDebug=true</c>
-        /// </para>
-        /// </summary>
-        public bool WaitForDebugger { get; set; } = false;


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?

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.

RikkiGibson · 2022-05-09T22:05:05Z

src/RazorSdk/SourceGenerators/IncrementalValueProviderExtensions.cs

+                if (y.IsSuppressed)
+                {
+                    // If source generation is suppressed, we can always use previously cached results.
+                    return true;


What's the meaning of x.IsSuppressed in this code path? Will it always also be true here?

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.

RikkiGibson · 2022-05-09T22:58:13Z

src/RazorSdk/SourceGenerators/RazorSourceGenerator.cs

                .Select(static (pair, _) =>
                {
-                    var ((((sourceItem, imports), allTagHelpers), razorSourceGeneratorOptions), parserOptions) = pair;
+                    var (((sourceItem, imports), allTagHelpers), razorSourceGeneratorOptions) = pair;


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.

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.

RikkiGibson · 2022-05-16T22:52:55Z

src/Tests/Microsoft.NET.Sdk.Razor.SourceGenerators.Tests/RazorSourceGeneratorTests.cs

        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');


don't we have a .NormalizeWhitespace() or similar helper that can remove the guesswork here?

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.

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.

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.

Cache pipeline inputs when suppressed:

2d6359e

- 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

ghost added the Area-Infrastructure label Apr 19, 2022

chsienki added Area-AspNetCore RazorSDK, BlazorWebAssemblySDK, StaticWebAssetsSDK and removed Area-Infrastructure labels Apr 19, 2022

chsienki marked this pull request as ready for review April 27, 2022 20:56

chsienki requested review from captainsafia and mkArtakMSFT as code owners April 27, 2022 20:56

RikkiGibson self-assigned this Apr 27, 2022

cston reviewed Apr 29, 2022

View reviewed changes

cston reviewed May 2, 2022

View reviewed changes

chsienki added 2 commits May 5, 2022 11:27

PR Feedback

f3cf646

Extract out suppression from the main options to simplify

28b694b

chsienki mentioned this pull request May 5, 2022

Hot Reload Takes Nearly 45 Seconds to Update dotnet/razor#7626

Closed

pranavkm reviewed May 6, 2022

View reviewed changes

cston reviewed May 6, 2022

View reviewed changes

PR Feedback

8925dd3

chsienki force-pushed the chsienki/razor-suppress-caching branch from 48246cc to 8925dd3 Compare May 6, 2022 22:46

cston approved these changes May 6, 2022

View reviewed changes

RikkiGibson approved these changes May 9, 2022

View reviewed changes

chsienki added 4 commits May 12, 2022 16:22

Don't compare pragmas in tests

c1cf119

Fix line ending diffs

a739e83

Correct char replacement

a80486b

Fix pragma filtering

1a3b066

RikkiGibson reviewed May 16, 2022

View reviewed changes

chsienki added 2 commits May 17, 2022 12:01

(*sigh*) Replace internal newlines

746bb12

Remove check with embedded file hash

4696442

chsienki merged commit 033522e into dotnet:main May 23, 2022

jasonmalinowski mentioned this pull request Jun 3, 2022

Add a hack to remove output from the Razor generator if suppressed dotnet/roslyn#61675

Merged

jasonmalinowski mentioned this pull request Jun 3, 2022

Ensure the Razor generator doesn't generate when it's suppressed #25833

Closed

jasonmalinowski mentioned this pull request Jun 16, 2022

Ensure the Razor generator doesn't generate when it's suppressed dotnet/razor-compiler#267

Closed

chsienki added a commit to chsienki/sdk that referenced this pull request Jun 29, 2022

Revert "Cache pipeline inputs when suppressed: (dotnet#24928)"

a066f9d

This reverts commit 033522e.

chsienki added a commit to chsienki/sdk that referenced this pull request Jun 29, 2022

Revert "Cache pipeline inputs when suppressed: (dotnet#24928)"

547e156

This reverts commit 033522e.

Conversation

chsienki commented Apr 19, 2022

Uh oh!

chsienki commented Apr 27, 2022

Uh oh!

cston Apr 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston commented May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston commented May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pranavkm May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston commented May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Apr 29, 2022 •

edited

Loading

cston May 2, 2022 •

edited

Loading

cston May 2, 2022 •

edited

Loading

cston commented May 2, 2022 •

edited

Loading

cston commented May 2, 2022 •

edited

Loading

pranavkm May 6, 2022 •

edited

Loading

cston May 6, 2022 •

edited

Loading

cston May 6, 2022 •

edited

Loading

cston May 6, 2022 •

edited

Loading

cston May 6, 2022 •

edited

Loading

cston commented May 6, 2022 •

edited

Loading

cston May 6, 2022 •

edited

Loading

cston May 6, 2022 •

edited

Loading

cston May 6, 2022 •

edited

Loading

cston May 6, 2022 •

edited

Loading

RikkiGibson May 9, 2022 •

edited

Loading