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

[WIP] In-Memory documents for FCS #11588

Closed
wants to merge 40 commits into from
Closed

Conversation

TIHan
Copy link
Contributor

@TIHan TIHan commented May 20, 2021

@vzarytovskii and I started looking at this today.

Currently, FCS relies on the file system to be the source of truth for incremental builder.

This PR allows the user to update the F# document that incremental builder holds so we can effectively give incremental build the in-memory state from the IDE.

Acceptance Criteria

@vzarytovskii
Copy link
Member

vzarytovskii commented May 21, 2021

@TIHan We've had some issues with VS tests trying to delete documents, I wonder if it's because we forget to release the initial viewstream or something.
Gonna take a look.

Edit: it's not failing locally and on CI anymore, maybe was just a temprorary CI hiccup?

@TIHan TIHan mentioned this pull request Jun 9, 2021
/// </summary>
/// <param name="options">The options for the project or script, used to determine active --define conditionals and other options relevant to parsing.</param>
/// <param name="docs">FSharp documents</param>
member UpdateDocuments: options: FSharpProjectOptions * docs: FSharpDocument seq -> Async<unit>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should rename this to: UpdateBackgroundDocuments

@@ -57,12 +57,12 @@ type internal FSharpCheckerProvider
let checker =
FSharpChecker.Create(
projectCacheSize = settings.LanguageServicePerformance.ProjectCheckCacheSize,
keepAllBackgroundResolutions = false,
keepAllBackgroundResolutions = true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is temporarily 'true'. It will be 'false' once we figure out a way to enable background resolutions only for open documents.

// Enabling this would mean that if devenv.exe goes above 2.3GB we do a one-off downsize of the F# Compiler Service caches
(* , MaxMemory = 2300 *)
legacyReferenceResolver=LegacyMSBuildReferenceResolver.getResolver(),
tryGetMetadataSnapshot = tryGetMetadataSnapshot,
keepAllBackgroundSymbolUses = false,
keepAllBackgroundSymbolUses = true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is temporarily 'true'. It will be 'false' once we figure out a way to enable background resolutions only for open documents.

@@ -1217,6 +1227,10 @@ type FSharpChecker(legacyReferenceResolver,
let userOpName = defaultArg userOpName "Unknown"
backgroundCompiler.InvalidateConfiguration(options, userOpName)

member _.UpdateBackgroundDocuments(options: FSharpProjectOptions, docs) =
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this can't be called concurrently, because underlying IncrementalBuilder setCurrentState

member _.SourceFiles = sourceFiles |> List.map (fun (_, f, _) -> f)
member _.SourceFiles = mainState.sourceFiles |> Seq.map (fun (_, f, _) -> f) |> List.ofSeq

member this.UpdateDocuments(docs: FSharpDocument seq) =
Copy link
Contributor

Choose a reason for hiding this comment

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

If two calls are made concurrently, one setting docA, another docB, then the update to one will be lost

@TIHan TIHan mentioned this pull request Aug 16, 2021
10 tasks
@jonsequitur jonsequitur mentioned this pull request Oct 7, 2021
13 tasks
@TIHan
Copy link
Contributor Author

TIHan commented Oct 26, 2021

Closing as we favor this PR: #12305

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.

3 participants