Fixed multi-targeted projects from invalidating the MruCache on document open/changed #4918

TIHan · 2018-05-15T22:52:37Z

Here are the things that this PR does:

Added a new field to FSharpProjectOptions, ProjectId. It is an optional string. If set to None, the cache will key off the ProjectFileName. When it is set, it will key off the ProjectId. This extends FCS to be able to handle multi-targeted projects since each target is a different project with the same ProjectFileName.
We clear the options table based on the ProjectId when a workspace ProjectRemoved event happens; this handles removing multi-targeted projects.

All of this should fix more perf issues related to multi-targeted projects, specifically when invalidating the cache. Type checking will still happen for each target, but that is what Roslyn does today; we could add option to only type check on the document in current context if people really want that.

TIHan · 2018-05-15T23:08:54Z

Not quite ready, I just tested some script files and they seem to have some trouble.

Pilchie

I'd like to understand the differences between those seemingly similar variables.

Pilchie · 2018-05-16T00:42:46Z

vsintegration/src/FSharp.Editor/Common/Extensions.fs

+type Microsoft.VisualStudio.LanguageServices.Implementation.ProjectSystem.VisualStudioWorkspaceImpl with
+
+    member this.GetCurrentProjectReferenceIds(projectId: ProjectId) =
+        match this.ProjectTracker.GetProject(projectId) with


Do you need to use the method that will cause the project's deferred state to be created here if it isn't already?

I don't think so if I'm understanding you correctly.

Pilchie · 2018-05-16T00:43:24Z

vsintegration/src/FSharp.Editor/Common/Extensions.fs

+    /// Gets a project file path. Throws if there is no project file path.
+    member this.GetProjectFilePath(projectId) =
+        match this.TryGetProjectFilePath(projectId) with
+        | None -> failwithf "Can't find project file path from %A." projectId


What happens to this error message? Should it be localized?

I don't know if should be localized. I have a habit of doing this sort of thing when I want a method to throw. These are internal methods too.

Pilchie · 2018-05-16T00:47:06Z

vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs

-                let site = ProjectSitesAndFiles.ProjectSiteOfSingleFile(fileName)
-                let deps, projectOptions = ProjectSitesAndFiles.GetProjectOptionsForProjectSite(Settings.LanguageServicePerformance.EnableInMemoryCrossProjectReferences, tryGetOptionsForReferencedProject, site, serviceProvider, (tryGetOrCreateProjectId fileName), fileName, extraProjectInfo, Some projectOptionsTable)
+                let site = ProjectSitesAndFiles.ProjectSiteOfSingleFile(filePath)
+                let _deps, projectOptions = ProjectSitesAndFiles.GetProjectOptionsForProjectSite(Settings.LanguageServicePerformance.EnableInMemoryCrossProjectReferences, tryGetOptionsForReferencedProject, site, serviceProvider, Some projectId, filePath, extraProjectInfo, Some projectOptionsTable)
                let parsingOptions, _ = checkerProvider.Checker.GetParsingOptionsFromProjectOptions(projectOptions)
                return (deps, parsingOptions, projectOptions)


Do you want to be returning deps or _deps here? What is the difference?

_deps means it's ignored or not used; same with _referencedProjectIds. Using _ makes the value not able to be used. At some point, GetProjectOptionsForProjectSite probably shouldn't return references to begin with.

Pilchie · 2018-05-16T00:49:57Z

vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs

+    member this.UpdateProjectInfo(projectId, site, userOpName, invalidateConfig) =
+        Logger.Log(LogEditorFunctionId.LanguageService_UpdateProjectInfo)
+
+        let referencedProjectIds = workspace.GetCurrentProjectReferenceIds(projectId) |> Seq.toArray


Same question on referencedProjectIds vs _ referencedProjectIds?

Pilchie · 2018-05-16T00:53:33Z

vsintegration/src/FSharp.Editor/LanguageService/ProjectSitesAndFiles.fs

@@ -274,6 +274,7 @@ type internal ProjectSitesAndFiles() =
        let option =
            let newOption () = {
                ProjectFileName = projectSite.ProjectFileName
+                ProjectId = projectId |> Option.map (fun x -> x.Id.ToString())


Not sure if this matters, but the ToString there is culture aware, so might return different results in different cultures (or if the thread's current culture changes between calls, or if calls happen on different threads that have different cultures).

That's a good point, I need to fix that.

In a recent change, I do the comparison with StringComparison.OrdinalIgnoreCase.

shouldn't it follow same pattern as above let strGuid = "_ProjectId=" + (match options.ProjectId with Some(projectId) -> projectId | _ -> "null") - mabe introduce a helper?

I was actually intending to specify a specific formatting (and possibly invariant culture) to both the ToString and the comparison.

Just OrdinalIgnoreCase is not the correct fix.

I also agree with @forki about extracting a helper to generated this string.

I think you are right that it's fine according to https://msdn.microsoft.com/en-us/library/97af8hh4(v=vs.110).aspx. I just don't like unspecified formats used for this sort of thing, since they go wrong in too many instances.

In that case, it may be better to make the field a Guid instead of a string so it's clear. What do you think?

I'm thinking it would force client to maintain a mapping between such guid and project name / target couples.

A simpler way for the client would be to concatenate the target name to the project name and use that as projectid, but this is only possible when ProjectId is a string.

What is wrong with leaving it case sensitive?

After some discussion, I will make the id case sensitive. Then I will be explicit with the ToString() to be the "D" format and make it lowercase.

TIHan · 2018-05-16T04:15:23Z

I was a bit over-zealous with some changes. I'm now going to try to do the bare minimum to get this to work correctly.

…ling multi-projects in a better way for LanguageService, making sure we clear it out of the options table

forki · 2018-05-16T05:39:51Z

src/fsharp/service/service.fs

@@ -2660,7 +2664,7 @@ type BackgroundCompiler(legacyReferenceResolver, projectCacheSize, keepAssemblyC
        let execWithReactorAsync action = reactor.EnqueueAndAwaitOpAsync(userOpName, "ParseAndCheckFileInProject", filename, action)
        async {
            try 
-                let strGuid = "_" + Guid.NewGuid().ToString()
+                let strGuid = "_ProjectId=" + (match options.ProjectId with Some(projectId) -> projectId | _ -> "null")


consider multi line pattern match

or defaultArg

or Option.defaultValue

forki · 2018-05-16T05:40:35Z

tests/service/MultiProjectAnalysisTests.fs

@@ -935,6 +935,7 @@ let ``Projects creating generated types should not utilize cross-project-referen
    let options = 
          {ProjectFileName =
            __SOURCE_DIRECTORY__ + @"/data/TypeProvidersBug/TestConsole/TestConsole.fsproj";
+           ProjectId = None;


please don't introduce new ;

auduchinok · 2018-05-16T08:16:14Z

src/fsharp/service/service.fs

@@ -1760,6 +1760,7 @@ type UnresolvedReferencesSet = UnresolvedReferencesSet of UnresolvedAssemblyRefe
 type FSharpProjectOptions =
    { 
      ProjectFileName: string
+      ProjectId: string option


Why it is optional? Could it be a unique Id string so it would be the only thing to compare in UseSameProject below?

I think it is more correct if it was that way. But existing code may not have an Id, unless they pass the file name again as the Id.

Existing client code has to be updated with some value anyway.

auduchinok · 2018-05-16T08:17:44Z

src/fsharp/service/service.fs

-        options1.ProjectFileName = options2.ProjectFileName
+    static member UseSameProject(options1,options2) =
+        match options1.ProjectId, options2.ProjectId with
+        | Some(projectId1), Some(projectId2) -> String.Equals(projectId1, projectId2, StringComparison.OrdinalIgnoreCase)


Why ignoring case? Consider using Ordinal?

Because this Id will most likely will be a Guid and I don't like the idea of making the guid comparisons case sensitive.

dsyme · 2018-05-16T09:15:08Z

I understand the change, it looks good. Basically

Before: keep at most one typecheck state for a project file name
After: keep at most one typecheck state for a project ID

That's fine, good fix

…ent open/changed (dotnet#4918) * Handling multi-targeting in FCS by adding an optional ProjectId; handling multi-projects in a better way for LanguageService, making sure we clear it out of the options table * Fixing tests with FSharpProjectOptions type changes * Fixed more tests * Fixed more tests * Fixed more tests * Fixed build

TIHan mentioned this pull request May 15, 2018

Completion performance for multitargeted projects is slow #4914

Closed

TIHan changed the title ~~Fixed multi-targeted projects from invalidating the MruCache on document open/changed~~ [WIP] Fixed multi-targeted projects from invalidating the MruCache on document open/changed May 15, 2018

Pilchie suggested changes May 16, 2018

View reviewed changes

TIHan force-pushed the perf-fixes2 branch from 82da8c5 to a8ade8f Compare May 16, 2018 04:14

Handling multi-targeting in FCS by adding an optional ProjectId; hand…

d2e9178

…ling multi-projects in a better way for LanguageService, making sure we clear it out of the options table

TIHan force-pushed the perf-fixes2 branch from a8ade8f to d2e9178 Compare May 16, 2018 04:54

TIHan added 2 commits May 15, 2018 22:01

Fixing tests with FSharpProjectOptions type changes

bcbb8ee

Fixed more tests

50a73b1

forki reviewed May 16, 2018

View reviewed changes

forki approved these changes May 16, 2018

View reviewed changes

Fixed more tests

335e5c5

TIHan changed the title ~~[WIP] Fixed multi-targeted projects from invalidating the MruCache on document open/changed~~ Fixed multi-targeted projects from invalidating the MruCache on document open/changed May 16, 2018

auduchinok reviewed May 16, 2018

View reviewed changes

dsyme approved these changes May 16, 2018

View reviewed changes

TIHan added 2 commits May 16, 2018 11:01

Fixed more tests

65327d6

Fixed build

d7c0b3f

TIHan merged commit a1f2f80 into dotnet:master May 16, 2018

TIHan deleted the perf-fixes2 branch May 16, 2018 19:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed multi-targeted projects from invalidating the MruCache on document open/changed #4918

Fixed multi-targeted projects from invalidating the MruCache on document open/changed #4918

TIHan commented May 15, 2018 •

edited

Loading

TIHan commented May 15, 2018

Pilchie left a comment

Pilchie May 16, 2018

TIHan May 16, 2018

Pilchie May 16, 2018

TIHan May 16, 2018

Pilchie May 16, 2018

TIHan May 16, 2018 •

edited

Loading

Pilchie May 16, 2018

Pilchie May 16, 2018

TIHan May 16, 2018

TIHan May 16, 2018

forki May 16, 2018

Pilchie May 17, 2018

Pilchie May 17, 2018

TIHan May 17, 2018

smoothdeveloper May 17, 2018 •

edited

Loading

TIHan May 17, 2018

Pilchie May 17, 2018

TIHan commented May 16, 2018

forki May 16, 2018

xuanduc987 May 16, 2018

vasily-kirichenko May 16, 2018

forki May 16, 2018

auduchinok May 16, 2018

TIHan May 16, 2018 •

edited

Loading

auduchinok May 17, 2018

auduchinok May 16, 2018

TIHan May 16, 2018 •

edited

Loading

dsyme commented May 16, 2018

Fixed multi-targeted projects from invalidating the MruCache on document open/changed #4918

Fixed multi-targeted projects from invalidating the MruCache on document open/changed #4918

Conversation

TIHan commented May 15, 2018 • edited Loading

TIHan commented May 15, 2018

Pilchie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TIHan May 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoothdeveloper May 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TIHan commented May 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TIHan May 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TIHan May 16, 2018 • edited Loading

Choose a reason for hiding this comment

dsyme commented May 16, 2018

TIHan commented May 15, 2018 •

edited

Loading

TIHan May 16, 2018 •

edited

Loading

smoothdeveloper May 17, 2018 •

edited

Loading

TIHan May 16, 2018 •

edited

Loading

TIHan May 16, 2018 •

edited

Loading