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

AsyncMemoize: do not cancel and restart jobs that are being requested #16547

Closed
wants to merge 52 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
52 commits
Select commit Hold shift + click to select a range
c3a6b0f
Improve AsyncMemoize tests
0101 Jan 16, 2024
de170ae
relax test condition
0101 Jan 16, 2024
af8fc6a
Revert "Cancellable: set token from node/async in features code (#163…
0101 Jan 16, 2024
0a9f728
remove UsingToken
0101 Jan 16, 2024
a03215e
remove UsingToken
0101 Jan 16, 2024
2ad0e81
test improvement
0101 Jan 16, 2024
5bdae17
test improvement
0101 Jan 16, 2024
c052c3f
Merge branch 'main' into revert-16348
0101 Jan 17, 2024
569b50a
Merge branch 'main' into improve-async-tests
0101 Jan 17, 2024
76223ff
Merge remote-tracking branch '0101/revert-16348' into asyncmemo
majocha Jan 17, 2024
f3f4559
Task.Yield
majocha Jan 17, 2024
4a12103
relax test condition
0101 Jan 17, 2024
c511763
Merge branch 'revert-16348' of github.com:0101/fsharp into revert-16348
0101 Jan 17, 2024
8ee674c
relax test condition
0101 Jan 17, 2024
c1fb312
unindent a bit
majocha Jan 17, 2024
77920a6
Merge remote-tracking branch '0101/revert-16348' into asyncmemo
majocha Jan 17, 2024
1a66d72
fantomas
majocha Jan 17, 2024
a8201eb
draft release notes
majocha Jan 17, 2024
8d00ee6
use thread-safe collections when collecting events from AsyncMemoize
0101 Jan 17, 2024
22cc080
use thread-safe collections when collecting events from AsyncMemoize
0101 Jan 17, 2024
da52d7c
try adding Cancellable.UsingToken in ProjectWorkflowBuilder
0101 Jan 17, 2024
058d256
Merge branch 'main' into revert-16348
0101 Jan 17, 2024
e0a81c6
this might be better
0101 Jan 17, 2024
de74123
sprinkle in some more Cancellable.UsingToken
0101 Jan 17, 2024
27b492a
revert
0101 Jan 17, 2024
ff13476
f
0101 Jan 17, 2024
adeeb88
Merge remote-tracking branch '0101/revert-16348' into asyncmemo
majocha Jan 17, 2024
a82f875
Merge branch 'main' into improve-async-tests
0101 Jan 17, 2024
6617652
Merge branch 'asyncmemo' of https://github.com/majocha/fsharp into as…
majocha Jan 17, 2024
78048ce
only posted cancel requests
majocha Jan 17, 2024
1cc0ffd
format
majocha Jan 17, 2024
4319d1d
don't restart while there are requestors
majocha Jan 18, 2024
9090f3f
add test
majocha Jan 18, 2024
36ed2ab
no restart
majocha Jan 18, 2024
607d1e1
wip rel notes
majocha Jan 18, 2024
6a06ad1
merge from improve-async.tests
majocha Jan 18, 2024
98e6271
Merge branch 'main' into asyncmemo-no-restart
majocha Jan 18, 2024
6132eae
try to fix test
majocha Jan 18, 2024
337ba5d
Merge branch 'asyncmemo-no-restart' of https://github.com/majocha/fsh…
majocha Jan 18, 2024
cb660ce
rel notes
majocha Jan 18, 2024
e23f203
fix flaky test
majocha Jan 18, 2024
e4e5b59
fix test again
majocha Jan 18, 2024
a6930de
Merge branch 'main' into asyncmemo-no-restart
0101 Jan 18, 2024
b152e6d
stringbuilder is not threadsafe
majocha Jan 19, 2024
2e0fa2f
merge test fix
majocha Jan 20, 2024
0001d0e
Merge branch 'main' into asyncmemo-no-restart
majocha Jan 20, 2024
c623ea8
try to fix test
majocha Jan 20, 2024
73643fd
just in case
majocha Jan 20, 2024
9fef34b
revert
majocha Jan 22, 2024
41a849c
tests
majocha Jan 22, 2024
298b8f0
Merge branch 'main' into asyncmemo-no-restart
majocha Jan 22, 2024
010a418
try fix test
majocha Jan 22, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion docs/release-notes/.FSharp.Compiler.Service/8.0.300.md
Expand Up @@ -13,4 +13,5 @@

* `implicitCtorSynPats` in `SynTypeDefnSimpleRepr.General` is now `SynPat option` instead of `SynSimplePats option`. ([PR #16425](https://github.com/dotnet/fsharp/pull/16425))
* `SyntaxVisitorBase<'T>.VisitSimplePats` now takes `SynPat` instead of `SynSimplePat list`. ([PR #16425](https://github.com/dotnet/fsharp/pull/16425))
* Reduce allocations in compiler checking via `ValueOption` usage ([PR #16323](https://github.com/dotnet/fsharp/pull/16323))
* Reduce allocations in compiler checking via `ValueOption` usage ([PR #16323](https://github.com/dotnet/fsharp/pull/16323))
* Rework cancellation in `AsyncMemoize`. ([PR #16547](https://github.com/dotnet/fsharp/pull/16547))
472 changes: 206 additions & 266 deletions src/Compiler/Facilities/AsyncMemoize.fs

Large diffs are not rendered by default.

24 changes: 1 addition & 23 deletions src/Compiler/Facilities/BuildGraph.fs
Expand Up @@ -17,14 +17,12 @@ let wrapThreadStaticInfo computation =
async {
let diagnosticsLogger = DiagnosticsThreadStatics.DiagnosticsLogger
let phase = DiagnosticsThreadStatics.BuildPhase
let ct = Cancellable.Token

try
return! computation
finally
DiagnosticsThreadStatics.DiagnosticsLogger <- diagnosticsLogger
DiagnosticsThreadStatics.BuildPhase <- phase
Cancellable.Token <- ct
}

type Async<'T> with
Expand Down Expand Up @@ -127,48 +125,28 @@ type NodeCode private () =
static member RunImmediate(computation: NodeCode<'T>, ct: CancellationToken) =
let diagnosticsLogger = DiagnosticsThreadStatics.DiagnosticsLogger
let phase = DiagnosticsThreadStatics.BuildPhase
let ct2 = Cancellable.Token

try
try
let work =
async {
DiagnosticsThreadStatics.DiagnosticsLogger <- diagnosticsLogger
DiagnosticsThreadStatics.BuildPhase <- phase
Cancellable.Token <- ct2
return! computation |> Async.AwaitNodeCode
}

Async.StartImmediateAsTask(work, cancellationToken = ct).Result
finally
DiagnosticsThreadStatics.DiagnosticsLogger <- diagnosticsLogger
DiagnosticsThreadStatics.BuildPhase <- phase
Cancellable.Token <- ct2
with :? AggregateException as ex when ex.InnerExceptions.Count = 1 ->
raise (ex.InnerExceptions[0])

static member RunImmediateWithoutCancellation(computation: NodeCode<'T>) =
NodeCode.RunImmediate(computation, CancellationToken.None)

static member StartAsTask_ForTesting(computation: NodeCode<'T>, ?ct: CancellationToken) =
let diagnosticsLogger = DiagnosticsThreadStatics.DiagnosticsLogger
let phase = DiagnosticsThreadStatics.BuildPhase
let ct2 = Cancellable.Token

try
let work =
async {
DiagnosticsThreadStatics.DiagnosticsLogger <- diagnosticsLogger
DiagnosticsThreadStatics.BuildPhase <- phase
Cancellable.Token <- ct2
return! computation |> Async.AwaitNodeCode
}

Async.StartAsTask(work, cancellationToken = defaultArg ct CancellationToken.None)
finally
DiagnosticsThreadStatics.DiagnosticsLogger <- diagnosticsLogger
DiagnosticsThreadStatics.BuildPhase <- phase
Cancellable.Token <- ct2
Async.StartAsTask(computation |> Async.AwaitNodeCode, ?cancellationToken = ct)
Comment on lines -154 to +149
Copy link
Member

Choose a reason for hiding this comment

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

Removing these bits looks suspicious to me. It's what keeps the needed parts of the compiler context when it switches threads.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I just replaced it with what we use in production? (AwaitNodeCode)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that I look at it, this code is different than wrapThreadStaticInfo.
Here DiagnosticsThreadStatics of the calling thread are passed onto the new thread.
In wrapThreadStaticInfo they are not:

let wrapThreadStaticInfo computation =
async {
let diagnosticsLogger = DiagnosticsThreadStatics.DiagnosticsLogger
let phase = DiagnosticsThreadStatics.BuildPhase
try
return! computation
finally
DiagnosticsThreadStatics.DiagnosticsLogger <- diagnosticsLogger
DiagnosticsThreadStatics.BuildPhase <- phase
}


static member CancellationToken = cancellationToken

Expand Down
3 changes: 0 additions & 3 deletions src/Compiler/Interactive/fsi.fs
Expand Up @@ -4089,7 +4089,6 @@ type FsiInteractionProcessor
?cancellationToken: CancellationToken
) =
let cancellationToken = defaultArg cancellationToken CancellationToken.None
use _ = Cancellable.UsingToken(cancellationToken)

if tokenizer.LexBuffer.IsPastEndOfStream then
let stepStatus =
Expand Down Expand Up @@ -4218,7 +4217,6 @@ type FsiInteractionProcessor

member _.EvalInteraction(ctok, sourceText, scriptFileName, diagnosticsLogger, ?cancellationToken) =
let cancellationToken = defaultArg cancellationToken CancellationToken.None
use _ = Cancellable.UsingToken(cancellationToken)
use _ = UseBuildPhase BuildPhase.Interactive
use _ = UseDiagnosticsLogger diagnosticsLogger
use _scope = SetCurrentUICultureForThread fsiOptions.FsiLCID
Expand Down Expand Up @@ -4895,7 +4893,6 @@ type FsiEvaluationSession
SpawnInteractiveServer(fsi, fsiOptions, fsiConsoleOutput)

use _ = UseBuildPhase BuildPhase.Interactive
use _ = Cancellable.UsingToken(CancellationToken.None)

if fsiOptions.Interact then
// page in the type check env
Expand Down
31 changes: 0 additions & 31 deletions src/Compiler/Service/BackgroundCompiler.fs
Expand Up @@ -574,9 +574,6 @@ type internal BackgroundCompiler
Activity.Tags.cache, cache.ToString()
|]

let! ct = Async.CancellationToken
use _ = Cancellable.UsingToken(ct)

if cache then
let hash = sourceText.GetHashCode() |> int64

Expand Down Expand Up @@ -629,9 +626,6 @@ type internal BackgroundCompiler
"BackgroundCompiler.GetBackgroundParseResultsForFileInProject"
[| Activity.Tags.fileName, fileName; Activity.Tags.userOpName, userOpName |]

let! ct = NodeCode.CancellationToken
use _ = Cancellable.UsingToken(ct)

let! builderOpt, creationDiags = getOrCreateBuilder (options, userOpName)

match builderOpt with
Expand Down Expand Up @@ -783,9 +777,6 @@ type internal BackgroundCompiler
Activity.Tags.userOpName, userOpName
|]

let! ct = NodeCode.CancellationToken
use _ = Cancellable.UsingToken(ct)

let! cachedResults =
node {
let! builderOpt, creationDiags = getAnyBuilder (options, userOpName)
Expand Down Expand Up @@ -846,9 +837,6 @@ type internal BackgroundCompiler
Activity.Tags.userOpName, userOpName
|]

let! ct = NodeCode.CancellationToken
use _ = Cancellable.UsingToken(ct)

let! builderOpt, creationDiags = getOrCreateBuilder (options, userOpName)

match builderOpt with
Expand Down Expand Up @@ -897,9 +885,6 @@ type internal BackgroundCompiler
Activity.Tags.userOpName, userOpName
|]

let! ct = NodeCode.CancellationToken
use _ = Cancellable.UsingToken(ct)

let! builderOpt, creationDiags = getOrCreateBuilder (options, userOpName)

match builderOpt with
Expand Down Expand Up @@ -969,9 +954,6 @@ type internal BackgroundCompiler
Activity.Tags.userOpName, userOpName
|]

let! ct = NodeCode.CancellationToken
use _ = Cancellable.UsingToken(ct)

let! builderOpt, _ = getOrCreateBuilder (options, userOpName)

match builderOpt with
Expand All @@ -991,9 +973,6 @@ type internal BackgroundCompiler
Activity.Tags.userOpName, userOpName
|]

let! ct = NodeCode.CancellationToken
use _ = Cancellable.UsingToken(ct)

let! builderOpt, creationDiags = getOrCreateBuilder (options, userOpName)

match builderOpt with
Expand Down Expand Up @@ -1134,9 +1113,6 @@ type internal BackgroundCompiler
Activity.Tags.userOpName, userOpName
|]

let! ct = NodeCode.CancellationToken
use _ = Cancellable.UsingToken(ct)

let! builderOpt, _ = getOrCreateBuilder (options, userOpName)

match builderOpt with
Expand Down Expand Up @@ -1185,8 +1161,6 @@ type internal BackgroundCompiler
/// Parse and typecheck the whole project (the implementation, called recursively as project graph is evaluated)
member private _.ParseAndCheckProjectImpl(options, userOpName) =
node {
let! ct = NodeCode.CancellationToken
use _ = Cancellable.UsingToken(ct)

let! builderOpt, creationDiags = getOrCreateBuilder (options, userOpName)

Expand Down Expand Up @@ -1327,9 +1301,6 @@ type internal BackgroundCompiler
// Do we assume .NET Framework references for scripts?
let assumeDotNetFramework = defaultArg assumeDotNetFramework true

let! ct = Cancellable.token ()
use _ = Cancellable.UsingToken(ct)

let extraFlags =
if previewEnabled then
[| "--langversion:preview" |]
Expand Down Expand Up @@ -1452,8 +1423,6 @@ type internal BackgroundCompiler
|]

async {
let! ct = Async.CancellationToken
use _ = Cancellable.UsingToken(ct)

let! ct = Async.CancellationToken
// If there was a similar entry (as there normally will have been) then re-establish an empty builder . This
Expand Down
2 changes: 0 additions & 2 deletions src/Compiler/Service/FSharpCheckerResults.fs
Expand Up @@ -3585,8 +3585,6 @@ type FsiInteractiveChecker(legacyReferenceResolver, tcConfig: TcConfig, tcGlobal

member _.ParseAndCheckInteraction(sourceText: ISourceText, ?userOpName: string) =
cancellable {
let! ct = Cancellable.token ()
use _ = Cancellable.UsingToken(ct)

let userOpName = defaultArg userOpName "Unknown"
let fileName = Path.Combine(tcConfig.implicitIncludeDir, "stdin.fsx")
Expand Down
25 changes: 1 addition & 24 deletions src/Compiler/Service/TransparentCompiler.fs
Expand Up @@ -1762,8 +1762,6 @@ type internal TransparentCompiler
node {
//use _ =
// Activity.start "ParseFile" [| Activity.Tags.fileName, fileName |> Path.GetFileName |]
let! ct = NodeCode.CancellationToken
use _ = Cancellable.UsingToken(ct)

// TODO: might need to deal with exceptions here:
let tcConfigB, sourceFileNames, _ = ComputeTcConfigBuilder projectSnapshot
Expand Down Expand Up @@ -1792,6 +1790,7 @@ type internal TransparentCompiler
ignore userOpName

node {

match! ComputeItemKeyStore(fileName, projectSnapshot) with
| None -> return Seq.empty
| Some itemKeyStore -> return itemKeyStore.FindAll symbol.Item
Expand All @@ -1818,8 +1817,6 @@ type internal TransparentCompiler
userOpName: string
) : NodeCode<FSharpCheckFileAnswer> =
node {
let! ct = NodeCode.CancellationToken
use _ = Cancellable.UsingToken(ct)

let! snapshot =
FSharpProjectSnapshot.FromOptions(options, fileName, fileVersion, sourceText)
Expand All @@ -1842,8 +1839,6 @@ type internal TransparentCompiler
userOpName: string
) : NodeCode<FSharpCheckFileAnswer option> =
node {
let! ct = NodeCode.CancellationToken
use _ = Cancellable.UsingToken(ct)

let! snapshot =
FSharpProjectSnapshot.FromOptions(options, fileName, fileVersion, sourceText)
Expand Down Expand Up @@ -1897,8 +1892,6 @@ type internal TransparentCompiler
userOpName: string
) : NodeCode<seq<range>> =
node {
let! ct = NodeCode.CancellationToken
use _ = Cancellable.UsingToken(ct)

ignore canInvalidateProject
let! snapshot = FSharpProjectSnapshot.FromOptions options |> NodeCode.AwaitAsync
Expand All @@ -1914,8 +1907,6 @@ type internal TransparentCompiler

member this.GetAssemblyData(options: FSharpProjectOptions, fileName, userOpName: string) : NodeCode<ProjectAssemblyDataResult> =
node {
let! ct = NodeCode.CancellationToken
use _ = Cancellable.UsingToken(ct)

let! snapshot = FSharpProjectSnapshot.FromOptions options |> NodeCode.AwaitAsync
return! this.GetAssemblyData(snapshot.ProjectSnapshot, fileName, userOpName)
Expand All @@ -1936,8 +1927,6 @@ type internal TransparentCompiler
userOpName: string
) : NodeCode<FSharpParseFileResults * FSharpCheckFileResults> =
node {
let! ct = NodeCode.CancellationToken
use _ = Cancellable.UsingToken(ct)

let! snapshot = FSharpProjectSnapshot.FromOptions options |> NodeCode.AwaitAsync

Expand All @@ -1953,8 +1942,6 @@ type internal TransparentCompiler
userOpName: string
) : NodeCode<FSharpParseFileResults> =
node {
let! ct = NodeCode.CancellationToken
use _ = Cancellable.UsingToken(ct)

let! snapshot = FSharpProjectSnapshot.FromOptions options |> NodeCode.AwaitAsync
return! this.ParseFile(fileName, snapshot.ProjectSnapshot, userOpName)
Expand All @@ -1969,8 +1956,6 @@ type internal TransparentCompiler
) : NodeCode<(FSharpParseFileResults * FSharpCheckFileResults) option> =
node {
ignore builder
let! ct = NodeCode.CancellationToken
use _ = Cancellable.UsingToken(ct)

let! snapshot =
FSharpProjectSnapshot.FromOptions(options, fileName, 1, sourceText)
Expand Down Expand Up @@ -2023,8 +2008,6 @@ type internal TransparentCompiler
) : NodeCode<EditorServices.SemanticClassificationView option> =
node {
ignore userOpName
let! ct = NodeCode.CancellationToken
use _ = Cancellable.UsingToken(ct)

let! snapshot = FSharpProjectSnapshot.FromOptions options |> NodeCode.AwaitAsync
return! ComputeSemanticClassification(fileName, snapshot.ProjectSnapshot)
Expand All @@ -2048,8 +2031,6 @@ type internal TransparentCompiler
userOpName: string
) : NodeCode<FSharpParseFileResults * FSharpCheckFileAnswer> =
node {
let! ct = NodeCode.CancellationToken
use _ = Cancellable.UsingToken(ct)

let! snapshot =
FSharpProjectSnapshot.FromOptions(options, fileName, fileVersion, sourceText)
Expand All @@ -2063,8 +2044,6 @@ type internal TransparentCompiler

member this.ParseAndCheckProject(options: FSharpProjectOptions, userOpName: string) : NodeCode<FSharpCheckProjectResults> =
node {
let! ct = NodeCode.CancellationToken
use _ = Cancellable.UsingToken(ct)

ignore userOpName
let! snapshot = FSharpProjectSnapshot.FromOptions options |> NodeCode.AwaitAsync
Expand All @@ -2073,8 +2052,6 @@ type internal TransparentCompiler

member this.ParseAndCheckProject(projectSnapshot: FSharpProjectSnapshot, userOpName: string) : NodeCode<FSharpCheckProjectResults> =
node {
let! ct = NodeCode.CancellationToken
use _ = Cancellable.UsingToken(ct)

ignore userOpName
return! ComputeParseAndCheckProject projectSnapshot.ProjectSnapshot
Expand Down
4 changes: 0 additions & 4 deletions src/Compiler/Service/service.fs
Expand Up @@ -348,8 +348,6 @@ type FSharpChecker
use _ = Activity.start "FSharpChecker.Compile" [| Activity.Tags.userOpName, _userOpName |]

async {
let! ct = Async.CancellationToken
use _ = Cancellable.UsingToken(ct)

let ctok = CompilationThreadToken()
return CompileHelpers.compileFromArgs (ctok, argv, legacyReferenceResolver, None, None)
Expand Down Expand Up @@ -485,8 +483,6 @@ type FSharpChecker
let userOpName = defaultArg userOpName "Unknown"

node {
let! ct = NodeCode.CancellationToken
use _ = Cancellable.UsingToken(ct)

if fastCheck <> Some true || not captureIdentifiersWhenParsing then
return! backgroundCompiler.FindReferencesInFile(fileName, options, symbol, canInvalidateProject, userOpName)
Expand Down