diff --git a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md index 8d9d75e12a8..2160b826702 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md +++ b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md @@ -147,6 +147,7 @@ * Import: Don't walk non-F# assemblies when labelling trait constraint sources (PR [#20090](https://github.com/dotnet/fsharp/pull/20090)) * Avoid per-instance lock object in InterruptibleLazy and DelayInitArrayMap (PR [#20088](https://github.com/dotnet/fsharp/pull/20088)) * IL: fix leaking binary view ([PR #20250](https://github.com/dotnet/fsharp/pull/20250)) +* FCS: fix races that made the background builder repeat work ([PR #20481](https://github.com/dotnet/fsharp/pull/20481)) ### Added diff --git a/src/Compiler/Service/BackgroundCompiler.fs b/src/Compiler/Service/BackgroundCompiler.fs index 50f9881260f..7bc27b023d2 100644 --- a/src/Compiler/Service/BackgroundCompiler.fs +++ b/src/Compiler/Service/BackgroundCompiler.fs @@ -416,12 +416,21 @@ type internal BackgroundCompiler // STATIC ROOT: FSharpLanguageServiceTestable.FSharpChecker.parseFileInProjectCache. Most recently used cache for parsing files. let parseFileCache = - MruCache( + MruCache( parseFileCacheSize, areSimilar = AreSimilarForParsing, areSame = AreSameForParsing ) + /// Parses that have not finished yet. They are kept apart from parseFileCache because it holds its older entries weakly, + /// and a node is not kept alive by the result its caller retains. + let parseFileInFlight = + MruCache>( + parseFileCacheSize, + areSimilar = AreSameForParsing, + areSame = AreSameForParsing + ) + // STATIC ROOT: FSharpLanguageServiceTestable.FSharpChecker.checkFileInProjectCache // /// Cache which holds recently seen type-checks. @@ -455,9 +464,6 @@ type internal BackgroundCompiler let tryGetBuilderNode options = incrementalBuildersCache.TryGet(AnyCallerThread, options) - let tryGetBuilder options : Async option = - tryGetBuilderNode options |> Option.map (fun x -> x.GetOrComputeValue()) - let tryGetSimilarBuilder options : Async option = incrementalBuildersCache.TryGetSimilar(AnyCallerThread, options) |> Option.map (fun x -> x.GetOrComputeValue()) @@ -475,10 +481,18 @@ type internal BackgroundCompiler incrementalBuildersCache.Set(AnyCallerThread, options, getBuilderNode) getBuilderNode) - let createAndGetBuilder (options, userOpName) = + /// Replaces the builder node the caller has observed (if any), unless a concurrent request already did so, + /// in which case that node is reused instead of creating a second builder for the same project. + let createAndGetBuilder (options, userOpName, observedNode: GraphNode<_> option) = async { let! ct = Async.CancellationToken - let getBuilderNode = createBuilderNode (options, userOpName, ct) + + let getBuilderNode = + lock gate (fun () -> + match tryGetBuilderNode options with + | Some node when not (observedNode |> Option.contains node) -> node + | _ -> createBuilderNode (options, userOpName, ct)) + return! getBuilderNode.GetOrComputeValue() } @@ -486,9 +500,9 @@ type internal BackgroundCompiler async { use! _holder = Cancellable.UseToken() - match tryGetBuilder options with - | Some getBuilder -> - match! getBuilder with + match tryGetBuilderNode options with + | Some node -> + match! node.GetOrComputeValue() with | builderOpt, creationDiags when builderOpt.IsNone || not builderOpt.Value.IsReferencesInvalidated -> return builderOpt, creationDiags | _ -> @@ -502,8 +516,8 @@ type internal BackgroundCompiler let key = (sourceFile, 0L, options) checkFileInProjectCache.RemoveAnySimilar(ltok, key))) - return! createAndGetBuilder (options, userOpName) - | _ -> return! createAndGetBuilder (options, userOpName) + return! createAndGetBuilder (options, userOpName, Some node) + | None -> return! createAndGetBuilder (options, userOpName, None) } let getSimilarOrCreateBuilder (options, userOpName) = @@ -560,6 +574,32 @@ type internal BackgroundCompiler checkFileInProjectCache.Set(ltok, key, res) res) + /// Ensures there is one parse per file, source and options while it runs; a finished parse lives in parseFileCache. + let getParseFileNode (key, parse: Async) = + parseCacheLock.AcquireLock(fun ltok -> + match parseFileCache.TryGet(ltok, key) with + | Some res -> GraphNode.FromResult res + | None -> + match parseFileInFlight.TryGet(ltok, key) with + | Some node -> node + | None -> + Interlocked.Increment(&actualParseFileCount) |> ignore + + let node = + GraphNode( + async { + try + let! res = parse + parseCacheLock.AcquireLock(fun ltok -> parseFileCache.Set(ltok, key, res)) + return res + finally + parseCacheLock.AcquireLock(fun ltok -> parseFileInFlight.RemoveAnySimilar(ltok, key)) + } + ) + + parseFileInFlight.Set(ltok, key, node) + node) + member _.ParseFile (fileName: string, sourceText: ISourceText, options: FSharpParsingOptions, cache: bool, flatErrors: bool, userOpName: string) = @@ -573,13 +613,8 @@ type internal BackgroundCompiler Activity.Tags.cache, cache.ToString() |] - if cache then - let hash = sourceText.GetHashCode() |> int64 - - match parseCacheLock.AcquireLock(fun ltok -> parseFileCache.TryGet(ltok, (fileName, hash, options))) with - | Some res -> return res - | None -> - Interlocked.Increment(&actualParseFileCount) |> ignore + let parse suggestNamesForErrors = + async { let! ct = Async.CancellationToken let parseDiagnostics, parseTree, anyErrors = @@ -594,27 +629,15 @@ type internal BackgroundCompiler ct ) - let res = - FSharpParseFileResults(parseDiagnostics, parseTree, anyErrors, options.SourceFiles) + return FSharpParseFileResults(parseDiagnostics, parseTree, anyErrors, options.SourceFiles) + } - parseCacheLock.AcquireLock(fun ltok -> parseFileCache.Set(ltok, (fileName, hash, options), res)) - return res + if cache then + let key = (fileName, sourceText.GetHashCode() |> int64, options) + let node = getParseFileNode (key, parse suggestNamesForErrors) + return! node.GetOrComputeValue() else - let! ct = Async.CancellationToken - - let parseDiagnostics, parseTree, anyErrors = - ParseAndCheckFile.parseFile ( - sourceText, - fileName, - options, - userOpName, - false, - flatErrors, - captureIdentifiersWhenParsing, - ct - ) - - return FSharpParseFileResults(parseDiagnostics, parseTree, anyErrors, options.SourceFiles) + return! parse false } /// Fetch the parse information from the background compiler (which checks w.r.t. the FileSystem API) diff --git a/src/Compiler/Service/IncrementalBuild.fs b/src/Compiler/Service/IncrementalBuild.fs index 4f02bda91e7..ed25223460d 100644 --- a/src/Compiler/Service/IncrementalBuild.fs +++ b/src/Compiler/Service/IncrementalBuild.fs @@ -1206,21 +1206,20 @@ type IncrementalBuilder(initialState: IncrementalBuilderInitialState, state: Inc let mutable currentState = state - let setCurrentState state cache (ct: CancellationToken) = + let updateCurrentState (update: IncrementalBuilderState -> IncrementalBuilderState) cache = async { + let! ct = Async.CancellationToken do! semaphore.WaitAsync(ct) |> Async.AwaitTask try ct.ThrowIfCancellationRequested() - currentState <- computeStampedFileNames initialState state cache + // Read the state only under the lock: concurrent updates starting from the same stale snapshot + // would each build a fresh chain of bound models and lose each other's notifications. + currentState <- computeStampedFileNames initialState (update currentState) cache finally semaphore.Release() |> ignore } - let checkFileTimeStamps (cache: TimeStampCache) = - async { - let! ct = Async.CancellationToken - do! setCurrentState currentState cache ct - } + let checkFileTimeStamps (cache: TimeStampCache) = updateCurrentState id cache do IncrementalBuilderEventTesting.MRU.Add(IncrementalBuilderEventTesting.IBECreated) @@ -1400,11 +1399,9 @@ type IncrementalBuilder(initialState: IncrementalBuilderInitialState, state: Inc async { let slotOfFile = builder.GetSlotOfFileName fileName let cache = TimeStampCache defaultTimeStamp - let! ct = Async.CancellationToken - do! setCurrentState - { currentState with - slots = currentState.slots |> List.updateAt slotOfFile (currentState.slots[slotOfFile].Notify timeStamp) } - cache ct + do! updateCurrentState + (fun state -> { state with slots = state.slots |> List.updateAt slotOfFile (state.slots[slotOfFile].Notify timeStamp) }) + cache } member _.SourceFiles = fileNames |> Seq.map (fun f -> f.Source.FilePath) |> List.ofSeq diff --git a/tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.Tests.fsproj b/tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.Tests.fsproj index e043d8554ad..fe67a1ca0ee 100644 --- a/tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.Tests.fsproj +++ b/tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.Tests.fsproj @@ -46,6 +46,7 @@ + diff --git a/tests/FSharp.Compiler.Service.Tests/IncrementalBuilderRaceTests.fs b/tests/FSharp.Compiler.Service.Tests/IncrementalBuilderRaceTests.fs new file mode 100644 index 00000000000..61465e036bb --- /dev/null +++ b/tests/FSharp.Compiler.Service.Tests/IncrementalBuilderRaceTests.fs @@ -0,0 +1,74 @@ +// Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information. +module FSharp.Compiler.Service.Tests.IncrementalBuilderRaceTests + +open System +open System.IO +open System.Collections.Concurrent +open FSharp.Compiler.CodeAnalysis +open FSharp.Compiler.Text +open FSharp.Compiler.Service.Tests.Common +open FSharp.Test.Assert +open Xunit + +[] +let ``Concurrent requests after a file change type check each file once`` () = + // A private checker because we subscribe to FileChecked. The incremental builder is what is under test. + let checker = FSharpChecker.Create(useTransparentCompiler = false) + + let dir = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()) + Directory.CreateDirectory dir |> ignore + + try + let fileNames = [| for i in 1 .. 5 -> Path.Combine(dir, $"File{i}.fs") |] + fileNames |> Array.iteri (fun i fileName -> File.WriteAllText(fileName, $"module File{i + 1}\nlet x = 1\n")) + + let dllName = Path.Combine(dir, "Project.dll") + let projFileName = Path.Combine(dir, "Project.fsproj") + let args = mkProjectCommandLineArgs (dllName, fileNames) + let options = { checker.GetProjectOptionsFromCommandLineArgs(projFileName, args) with SourceFiles = fileNames } + + let checkCounts = ConcurrentDictionary() + checker.FileChecked.Add(fun (fileName, _) -> checkCounts.AddOrUpdate(fileName, 1, (fun _ n -> n + 1)) |> ignore) + + let checkedFiles () = + checkCounts |> Seq.map (fun kv -> Path.GetFileName kv.Key, kv.Value) |> Seq.sortBy fst |> List.ofSeq + + checker.ParseAndCheckProject options |> Async.RunSynchronouslyImmediate |> ignore + checkedFiles () |> shouldEqual [ for i in 1 .. 5 -> $"File{i}.fs", 1 ] + + // Invalidate the whole chain by touching the first file, then hit the builder with many concurrent requests. + // Each of them used to re-stamp the files from the same stale snapshot and build its own chain of bound models. + checkCounts.Clear() + File.SetLastWriteTimeUtc(fileNames[0], DateTime.UtcNow.AddSeconds 2.0) + + Seq.init 50 (fun _ -> checker.ParseAndCheckProject options |> Async.Ignore) + |> Async.Parallel + |> Async.RunSynchronouslyImmediate + |> ignore + + checkedFiles () |> shouldEqual [ for i in 1 .. 5 -> $"File{i}.fs", 1 ] + finally + try Directory.Delete(dir, true) with _ -> () + +[] +let ``Parse results the caller retains are served from the cache after a collection`` () = + let checker = FSharpChecker.Create(useTransparentCompiler = false) + + // One file more than the cache holds strongly, so the oldest entry is only reachable through its result. + let fileNames = + [| for i in 1 .. EnvMisc.parseFileCacheSize + 1 -> Path.GetFullPath $"CacheProbe{i}.fs" |] + + let options = { FSharpParsingOptions.Default with SourceFiles = fileNames } + let sourceText = SourceText.ofString "module CacheProbe\nlet value = 1\n" + + let parse fileName = + checker.ParseFile(fileName, sourceText, options, cache = true) |> Async.RunSynchronouslyImmediate + + let retained = fileNames |> Array.map parse + + GC.Collect() + GC.WaitForPendingFinalizers() + GC.Collect() + + Object.ReferenceEquals(retained[0], parse fileNames[0]) |> shouldBeTrue + GC.KeepAlive retained