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

Cleanup eventually, cancellable, allow foreground type checking #11153

Merged
merged 19 commits into from
Mar 3, 2021

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented Feb 26, 2021

Based on conversations with @TIHan, this cleans up the whole eventually/cancellable code so what's going on is much clearer. It builds over #11151

Additionally, the FCS checking of files using *CheckFileInProject operations no longer run on the reactor thread but run on the caller thread. More specifically, using the terminology below, this PR changes the "TypeCheck" part of the CheckFileInProject operation from being "Queued-at-high-priority" (running on reactor thread) to "Concurrent" (runs on caller thread). The *CheckFileInProject operations still have some elements which are Queued-at-high-priority - in particular the preparation of the "prior state" before checking the file, hence the return type is still Async.

  1. repeatedlyProgressUntilDoneOrTimeShareOverOrCanceled is removed for the much simpler primitive Eventually.forceForTimeSlice.

  2. "Eventually" no longer propagates CompilationThreadToken but does propagate a cancellation token. This makes the relationships Cancellable --> Eventually and Cancellable --> Async clearer, through operations

    Eventually.ofCancellable (lifting = no loss of capability)
    Cancellable.toAsync (lifting = no loss of capability)
    Eventually.toCancellable (loss of capability to timeslice)
    
  3. The background op of the Reactor becomes an Eventually<unit>. That is the op may be run for a time slice and then continued

Operations queue terminology

I've updated the doc here https://github.com/dotnet/fsharp/pull/11153/files#diff-5559c50e82ea049a7d5af789014c903e43fde092e13a555c58f8798b167c74ed

@dsyme dsyme changed the title Cleanup eventually, cancellable Cleanup eventually, cancellable, allow foreground type checking Feb 26, 2021
@dsyme dsyme changed the base branch from cleanup/ac1 to main February 26, 2021 01:46
@dsyme dsyme changed the base branch from main to cleanup/ac1 February 26, 2021 12:15
@cartermp
Copy link
Contributor

@dsyme could you add a note in the FCS docs about foreground vs. background checking? I honestly don't know the difference here and with all the changes that have gone in to simplify the incremental builder and move things off the reactor, we'll need to update docs to accurately reflect the nature of this stuff

src/fsharp/absil/illib.fs Outdated Show resolved Hide resolved
@dsyme dsyme changed the base branch from cleanup/ac1 to main March 1, 2021 14:26
@dsyme
Copy link
Contributor Author

dsyme commented Mar 1, 2021

@dsyme could you add a note in the FCS docs about foreground vs. background checking? I honestly don't know the difference here and with all the changes that have gone in to simplify the incremental builder and move things off the reactor, we'll need to update docs to accurately reflect the nature of this stuff

The document here is still largely valid https://github.com/dotnet/fsharp/blob/main/docs/fcs/queue.fsx. I'll double check it.

Essentially FCS currently runs a two-priority queue

  1. Higher priority "foreground" requests from the API (e.g. from FSharp.Editor). These are not typically on the UI thread but are associated with active actions by the user (editing a file etc.)

  2. Lower priority "background" job for preparing the project builder state of the current project being worked on. The "background" work is intended to be divided into little chunks so it can always be interrupted in order to service the higher-priority work.

Some requests from FSharp.Editor are serviced concurrently without using the queue at all. Anything with "Async" in the return type is likely to representa foreground operation.

I'm not particularly tied to the terminology.

Our intent is to eventually remove the queue altogether while keeping the background prepare-the-current-project work. However the queue has several operational impacts we need to be mindful of

  • It acts as a brake on the overall resource usage (if 1000 requests get made from FSharp.Editor they are serviced one at a time, and the work is not generally repeated as it get cached).

  • It acts as a data-lock on the project builder compilation state, not all of which is thread safe (though we keep reviewing it).

@dsyme
Copy link
Contributor Author

dsyme commented Mar 3, 2021

@TIHan This is getting close to being ready

  • I used it myself with VisualFSharp.sln extensively yesterday and everything felt much, much snappier in the IDE. It would be good to think if we can quantify this.

  • I did notice some glitches where strange red-squiggly errors were appearing but I think they were related to the unit tests I fixed today.

  • I had to rewrite "Eventually" to allow for injecting a CompilationsGlobalScope on each restart of the eventually.

  • Along the way I noticed some testing bugs where GetProjectOptionsFromCommandLineArgs was not adding the COMPILED, EDITING and INTERACTIVE defines to the computed project options, resulting in repeated reparsing of files under slightly different conditional defines. This is fixed.

  • Eventually/Cancellable code is hard to debug, as is code being processed by the reactor queue. I'd like to understand if ther's anything we can do to improve this. I played around with using the "inlining trick" to associate the second-phase lambdas in the eventually/cancellable code for things like "bind" with the appropriate code locations and this makes a significant improvement to the stacks.

@cartermp
Copy link
Contributor

cartermp commented Mar 3, 2021

Eventually/Cancellable code is hard to debug, as is code being processed by the reactor queue. I'd like to understand if ther's anything we can do to improve this. I played around with using the "inlining trick" to associate the second-phase lambdas in the eventually/cancellable code for things like "bind" with the appropriate code locations and this makes a significant improvement to the stacks.

@dsyme heh, I filed this puppy about that: #5586

@dsyme
Copy link
Contributor Author

dsyme commented Mar 3, 2021

@TIHan this is ready once green (there was a transient FSharp.Core test failure on Mac, but the PR doesn't touch the FSharp.Core code or tests, it is unrelated)

@dsyme
Copy link
Contributor Author

dsyme commented Mar 3, 2021

Hmmmm

  1. I'm seeing a strange error when using this with VisualFSharp.sln. I need to check if this is present in main

image

  1. There is a new test failure after merging with main
2021-03-03T14:04:21.7982400Z   Failed FSharp.Compiler.Scripting.DependencyManager.UnitTests.DependencyManagerInteractiveTests.Multiple Instances of DependencyProvider should be isolated [2 s]
2021-03-03T14:04:21.7983718Z   Error Message:
2021-03-03T14:04:21.7984188Z    Assert.True() Failure
2021-03-03T14:04:21.7984600Z Expected: True
2021-03-03T14:04:21.7985019Z Actual:   False
2021-03-03T14:04:21.7985434Z   Stack Trace:
2021-03-03T14:04:21.7986342Z      at FSharp.Compiler.Scripting.DependencyManager.UnitTests.DependencyManagerInteractiveTests.Multiple Instances of DependencyProvider should be isolated() in D:\workspace\_work\1\s\tests\FSharp.Compiler.Private.Scripting.UnitTests\DependencyManagerInteractiveTests.fs:line 170

@dsyme
Copy link
Contributor Author

dsyme commented Mar 3, 2021

@TIHan this is now ready

Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Changes all look good/reasonable to me, though this incorporates several API changes to FCS and so we'll need to do an FCS 40. We already have API breaking changes to account for anyways, just more of a note.

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.

None yet

3 participants