Skip to content
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

Cancel background work #3063

Merged
merged 15 commits into from
Jul 13, 2017
Merged

Cancel background work #3063

merged 15 commits into from
Jul 13, 2017

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented May 16, 2017

This is a WIP PR to extend #3061.

In #3061 I noticed that therre we some long running, non-cancellable operations on the background thread. This means, for example, that if you

  1. start by opening a file in a project P,
  2. switch to a file or script unrelated to project P

then there will be no intellisense or colorization in the script until the basic preparation of project P (and possibly the complete checking of its dependencies) is finished

This PR cancels background work when new foreground work arrives that will implicitly start replacement background work (e.g. to prepare a script).

@dsyme dsyme changed the title [WIP] [WIP] Cancel background work May 16, 2017
Trace.TraceInformation("Reactor: --> wait for background {0}.{1} ({2}), remaining {3}", bgUserOpName, bgOpName, bgOpArg, inbox.CurrentQueueLength)
while bgOp ctok do
Trace.TraceInformation("FCS: --> wait for background {0}.{1} ({2}), remaining {3}", bgUserOpName, bgOpName, bgOpArg, inbox.CurrentQueueLength)
bgOpCts <- new CancellationTokenSource()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we dispose the old bgOpCts first?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks

let time = Stopwatch()
time.Start()
let res = bgOp ctok
bgOpCts <- new CancellationTokenSource()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

@@ -10,7 +10,7 @@ open OptionsUIHelpers

module DefaultTuning =
let SemanticColorizationInitialDelay = 0 (* milliseconds *)
let UnusedDeclarationsAnalyzerInitialDelay = 1000 (* milliseconds *)
let UnusedDeclarationsAnalyzerInitialDelay = 0 (* milliseconds *)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think this is ok.

@dsyme
Copy link
Contributor Author

dsyme commented May 24, 2017

A quick update on this: I've been trying to use this in practice to get a sense if the cancellation makes for a better overall experience - for example I'm vaguely concerned there might be situations where the background builder never really makes progress because so many requests are coming in to FCS from things like the "SimplifyName" code fix.

I'll keep using it myself and keep an eye on things.

@dsyme dsyme changed the title [WIP] Cancel background work Cancel background work Jul 13, 2017
@dsyme
Copy link
Contributor Author

dsyme commented Jul 13, 2017

This is now ready

@KevinRansom KevinRansom merged commit f180548 into dotnet:master Jul 13, 2017
@dsyme
Copy link
Contributor Author

dsyme commented Jul 13, 2017

For the record, @KevinRansom asked me why I was originally hesitant about this PR. My reply:

It's not flakey (unless the act of cancellation exposes some latent bug). It cooperatively cancels the slice of the background build if language service requests come in. That could in theory mean that the background build never progresses if there is a never ending flood of language service requests (e.g. you endlessly type, move the mouse etc. very fast). But that would have happened anyway prior to the PR. So it’s ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants