Fix incrementalism when suppressed: #9717

chsienki · 2023-12-16T00:28:01Z

Previously when going from unsuppressed -> suppressed we cleared certain caches
This meant that when going from suppressed -> unsuppressed we were always re compiling every razor file
Now instead, we produce nothing until the generator is unsuppressed, after which we just say that whatever is in the caches is up to date if we're suppressed.
This ensures no downstream processing takes place when the generator is suppressed, but there is still data to incrementally update when unsuppressed
We still remove the generated files at the last step when suppressed, so there is no output, but don't have to recompile anything to get the output back again.

Helps (at least for second edit) #7626

- Previously when going from unsuppressed -> suppressed we cleared certan caches - This meant that when going from suppressed -> unsuppressed we were always re compiling every razor file - Now instead, we produce nothing *until* the generator is unsuppressed, after which we just say that whatever is in the caches is up to date if we're suppressed. - This ensures no downstream processing takes place when the generator is suppressed, but there is still data to incrementally update when unsuppressed - We still remove the generated files at the last step when suppressed, so there is no output, but don't have to recompile anything to get the output back again.

Mike-E-angelo · 2023-12-16T10:45:38Z

Baby steps. Thank you for your efforts out there @chsienki & team. 🙏

src/Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators/IncrementalValueProviderExtensions.cs

jjonescz · 2023-12-18T08:46:53Z

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

+            Assert.Empty(eventListener.Events);
+
+            // Now enable the generator and confirm we get the expected output
+            driver = SetSuppressionState(false);


From the linked issue I understand this happens during hot reload. Can you share more details when do we suppress and unsuppress the generator there?

By default, in Roslyn the suppression state flag is set. If you never do a hot reload it stays that way, and so the generator never runs. When you do a hot reload/EnC, it modifies the workspace to remove the flag, meaning the generator actually runs.

I had thought that because EnC forks the workspace it stays that way, and never turns it back off again, but it does seem to (hence this bug). So in the hot reload workspace the flag can essentially toggle on and off arbitrarily. This PR makes sure that when that happens we keep the caches full so the next 'on' phase doesn't have to re-create everything from scratch.

chsienki · 2024-01-16T22:50:10Z

@dotnet/razor-compiler for review please

src/Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators/IncrementalValueProviderExtensions.cs

jjonescz · 2024-01-17T09:53:55Z

src/Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators/RazorSourceGenerator.cs


            var razorSourceGeneratorOptions = analyzerConfigOptions
                .Combine(parseOptions)
-                .Combine(isGeneratorSuppressed)
+                .SuppressIfNeeded(isGeneratorSuppressed)


So I imagine this can happen?

The generator is not suppressed, the inputs (analyzerConfigOptions or parseOptions) are changing and hence we are recomputing the output (razorSourceGeneratorOptions). All is fine.

The generator is suppressed.

Some inputs change - but we report the output is up to date since the suppression flag is true.

The suppression flag changes to false but inputs don't change. Now we still report the output is up to date - but it's not since the options changed in the previous step and we haven't recomputed the output. Seems bad.

This scenario is handled (we test is here: https://github.com/dotnet/razor/pull/9717/files#diff-21bee42801ceadff30fba484ddb46162fb1af8d819b27b06e2c1ebcd612f4feeR2619)

Some of the inputs all have EmptyOrCachedWhen on them: that means when the suppression flag changes, we'll re-evaluate them, and realize that one of the items is different.

The other inputs are all used in conjunction with SuppressIfNeeded that ensures they will also be re-evaluated when that value changes.

I amended the test to cover the compilation changing too.

The test adds a document, does that change the inputs here (analyzerConfigOptions or parseOptions)? Because I'm talking specifically about SuppressIfNeeded which seems it won't re-run if the inputs change while the suppression was on - since the lambda comparer inside is returning true while suppressed, then when unsuppressed, it compares the old with the new - but the old could have changed during the suppression, hence the old would be equal to new, but could be different from the state when the suppression was off previously. For example:

Suppression = off.

parseOptions = A -> razorSourceGeneratorOptions = A

parseOptions = B -> razorSourceGeneratorOptions = B

Suppression = on.

parseOptions = A -> razorSourceGeneratorOptions = B (since SuppressIfNeeded reports "up to date" due to suppression)

Suppression = off -> razorSourceGeneratorOptions = B (since SuppressIfNeeded compares the previous value of parseOptions = A with the current value A hence "up to date") - seems wrong, should be changed to A.

Isn't that what could happen?

No, that should still work. Because we combine with the configOptions (which has to change to change suppression) ComputeRazorSourceGeneratorOptions still runs as needed and updates the options.

Added more to the test to cover that scenario.

Ok, thanks. Although it sounds like the SuppressIfNeeded works only due to the pipeline being constructed in a specific way (eg, here it works only because one of the inputs contains the suppression check). Might be worthwhile to add a warning to the doc comment of SuppressIfNeeded to make sure it's not used incorrectly in the future (if it can really behave as I describe if used on different inputs for example).

…talValueProviderExtensions.cs Co-authored-by: Jan Jones <jan.jones.cz@gmail.com>

chsienki added the area-compiler Umbrella for all compiler issues label Dec 16, 2023

chsienki added this to the 17.9 P3 milestone Dec 16, 2023

chsienki self-assigned this Dec 16, 2023

chsienki requested a review from a team as a code owner December 16, 2023 00:28

chsienki mentioned this pull request Dec 16, 2023

Hot Reload Takes Nearly 45 Seconds to Update #7626

Open

jjonescz reviewed Dec 18, 2023

View reviewed changes

Add comment

a841229

333fred approved these changes Jan 17, 2024

View reviewed changes

jjonescz reviewed Jan 17, 2024

View reviewed changes

chsienki and others added 3 commits January 17, 2024 11:42

Amend test to check compilation chaging during suppression

bb54da2

Update src/Compiler/Microsoft.NET.Sdk.Razor.SourceGenerators/Incremen…

a31a8f9

…talValueProviderExtensions.cs Co-authored-by: Jan Jones <jan.jones.cz@gmail.com>

Add parse options change to test

7d56659

jjonescz approved these changes Jan 17, 2024

View reviewed changes

chsienki merged commit cd0eeb4 into dotnet:main Jan 17, 2024
12 checks passed

chsienki deleted the incremental_suppression_fix branch January 17, 2024 23:35

ghost modified the milestones: 17.9 P3, Next Jan 17, 2024

Cosifne modified the milestones: Next, 17.10 P1 Jan 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix incrementalism when suppressed: #9717

Fix incrementalism when suppressed: #9717

chsienki commented Dec 16, 2023

Mike-E-angelo commented Dec 16, 2023

jjonescz Dec 18, 2023

chsienki Jan 16, 2024

chsienki commented Jan 16, 2024

jjonescz Jan 17, 2024

chsienki Jan 17, 2024

chsienki Jan 17, 2024

jjonescz Jan 17, 2024

chsienki Jan 17, 2024

chsienki Jan 17, 2024

jjonescz Jan 17, 2024 •

edited

Loading

Fix incrementalism when suppressed: #9717

Fix incrementalism when suppressed: #9717

Conversation

chsienki commented Dec 16, 2023

Mike-E-angelo commented Dec 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chsienki commented Jan 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjonescz Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

jjonescz Jan 17, 2024 •

edited

Loading