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

[Dev16 Preview2]Single File Management Refactor #5845

Merged
merged 26 commits into from
Nov 18, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
3944a88
Trying to fix dev16 with latest changes
TIHan Oct 25, 2018
9a6bb62
Dev16 is partially working
TIHan Oct 26, 2018
e5b98f8
Fixed several issues, should be working dev16
TIHan Oct 27, 2018
2ea60a7
Using try..with on computing options
TIHan Oct 29, 2018
3e85e8b
Deleting unused code
TIHan Oct 29, 2018
3d633bb
Minor refactor
TIHan Oct 29, 2018
736bf3a
Check command line option cps stamps for recompute as a backup
TIHan Oct 29, 2018
d05af2e
Using ProjectId for project options
TIHan Oct 29, 2018
415bdbb
Updating binoutputpath for legacy projects to workspace project
TIHan Oct 30, 2018
f4e0c88
Removing cps stamp as we don't need it
TIHan Oct 30, 2018
8169490
Updated roslyn package. Using IWorkspaceProjectContext.Id.
TIHan Oct 30, 2018
a099e6d
Handling single files (script files) differently
TIHan Oct 31, 2018
c7ead94
Removing dead code
TIHan Oct 31, 2018
ca4e2a9
Removing dead code
TIHan Oct 31, 2018
557d281
Removing dead code
TIHan Oct 31, 2018
5b9e6dd
Removing dead code
TIHan Oct 31, 2018
5d0e105
Removing dead code
TIHan Oct 31, 2018
a45274c
Removing ProjectSitesAndFiles.fs
TIHan Oct 31, 2018
61c976b
Initializing early to capture solution/workspace events
TIHan Oct 31, 2018
d50fce6
Handling rename of script files
TIHan Nov 1, 2018
ac0a91d
Merged into dev16
TIHan Nov 13, 2018
671e7aa
Delete Fsi.nuget.props
TIHan Nov 15, 2018
9b49622
Update Extensions.fs
TIHan Nov 15, 2018
44bf2a4
Create project per script file
TIHan Nov 18, 2018
13f579c
Make sure to dispose in fallback
TIHan Nov 18, 2018
ded440f
Don't need that
TIHan Nov 18, 2018
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions vsintegration/src/FSharp.Editor/Common/Constants.fs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ module internal FSharpConstants =
/// "FSharp"
let FSharpLanguageLongName = "FSharp"

[<Literal>]
/// "F# Miscellaneous Files"
let FSharpMiscellaneousFilesName = "F# Miscellaneous Files"

[<RequireQualifiedAccess>]
module internal FSharpProviderConstants =

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,12 @@ type internal FSharpDocumentDiagnosticAnalyzer() =
let! parsingOptions, _, projectOptions = projectInfoManager.TryGetOptionsForDocumentOrProject(document)
let! sourceText = document.GetTextAsync(cancellationToken)
let! textVersion = document.GetTextVersionAsync(cancellationToken)
return!
FSharpDocumentDiagnosticAnalyzer.GetDiagnostics(getChecker document, document.FilePath, sourceText, textVersion.GetHashCode(), parsingOptions, projectOptions, DiagnosticsType.Semantic)
|> liftAsync
if document.Project.Name <> FSharpConstants.FSharpMiscellaneousFilesName || isScriptFile document.FilePath then
return!
FSharpDocumentDiagnosticAnalyzer.GetDiagnostics(getChecker document, document.FilePath, sourceText, textVersion.GetHashCode(), parsingOptions, projectOptions, DiagnosticsType.Semantic)
|> liftAsync
else
return ImmutableArray<Diagnostic>.Empty
}
|> Async.map (Option.defaultValue ImmutableArray<Diagnostic>.Empty)
|> RoslynHelpers.StartAsyncAsTask cancellationToken
Expand Down
3 changes: 2 additions & 1 deletion vsintegration/src/FSharp.Editor/FSharp.Editor.fsproj
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,10 @@
<Compile Include="LanguageService\Symbols.fs" />
<Compile Include="LanguageService\FSharpCheckerExtensions.fs" />
<Compile Include="LanguageService\IProjectSite.fs" />
<Compile Include="LanguageService\ProjectSitesAndFiles.fs" />
<Compile Include="LanguageService\ProvideFSharpVersionRegistrationAttribute.fs" />
<Compile Include="LanguageService\FSharpCheckerProvider.fs" />
<Compile Include="LanguageService\FSharpProjectOptionsManager.fs" />
<Compile Include="LanguageService\SingleFileWorkspaceMap.fs" />
<Compile Include="LanguageService\LegacyProjectWorkspaceMap.fs" />
<Compile Include="LanguageService\LanguageService.fs" />
<Compile Include="LanguageService\AssemblyContentProvider.fs" />
Expand Down Expand Up @@ -153,6 +153,7 @@
<PackageReference Include="Microsoft.VisualStudio.Shell.Immutable.10.0" Version="$(MicrosoftVisualStudioShellImmutable100PackageVersion)" PrivateAssets="all" ExcludeAssets="runtime;contentFiles;build;analyzers;native" />
<PackageReference Include="Microsoft.VisualStudio.Shell.Immutable.11.0" Version="$(MicrosoftVisualStudioShellImmutable110PackageVersion)" PrivateAssets="all" ExcludeAssets="runtime;contentFiles;build;analyzers;native" />
<PackageReference Include="Microsoft.VisualStudio.Shell.Interop.11.0" Version="$(MicrosoftVisualStudioShellInterop110PackageVersion)" PrivateAssets="all" ExcludeAssets="runtime;contentFiles;build;analyzers;native" />
<PackageReference Include="Microsoft.VisualStudio.Shell.Interop.12.0" Version="$(MicrosoftVisualStudioShellInterop120PackageVersion)" PrivateAssets="all" ExcludeAssets="runtime;contentFiles;build;analyzers;native" />
<PackageReference Include="Microsoft.VisualStudio.Text.UI" Version="$(MicrosoftVisualStudioTextUIPackageVersion)" PrivateAssets="all" ExcludeAssets="runtime;contentFiles;build;analyzers;native" />
<PackageReference Include="Microsoft.VisualStudio.Text.UI.Wpf" Version="$(MicrosoftVisualStudioTextUIWpfPackageVersion)" PrivateAssets="all" ExcludeAssets="runtime;contentFiles;build;analyzers;native" />
<PackageReference Include="Microsoft.VisualStudio.TextManager.Interop" Version="$(MicrosoftVisualStudioTextManagerInteropPackageVersion)" PrivateAssets="all" ExcludeAssets="runtime;contentFiles;build;analyzers;native" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ open Microsoft.FSharp.Compiler.CompileOps
open Microsoft.FSharp.Compiler.SourceCodeServices
open Microsoft.VisualStudio
open Microsoft.VisualStudio.FSharp.Editor
open Microsoft.VisualStudio.FSharp.Editor.SiteProvider
open Microsoft.VisualStudio.LanguageServices
open Microsoft.VisualStudio.LanguageServices.Implementation.ProjectSystem
open Microsoft.VisualStudio.Shell
Expand All @@ -34,8 +33,6 @@ module private FSharpProjectOptionsHelpers =
{
new IProvideProjectSite with
member x.GetProjectSite() =
let fst (a, _, _) = a
let snd (_, b, _) = b
let mutable errorReporter =
let reporter = ProjectExternalErrorReporter(project.Id, "FS", serviceProvider)
Some(reporter:> IVsLanguageServiceBuildErrorReporter2)
Expand Down Expand Up @@ -83,8 +80,10 @@ module private FSharpProjectOptionsHelpers =

[<RequireQualifiedAccess>]
type private FSharpProjectOptionsMessage =
| TryGetOptions of Project * AsyncReplyChannel<(FSharpParsingOptions * FSharpProjectOptions) option>
| TryGetOptionsByDocument of Document * AsyncReplyChannel<(FSharpParsingOptions * FSharpProjectOptions) option>
| TryGetOptionsByProject of Project * AsyncReplyChannel<(FSharpParsingOptions * FSharpProjectOptions) option>
| ClearOptions of ProjectId
| ClearSingleFileOptionsCache of DocumentId

[<Sealed>]
type private FSharpProjectOptionsReactor (workspace: VisualStudioWorkspaceImpl, settings: EditorOptions, serviceProvider, checkerProvider: FSharpCheckerProvider) =
Expand All @@ -94,6 +93,49 @@ type private FSharpProjectOptionsReactor (workspace: VisualStudioWorkspaceImpl,
let cpsCommandLineOptions = new ConcurrentDictionary<ProjectId, string[] * string[]>()

let cache = Dictionary<ProjectId, VersionStamp * FSharpParsingOptions * FSharpProjectOptions>()
let singleFileCache = Dictionary<DocumentId, VersionStamp * FSharpParsingOptions * FSharpProjectOptions>()

let rec tryComputeOptionsByFile (document: Document) =
async {
let! text = document.GetTextAsync() |> Async.AwaitTask
let! fileStamp = document.GetTextVersionAsync() |> Async.AwaitTask
let! scriptProjectOptions, _ = checkerProvider.Checker.GetProjectOptionsFromScript(document.FilePath, text.ToString(), DateTime.Now)
match singleFileCache.TryGetValue(document.Id) with
| false, _ ->
let projectOptions =
if isScriptFile document.FilePath then
scriptProjectOptions
else
{
ProjectFileName = document.FilePath
ProjectId = None
SourceFiles = [|document.FilePath|]
OtherOptions = [||]
ReferencedProjects = [||]
IsIncompleteTypeCheckEnvironment = false
UseScriptResolutionRules = SourceFile.MustBeSingleFileProject (Path.GetFileName(document.FilePath))
LoadTime = DateTime.Now
UnresolvedReferences = None
OriginalLoadReferences = []
ExtraProjectInfo= None
Stamp = Some(int64 (fileStamp.GetHashCode()))
}

checkerProvider.Checker.InvalidateConfiguration(projectOptions, startBackgroundCompileIfAlreadySeen = true, userOpName = "computeOptions")

let parsingOptions, _ = checkerProvider.Checker.GetParsingOptionsFromProjectOptions(projectOptions)

singleFileCache.[document.Id] <- (fileStamp, parsingOptions, projectOptions)

return Some(parsingOptions, projectOptions)

| true, (fileStamp2, parsingOptions, projectOptions) ->
if fileStamp <> fileStamp2 then
singleFileCache.Remove(document.Id) |> ignore
return! tryComputeOptionsByFile document
else
return Some(parsingOptions, projectOptions)
}

let rec tryComputeOptions (project: Project) =
let projectId = project.Id
Expand Down Expand Up @@ -194,24 +236,48 @@ type private FSharpProjectOptionsReactor (workspace: VisualStudioWorkspaceImpl,
async {
while true do
match! agent.Receive() with
| FSharpProjectOptionsMessage.TryGetOptions(project, reply) ->
| FSharpProjectOptionsMessage.TryGetOptionsByDocument(document, reply) ->
try
// For now, disallow miscellaneous workspace since we are using the hacky F# miscellaneous files project.
if document.Project.Solution.Workspace.Kind = WorkspaceKind.MiscellaneousFiles then
reply.Reply(None)
elif document.Project.Name = FSharpConstants.FSharpMiscellaneousFilesName then
let! options = tryComputeOptionsByFile document
reply.Reply(options)
else
reply.Reply(tryComputeOptions document.Project)
with
| _ ->
reply.Reply(None)
| FSharpProjectOptionsMessage.TryGetOptionsByProject(project, reply) ->
try
reply.Reply(tryComputeOptions project)
if project.Solution.Workspace.Kind = WorkspaceKind.MiscellaneousFiles || project.Name = FSharpConstants.FSharpMiscellaneousFilesName then
reply.Reply(None)
else
reply.Reply(tryComputeOptions project)
with
| _ ->
reply.Reply(None)
| FSharpProjectOptionsMessage.ClearOptions(projectId) ->
cache.Remove(projectId) |> ignore
| FSharpProjectOptionsMessage.ClearSingleFileOptionsCache(documentId) ->
singleFileCache.Remove(documentId) |> ignore
}

let agent = MailboxProcessor.Start((fun agent -> loop agent), cancellationToken = cancellationTokenSource.Token)

member __.TryGetOptionsByProjectAsync(project) =
agent.PostAndAsyncReply(fun reply -> FSharpProjectOptionsMessage.TryGetOptions(project, reply))
agent.PostAndAsyncReply(fun reply -> FSharpProjectOptionsMessage.TryGetOptionsByProject(project, reply))

member __.TryGetOptionsByDocumentAsync(document) =
agent.PostAndAsyncReply(fun reply -> FSharpProjectOptionsMessage.TryGetOptionsByDocument(document, reply))

member __.ClearOptionsByProjectId(projectId) =
agent.Post(FSharpProjectOptionsMessage.ClearOptions(projectId))

member __.ClearSingleFileOptionsCache(documentId) =
agent.Post(FSharpProjectOptionsMessage.ClearSingleFileOptionsCache(documentId))

member __.SetCpsCommandLineOptions(projectId, sourcePaths, options) =
cpsCommandLineOptions.[projectId] <- (sourcePaths, options)

Expand Down Expand Up @@ -241,11 +307,11 @@ type internal FSharpProjectOptionsManager
settings: EditorOptions
) =

let reactor = new FSharpProjectOptionsReactor(workspace, settings, serviceProvider, checkerProvider)
let projectDisplayNameOf projectFileName =
if String.IsNullOrWhiteSpace projectFileName then projectFileName
else Path.GetFileNameWithoutExtension projectFileName

// A table of information about single-file projects. Currently we only need the load time of each such file, plus
// the original options for editing
let singleFileProjectTable = ConcurrentDictionary<ProjectId, DateTime * FSharpParsingOptions * FSharpProjectOptions>()
let reactor = new FSharpProjectOptionsReactor(workspace, settings, serviceProvider, checkerProvider)

do
// We need to listen to this event for lifecycle purposes.
Expand All @@ -259,35 +325,8 @@ type internal FSharpProjectOptionsManager
member this.ClearInfoForProject(projectId:ProjectId) =
reactor.ClearOptionsByProjectId(projectId)

/// Clear a project from the single file project table
member this.ClearInfoForSingleFileProject(projectId) =
singleFileProjectTable.TryRemove(projectId) |> ignore

/// Update a project in the single file project table
member this.AddOrUpdateSingleFileProject(projectId, data) = singleFileProjectTable.[projectId] <- data

/// Get the exact options for a single-file script
member this.ComputeSingleFileOptions (tryGetOrCreateProjectId, fileName, loadTime, fileContents, solution) =
async {
let extraProjectInfo = Some(box workspace)
if SourceFile.MustBeSingleFileProject(fileName) then
// NOTE: we don't use a unique stamp for single files, instead comparing options structurally.
// This is because we repeatedly recompute the options.
let optionsStamp = None
let! options, _diagnostics = checkerProvider.Checker.GetProjectOptionsFromScript(fileName, fileContents, loadTime, [| |], ?extraProjectInfo=extraProjectInfo, ?optionsStamp=optionsStamp)
// NOTE: we don't use FCS cross-project references from scripts to projects. THe projects must have been
// compiled and #r will refer to files on disk
let referencedProjectFileNames = [| |]
let site = ProjectSitesAndFiles.CreateProjectSiteForScript(fileName, referencedProjectFileNames, options)
let deps, projectOptions = ProjectSitesAndFiles.GetProjectOptionsForProjectSite(settings.LanguageServicePerformance.EnableInMemoryCrossProjectReferences, site, serviceProvider, (tryGetOrCreateProjectId fileName), fileName, options.ExtraProjectInfo, solution, None)
let parsingOptions, _ = checkerProvider.Checker.GetParsingOptionsFromProjectOptions(projectOptions)
return (deps, parsingOptions, projectOptions)
else
let site = ProjectSitesAndFiles.ProjectSiteOfSingleFile(fileName)
let deps, projectOptions = ProjectSitesAndFiles.GetProjectOptionsForProjectSite(settings.LanguageServicePerformance.EnableInMemoryCrossProjectReferences, site, serviceProvider, (tryGetOrCreateProjectId fileName), fileName, extraProjectInfo, solution, None)
let parsingOptions, _ = checkerProvider.Checker.GetParsingOptionsFromProjectOptions(projectOptions)
return (deps, parsingOptions, projectOptions)
}
member this.ClearSingleFileOptionsCache(documentId) =
reactor.ClearSingleFileOptionsCache(documentId)

/// Get compilation defines relevant for syntax processing.
/// Quicker then TryGetOptionsForDocumentOrProject as it doesn't need to recompute the exact project
Expand All @@ -305,45 +344,20 @@ type internal FSharpProjectOptionsManager
/// Get the exact options for a document or project
member this.TryGetOptionsForDocumentOrProject(document: Document) =
async {
let projectId = document.Project.Id

// The options for a single-file script project are re-requested each time the file is analyzed. This is because the
// single-file project may contain #load and #r references which are changing as the user edits, and we may need to re-analyze
// to determine the latest settings. FCS keeps a cache to help ensure these are up-to-date.
match singleFileProjectTable.TryGetValue(projectId) with
| true, (loadTime, _, _) ->
try
let fileName = document.FilePath
let! cancellationToken = Async.CancellationToken
let! sourceText = document.GetTextAsync(cancellationToken) |> Async.AwaitTask
// NOTE: we don't use FCS cross-project references from scripts to projects. The projects must have been
// compiled and #r will refer to files on disk.
let tryGetOrCreateProjectId _ = None
let! _referencedProjectFileNames, parsingOptions, projectOptions = this.ComputeSingleFileOptions (tryGetOrCreateProjectId, fileName, loadTime, sourceText.ToString(), document.Project.Solution)
this.AddOrUpdateSingleFileProject(projectId, (loadTime, parsingOptions, projectOptions))
return Some (parsingOptions, None, projectOptions)
with ex ->
Assert.Exception(ex)
return None
match! reactor.TryGetOptionsByDocumentAsync(document) with
| Some(parsingOptions, projectOptions) ->
return Some(parsingOptions, None, projectOptions)
| _ ->
match! reactor.TryGetOptionsByProjectAsync(document.Project) with
| Some(parsingOptions, projectOptions) ->
return Some(parsingOptions, None, projectOptions)
| _ ->
return None
return None
}

/// Get the options for a document or project relevant for syntax processing.
/// Quicker then TryGetOptionsForDocumentOrProject as it doesn't need to recompute the exact project options for a script.
member this.TryGetOptionsForEditingDocumentOrProject(document:Document) =
let projectId = document.Project.Id
Copy link
Contributor

Choose a reason for hiding this comment

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

@TIHan I don't understand this change in dev16.

#10156 is a synchronous call to this on the UI thread

As the comment to this method says, the point of "TryGetOptionsForEditingDocumentOrProject" is to get editing options quickly for scripts, in the cases where we need them fast on the UI thread for editing operations. But this change removed this and now this method computed the exact options.

I believe this is a primary cause for slower script editing in dev16, e.g. #6646, and is related to the UI thread aspects of #8757

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking back, I don't think I understood this well enough at the time and made the change in hopes to normalize everything.

If this call was being invoked on the UI thread somewhere, I can totally understand why there would be a performance issue.

match singleFileProjectTable.TryGetValue(projectId) with
| true, (_loadTime, parsingOptions, originalOptions) -> async { return Some (parsingOptions, originalOptions) }
| _ ->
async {
let! result = this.TryGetOptionsForDocumentOrProject(document)
return result |> Option.map(fun (parsingOptions, _, projectOptions) -> parsingOptions, projectOptions)
}
async {
let! result = this.TryGetOptionsForDocumentOrProject(document)
return result |> Option.map(fun (parsingOptions, _, projectOptions) -> parsingOptions, projectOptions)
}

[<Export>]
/// This handles commandline change notifications from the Dotnet Project-system
Expand Down
Loading