Respond to cancellation when a generator is running, so we can still report time. #57122

chsienki · 2021-10-13T17:13:31Z

Expose ElapsedTime publicly
Create a derived type for run results that expresses cancellation

chsienki · 2021-10-13T17:16:21Z

src/Compilers/Core/Portable/SourceGeneration/GeneratorDriver.cs

@@ -259,9 +260,17 @@ internal GeneratorDriverState RunGeneratorsCore(Compilation compilation, Diagnos
                {
                    stateBuilder[i] = SetGeneratorException(MessageProvider, stateBuilder[i], state.Generators[i], ufe.InnerException, diagnosticsBag, generatorTimer.Elapsed);
                }
+                catch (OperationCanceledException)


This is a breaking change. Should we put an option in the GeneratorOptions to enable/disable it?

Alternatively, if we did that, we could remove the 'WasCancelled' flag from the run results. Presumably the caller could just check the cancellation token that was passed in?

A simpler alternative is to consider the want to know about cancellation: essentially, what generator was running when it occurred. Could we simply stuff the information in the cancellation exception, so the host can extract it?

So yes, this is a breaking change for all the people calling it which...might just be you and me? If you know of other callers that are broken then we should think about those.

That all said, even if we don't have people being broken, there's still a "principle of least surprise" here too where if you passed in a cancellation token but don't realize you have to check the flag, you are now getting broken results. My gut is saying make this behavior an opt-in flag.

Presumably the caller could just check the cancellation token that was passed in?

Depends how you want to handle the case of we requested cancellation but it wasn't observed. If a poorly behaving generator didn't observe it's cancellation request, having a separate flag would let us know whether the result actually was usable, even if we're going to cancel off later on our side. Only checking our own token would mean we can't tell the difference.

It would also mean we better make telemetry and flag generators that we are requesting to cancel but are coming back slowly and not cancelling.

If we did want that latter telemetry, it'd be ideal if we could know if some generators did cancel and some completed, i.e. move the Cancelled flag to GeneratorRunResult. I'm not sure if that's easy/hard to do though, and if you told me it's the latter I'd absolutely believe it.

move the Cancelled flag to GeneratorRunResult.

Hmm this is an interesting point, and fairly simple to do.

It occurs to me that we have a long standing issue that we're not being good cancellation citizens inside the generator driver either (#46528). I think if we did that we'd mostly be out of the 'cooperative' cancellation and no generator should be able to delay it too much.

~~Do we care much if the generator doesn't respect cancellation if we're able to intercept it?~~ Edit: actually V1 generators running on top of the framework will have quite a big opportunity to stall where we can't intercept. It does seem like it would be useful to know if we successfully cancelled the generator or not.

For V2 since you have more parallelism does it mean the requests can run on some other thread while you've observed the cancellation on the original thread? Because yeah, those aren't directly impactful to our responsiveness, but it does still mean that there's still CPU cores being tied up doing work we won't ultimately observe the result of; those are still CPU cores that could be doing something else more productive, or at least could be doing nothing and not killing your battery.

A simpler alternative is to consider the want to know about cancellation: essentially, what generator was running when it occurred. Could we simply stuff the information in the cancellation exception, so the host can extract it?

@chsienki I missed this earlier -- the framework does do things like this; there's TaskCanceledException that inherits from OperationCancelledException. I'd check with the framework folks though whether they really want others doing this or perhaps regret it.

Also, if some generators were running but others were complete, do we have any way to run only the cancelled ones again later?

Oh I was imagining just stuffing it in the Data property bag. Essentially, 'when cancellation occurred, this is the generator that was running' (along with potentially, 'it was cooperative' / 'we had to wait').

With that approach we'd need to re-run the successful generators again. Which actually is a really good point and strongly makes me prefer the approach we're going for.

chsienki · 2021-10-13T17:17:36Z

src/Compilers/Core/Portable/SourceGeneration/GeneratorDriver.cs

@@ -237,6 +237,7 @@ internal GeneratorDriverState RunGeneratorsCore(Compilation compilation, Diagnos
            }
            constantSourcesBuilder.Free();

+            bool cancelled = false;


We only track cancellation during driver execution, not during init etc. That means we still throw at those points, but not during execution. That seems... odd? Should we always return success when cancelled, and just have an empty run result?

I'm not terribly worried about people doing terribly long things in init, just because it'd be so hard to do. But as long as nothing in this design boxes us out of changing that later, should be good.

src/Compilers/Core/Portable/PublicAPI.Unshipped.txt

…report time. - Expose ElapsedTime publically - Expose WasCancelled flag

chsienki · 2021-11-01T23:13:22Z

src/Compilers/Core/Portable/SourceGeneration/GeneratorDriverOptions.cs

@@ -15,9 +15,18 @@ namespace Microsoft.CodeAnalysis
    {
        public readonly IncrementalGeneratorOutputKind DisabledOutputs;

+        public readonly bool EnableGracefulCancellation;


TBD: naming.

"EnableCancellationTiming"?

chsienki · 2021-11-01T23:13:37Z

src/Compilers/Core/Portable/SourceGeneration/RunResults.cs

+                LastGeneratorRunning = cancelledOn;
+            }
+
+            public GeneratorRunResult? LastGeneratorRunning { get; }


TBD: naming

We can't have more than one here if there's potential parallelism? I'm wondering if it makes sense to make this an ImmutableArray<> or something so we can have more down the road without revving the API.

chsienki · 2021-11-05T18:35:53Z

@jasonmalinowski does this look ok for what you need in the IDE?

Note we're still going to erroneously assign all the syntax walk time to which ever generator gets there first. Is that going to be a deal breaker we need to fix for the IDE work?

jasonmalinowski

I think. There's some small details but nothing blocking and nothing we can't easily do in a mop-up PR.

src/Compilers/Core/Portable/SourceGeneration/RunResults.cs

jasonmalinowski · 2021-11-05T20:07:25Z

src/Compilers/Core/Portable/SourceGeneration/RunResults.cs

@@ -70,6 +70,17 @@ public ImmutableArray<SyntaxTree> GeneratedTrees
                return _lazyGeneratedTrees;
            }
        }
+
+        public sealed class CancelledResult : GeneratorDriverRunResult


Want to call this CanceleldGeneratorDriverRunResult or something? "CancelledResult" is a bit vague, but my alternative seems like a mouthful.

I made it a nested class to avoid the naming issue too much, but happy to change it.

jasonmalinowski · 2021-11-05T20:07:45Z

src/Compilers/Core/Portable/SourceGeneration/RunResults.cs

+                LastGeneratorRunning = cancelledOn;
+            }
+
+            public GeneratorRunResult? LastGeneratorRunning { get; }


We can't have more than one here if there's potential parallelism? I'm wondering if it makes sense to make this an ImmutableArray<> or something so we can have more down the road without revving the API.

jasonmalinowski · 2021-11-05T20:08:23Z

src/Compilers/Core/Portable/SourceGeneration/RunResults.cs

+                LastGeneratorRunning = cancelledOn;
+            }
+
+            public GeneratorRunResult? LastGeneratorRunning { get; }


What does null mean?

Currently we'll always have a value, because we only return a cancelled result if the cancellation occurred during a generator run, but I didn't want to box us in to graceful cancellation at other points too. If we go with the array, then its easier as we can just return an empty one in that case.

jasonmalinowski · 2021-11-05T20:11:14Z

src/Compilers/Core/Portable/SourceGeneration/GeneratorDriverOptions.cs

@@ -15,9 +15,18 @@ namespace Microsoft.CodeAnalysis
    {
        public readonly IncrementalGeneratorOutputKind DisabledOutputs;

+        public readonly bool EnableGracefulCancellation;


"EnableCancellationTiming"?

jasonmalinowski · 2021-11-05T21:18:32Z

src/Compilers/Core/Portable/SourceGeneration/RunResults.cs

+                LastGeneratorRunning = cancelledOn;
+            }
+
+            public GeneratorRunResult? LastGeneratorRunning { get; }


Given this is a GeneratorRunResult, is there data that's usable other than the time? Will there be whatever results did come back up until the point of cancellation?

No. The results are empty. This was one of the things I was really unsure about and why I ended up going with a subtype rather than a property on the parent class.

It gets pretty confusing if we return partial results as we either:

Only return the ones we actually ran

You get a variable number of results back, and the host has to track which ones came back compared to the last run

Host has to decide what to do with the 'missing' results.

We return cached results for the ones that didn't run:

You need to add more properties to the results, and have the host go through and check them. The cached values may not actually compile now, if the source compilation has changed significantly.

That is true if we return nothing too, but at least everything is out of date, rather than some strange hybrid situation.

jasonmalinowski · 2021-11-05T21:21:00Z

Note we're still going to erroneously assign all the syntax walk time to which ever generator gets there first. Is that going to be a deal breaker we need to fix for the IDE work?

That wouldn't result in any API shape changes, right? I'd say no reason to block -- let's get this merged in so we can start our side of this, and we can bugfix later.

chsienki · 2021-11-10T18:14:33Z

@dotnet/roslyn-compiler for review please

333fred · 2021-11-18T00:29:41Z

@chsienki is there an approved issue for the public API stuff?

dotnet-issue-labeler bot added the Area-Compilers label Oct 13, 2021

chsienki commented Oct 13, 2021

View reviewed changes

jasonmalinowski reviewed Oct 13, 2021

View reviewed changes

src/Compilers/Core/Portable/PublicAPI.Unshipped.txt Outdated Show resolved Hide resolved

chsienki added 2 commits November 1, 2021 16:11

Respond to cancellation when a generator is running, so we can still …

5198f34

…report time. - Expose ElapsedTime publically - Expose WasCancelled flag

Create a derived type for cancelled run results

4b9d289

chsienki force-pushed the source-generators/timing-with-cancellation branch from 9c714a4 to 4b9d289 Compare November 1, 2021 23:12

chsienki commented Nov 1, 2021

View reviewed changes

Add extra test

8ec1772

runfoapp bot mentioned this pull request Nov 3, 2021

Flaky test: Roslyn.VisualStudio.IntegrationTests.CSharp.CSharpFindReferences.FindReferencesToCtor #57552

Closed

jasonmalinowski approved these changes Nov 5, 2021

View reviewed changes

PR Feedback

c82354e

runfoapp bot mentioned this pull request Nov 9, 2021

Timeout on Microsoft.VisualStudio.LanguageServices.CSharp.UnitTests.dll tests #55638

Open

chsienki marked this pull request as ready for review November 10, 2021 18:14

chsienki requested a review from a team as a code owner November 10, 2021 18:14

chsienki added the New Feature - Source Generators Source Generators label Nov 10, 2021

chsienki added this to the 17.1 milestone Nov 10, 2021

chsienki mentioned this pull request Dec 2, 2021

Source Generators: Graceful cancellation for timing #58080

Open

jinujoseph modified the milestones: 17.1, 17.2 Mar 2, 2022

arunchndr removed this from the 17.2 milestone Oct 3, 2022

arunchndr modified the milestones: 17.5, 17.5 P1 Oct 3, 2022

jcouv marked this pull request as draft February 3, 2023 00:22

arunchndr modified the milestones: 17.5, Backlog Sep 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respond to cancellation when a generator is running, so we can still report time. #57122

Respond to cancellation when a generator is running, so we can still report time. #57122

chsienki commented Oct 13, 2021 •

edited

Loading

chsienki Oct 13, 2021

chsienki Oct 13, 2021

jasonmalinowski Oct 13, 2021

jasonmalinowski Oct 13, 2021

jasonmalinowski Oct 13, 2021

chsienki Oct 13, 2021 •

edited

Loading

jasonmalinowski Oct 13, 2021

jasonmalinowski Oct 20, 2021

jasonmalinowski Oct 20, 2021

chsienki Oct 20, 2021

chsienki Oct 13, 2021

jasonmalinowski Oct 13, 2021

chsienki Nov 1, 2021

jasonmalinowski Nov 5, 2021

chsienki Nov 1, 2021

jasonmalinowski Nov 5, 2021

chsienki commented Nov 5, 2021

jasonmalinowski left a comment

jasonmalinowski Nov 5, 2021

chsienki Nov 9, 2021

jasonmalinowski Nov 5, 2021

jasonmalinowski Nov 5, 2021

chsienki Nov 9, 2021

jasonmalinowski Nov 5, 2021

jasonmalinowski Nov 5, 2021

chsienki Nov 9, 2021

jasonmalinowski commented Nov 5, 2021

chsienki commented Nov 10, 2021

333fred commented Nov 18, 2021

Respond to cancellation when a generator is running, so we can still report time. #57122

Are you sure you want to change the base?

Respond to cancellation when a generator is running, so we can still report time. #57122

Conversation

chsienki commented Oct 13, 2021 • edited Loading

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

chsienki Oct 13, 2021 • edited Loading

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

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

chsienki commented Nov 5, 2021

jasonmalinowski left a 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

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

jasonmalinowski commented Nov 5, 2021

chsienki commented Nov 10, 2021

333fred commented Nov 18, 2021

chsienki commented Oct 13, 2021 •

edited

Loading

chsienki Oct 13, 2021 •

edited

Loading