[WIP] Improve solution load performance #2909

saul · 2017-04-24T16:11:12Z

Fixes #2793, fixes #2916, fixes #1944.

This is a very WIP pull request that improves solution load performance by moving ComputeSourcesAndFlags from being a UI thread operation to a background thread operation.

The design consists of a new ProjectFlagsService which processes a queue of projects that are dirty and need their sources/flags recalculating. It takes an exclusive lock on VS's design-time build and then runs an asynchronous build of the project (stopping just before invoking the compiler).

This PR also contains changes to ProjectsSitesAndFiles options caching. These changes mean that the Dense solution loads in 20 seconds instead of hanging VS forever. The ProjectsSitesAndFiles changes brought the load time down to 2:50mins, and the off-loading of ComputeSourcesAndFlags brings it down further to 20 seconds.

Remaining work

Investigate whether Roslyn workspace setup can now be made synchronous
Revert changes conflicting with use graph of option objects - fix 2793 #3002
Use MEF for ProjectFlagsService
Prioritise the currently viewed project when the active document is changed
Be resilient to Path.GetFullPath failing in reference node properties
Fix sources and flags results not being passed down to the background compiler

… running asyncronously after load

saul · 2017-04-24T16:13:01Z

I could do with a hand with the following issues:

I can't get the ProjectFlagsService registered with MEF. When I do GetService on the site service provider in the ProjectSystem, it cannot find the service.
When InvalidateConfiguration is called on the background request when the sources and flags have been calculated by MSBuild, it doesn't seem to update IntelliSense (still squiggles everywhere)

vasily-kirichenko · 2017-04-24T16:22:37Z

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

@@ -208,7 +209,7 @@ type
                             CodeSense = true,
                             DefaultToNonHotURLs = true,
                             EnableCommenting = true,
-                             CodeSenseDelay = 100,
+                             CodeSenseDelay = 0,


Need to actually investigate if it's made a difference...

forki · 2017-04-24T16:23:49Z

is there already noticable difference?

saul · 2017-04-24T16:52:05Z

@forki with regards to what?

majocha · 2017-04-24T17:15:41Z

vsintegration/src/FSharp.ProjectSystem.FSharp/ProjectFlagsService.fs

+open Microsoft.Build.Execution
+
+(*
+[<Export(typeof<ProjectFlagsService>)>]


Just [<Export>] would do?

Maybe make it a Workspace Service instead like this http://source.roslyn.io/#Microsoft.CodeAnalysis.EditorFeatures/Shared/DefaultDocumentSupportsSuggestionService.cs?

...then query it like this http://source.roslyn.io/#Microsoft.CodeAnalysis.EditorFeatures/CommandHandlers/GoToAdjacentMemberCommandHandler.cs,53

Under the hood it still just uses ExportAttribute though - so I doubt that would help. Thanks for the input though :)

majocha · 2017-04-24T17:17:15Z

vsintegration/src/FSharp.ProjectSystem.FSharp/ProjectFlagsService.fs

+type internal ProjectFlagsService 
+    [<ImportingConstructor>]
+    (
+        [<Import(typeof<SVsBuildManagerAccessor>)>] accessor : IVsBuildManagerAccessor


I'm not sure you'll get this from MEF.

IServiceProvider GetService maybe?
like here:

https://github.com/Microsoft/visualfsharp/blob/9a6ac3da4249ae5c397d331c99c23eea8fb37bdc/vsintegration/src/FSharp.Editor/Options/SettingsPersistence.fs#L35-L42

Thanks, I'll give that a shot tomorrow.

Also in project.fs it looks like I need to call GetService on IComponentModel instead of on this.Site to actually get a reference to ProjectFlagsService 👍

See https://github.com/Microsoft/visualfsharp/blob/9a6ac3da4249ae5c397d331c99c23eea8fb37bdc/vsintegration/src/FSharp.Editor/Options/UIHelpers.fs#L24

I experimented a bit and it turns out exports from other assemblies like FSharp.Editor work fine and are resolved with IComponentModel. Anything declared here fails.

That's bizarre, thanks for helping me with this. @cartermp do you know anyone who could weigh in/explain why our project system assembly doesn't seem to be linked up to MEF?

I have no idea. @dsyme would you know, or know who might know?

@saul @cartermp I'd ask @OmarTawfik, @brettfo, @KevinRansom . It may be just historical. Note I know almost nothing about MEF

@saul might be helpful, too https://github.com/Microsoft/vs-mef

vasily-kirichenko · 2017-04-24T17:19:59Z

vsintegration/src/FSharp.ProjectSystem.FSharp/Project.fs

@@ -1558,7 +1608,7 @@ namespace rec Microsoft.VisualStudio.FSharp.ProjectSystem
                        Debug.Assert(System.Threading.Thread.CurrentThread.ManagedThreadId = uiThreadId, "called GetProjectSite() while still in Opening state from non-UI thread")
 #endif                        
                        x.ComputeSourcesAndFlags()
-                        if not(projectSite.State = ProjectSiteOptionLifetimeState.Opened) then
+                        if projectSite.State <> ProjectSiteOptionLifetimeState.Opened then


majocha · 2017-04-24T17:40:45Z

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

+            |> Seq.map(fun ref -> (getProject ref.ProjectId).FilePath)
+            |> hashSetIgnoreCase
+
+        for referencedSite in  do


updatedProjects.Except(workspaceProjects) ?

Yes I was wondering that... will change them to use that tomorrow 👍

forki · 2017-04-24T17:45:34Z

@saul I guess my question is if this work already shows performance benefits for your projects.

saul · 2017-04-24T17:46:58Z

@forki as in the PR description:

These changes mean that the Dense solution loads in 20 seconds instead of hanging VS forever. The ProjectsSitesAndFiles changes brought the load time down to 2:50mins, and the off-loading of ComputeSourcesAndFlags brings it down further to 20 seconds.

So yes, in large solutions with dense dependency graphs, it's essentially gone from impossible to load to 20 seconds 😃

saul · 2017-04-24T17:48:45Z

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

+                            let solution = workspace.CurrentSolution
+                            let documentIds = solution.GetDocumentIdsWithFilePath(fileName)
+                            if not documentIds.IsEmpty then 
+                                analyzerService.Reanalyze(workspace, documentIds=documentIds, highPriority=true)


Also wondering if this is going to get us faster squiggles in the editor. Will probably move this change out of this PR though.

saul · 2017-04-24T17:49:18Z

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

@@ -316,29 +328,50 @@ and
                projectContext.RemoveSourceFile(file)
                updated <- true

-        let updatedRefs = site.AssemblyReferences() |> hashSetIgnoreCase
+        (*


This will be put back - it's commented out as it's causing issues at the moment.

I may be wrong but IIRC project updates must run on UI thread, contrary to what some comments in Roslyn say.

That's fine with this PR - the sources/flags notifier (which eventually calls through to this) is running on the UI thread.

saul · 2017-04-24T17:50:25Z

vsintegration/src/FSharp.ProjectSystem.Base/Project/ProjectNode.cs

@@ -697,7 +697,7 @@ public override string Url

        public void SetDebugLogger(Microsoft.Build.Framework.ILogger debugLogger)
        {
-            myDebugLogger = debugLogger;
+            //myDebugLogger = debugLogger;


Temporary change, should not have committed

forki · 2017-04-24T17:50:43Z

How could I miss that. Probably just read the email.
Anyways: that's awesome! 💋

saul · 2017-04-24T17:56:32Z

Yeah I added the perf stats in after I posted. I assume you've got the repo setup to email you immediately whenever there's action 😆

saul · 2017-04-25T09:38:32Z

With regards to being able to export/get the ProjectFlagsService - any more ideas on this? Even if I use just [<Export>] and try to get the service via IComponentModel, it can never find it. I've no idea what I'm doing wrong - it seems to be exactly how other projects like PTVS and the GitHub VS extension use MEF.

cc @majocha

majocha · 2017-04-25T12:23:05Z

@saul from what I gather ProjectPackage, where you want to import it, is not MEF composed itself. To get the ProjectFlagsService there you would need to go through SComponentModel. I tried to get SComponentModel service and then import a small dummy part. It fails, so it's not just ProjectFlagsService.
Is it worth the trouble here? Do you plan on reusing ProjectFlagsService somewhere else?

saul · 2017-04-25T15:02:16Z

@majocha yeah I was hoping to use it in FSharp.Editor. I want to call PrioritiseProject when a document is opened.

…ion (add support changing project references) Fixes dotnet#2916, fixes dotnet#1944

saul · 2017-04-25T16:24:24Z

In the latest commit I've fixed #1944 and #2916 as I moved some LanguageService code over to use IVsRunningDocumentTable instead of hooking into SetupNewTextView (that's been removed entirely). I've also re-enabled the Roslyn workspace syncronisation that I disabled in the previous commit.

Also some more stats - the VisualFSharp solution now loads in 10 seconds (down from 20).

forki · 2017-04-25T16:50:29Z

That's so awesome. Can we already merge it and just iterate on it? Am 25.04.2017 6:24 nachm. schrieb "Saul Rennison" <notifications@github.com

:

In the latest commit I've fixed #1944 <#1944> and #2916 <#2916> as I moved some LanguageService code over to use IVsRunningDocumentTable instead of hooking into SetupNewTextView (that's been removed entirely). I've also re-enabled the Roslyn workspace syncronisation that I disabled in the previous commit. Also some more stats - the VisualFSharp solution now loads in 10 seconds (down from 20). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2909 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADgNO1SIxFSDbiCY0zD_KRo4CECVhoSks5rzh46gaJpZM4NGYh5> .

vasily-kirichenko · 2017-04-25T17:09:49Z

Awesome.

vasily-kirichenko · 2017-04-25T17:29:11Z

FSharp.Compiler project cannot load with this PR:

saul · 2017-04-25T17:35:30Z

@vasily-kirichenko I believe that's because some of the references aren't of a legal form. I'm now calling Path.GetFullPath to normalise the paths (some were like C:\Foo\Bar....\Something... and so were failing the equality check)

vasily-kirichenko · 2017-04-25T17:37:06Z

@saul I tried to load FSharp.Configuration - it loaded insanely fast, but nothing works (coloring, etc.), no errors anywhere.

saul · 2017-04-25T17:56:45Z

@vasily-kirichenko @dsyme I think I've found the cause of why you aren't getting semantic colourisation. (I do get block structure and syntactic). See the reactor log https://gist.github.com/saul/bc9d2ef16d8d9b43e4c44f2d2c211b9b

Search that log for long running tasks. Especially noteworthy:

Reactor: <-- background step, remaining 41, took 324170.6876ms
Reactor: <-- ParseFileInProject C:\Code\visualfsharp\vsintegration\src\FSharp.Editor\LanguageService\LanguageService.fs, remaining 35, took 85086.3554ms
Reactor: <-- ParseAndCheckFileInProject C:\Code\visualfsharp\vsintegration\src\FSharp.Editor\LanguageService\LanguageService.fs, remaining 12, took 61422.0086ms
Reactor: <-- ParseAndCheckFileInProject C:\Code\visualfsharp\vsintegration\src\FSharp.Editor\LanguageService\LanguageService.fs, remaining 3, took 16677.1243ms

I've no idea why these are taking so long... time to break out the profiler? 😦

dsyme · 2017-04-25T18:46:08Z

vsintegration/src/FSharp.ProjectSystem.FSharp/ProjectFlagsService.fs

+        }
+
+        async {
+            while true do


This basically looks very clear. But I'd somehow like to see some safeguards where we try/catch any exceptions in chunks of this code. Infinite loops starting mailbox processors make me nervous if exceptions might happen.

And are we sure the solution events get raised reliably?

Basically I'm wondering "what can go catastrophically wrong that can cause this service to malfunction in really bad ways"

I'm confident that the solution events get raised reliably, yes.

With regards to try/catches, where abouts would you like those? It's worth noting that the project system DoMSBuildSubmission is already very resilient to exceptions and will always call our callback on the UI thread.

dsyme · 2017-04-25T18:47:10Z

vsintegration/src/FSharp.ProjectSystem.FSharp/ProjectFlagsService.fs

+
+            // Keep trying to grab the exclusive lock for a design time build
+            while accessor.BeginDesignTimeBuild() |> ErrorHandler.Failed do
+                do! Async.Sleep 2000


2sec is a long pause - does it need to be so long?

No I'll reduce this to less than a second.

dsyme · 2017-04-25T18:47:46Z

vsintegration/src/FSharp.ProjectSystem.FSharp/ProjectFlagsService.fs

+                            // Having an exclusive lock on design-time builds means the user
+                            // can't initiate any builds themselves. Wait a few seconds before
+                            // trying to grab one again.
+                            do! Async.Sleep 2000


Likewise - why such a comparatively long pause?

It leaves some time so that the user can initiate builds/move files around in the Solution Explorer. When a design-time build is running, quite a few parts of the UI are locked out.

dsyme · 2017-04-25T18:48:28Z

vsintegration/src/FSharp.LanguageService/ProjectSitesAndFiles.fs

@@ -121,26 +127,38 @@ type internal ProjectSitesAndFiles() =
                | _ -> None)
        | None -> Seq.empty

+    static let projectCache = ConcurrentDictionary<string,FSharpProjectOptions>()


What removes entries from this cache?

Nothing yet - good catch

dsyme · 2017-05-08T21:12:44Z

@saul Please take a look at #3002 for a different fix.

If we went with that fix would you want to keep this? It could still be valuable since you mention other bug fixes?

dsyme · 2017-05-08T21:30:53Z

vsintegration/src/FSharp.LanguageService/ProjectSitesAndFiles.fs

-            UnresolvedReferences = None
-            OriginalLoadReferences = []
-            ExtraProjectInfo=extraProjectInfo }   
+    and getProjectOptionsForProjectSite(projectSite:IProjectSite, fileName:string, extraProjectInfo, serviceProvider, cacheUsage:CacheUsage) =


This will clash with my other proposed fix. The project options are already stored in a table in LanguageService.fs, it seems to me we should just callback to there to look up the table.

dsyme · 2017-05-08T21:31:17Z

vsintegration/src/FSharp.ProjectSystem.Base/Project/AssemblyReferenceNode.cs

@@ -76,7 +76,7 @@ public override string Url
        {
            get
            {
-                return this.myAssemblyPath;
+                return Path.GetFullPath(this.myAssemblyPath);


This fix still looks useful?

This was used by the caching code to determine if two references were the same. this.myAssemblyPath is not absolute and so it made comparing them difficult when the rooted-ness was inconsistent within a solution. It's not a bug fix per se.

dsyme · 2017-05-09T10:00:00Z

vsintegration/src/FSharp.ProjectSystem.Base/Project/ProjectReferenceNode.cs

-
-            string targetFrameworkMoniker = (string)otherTargetFrameworkMonikerObj;
+            object otargetFrameworkMoniker = null;
+            int hr = hierarchy.GetProperty(VSConstants.VSITEMID_ROOT, (int)__VSHPROPID4.VSHPROPID_TargetFrameworkMoniker, out otargetFrameworkMoniker);


Is this the correctness fix?

No this stops an occasional assertion on startup

saul · 2017-05-12T18:06:54Z

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

-            | _ -> ()
+
+    member this.TrackDocument(fileName) =
+        match VsRunningDocumentTable.FindDocumentWithoutLocking(package.RunningDocumentTable, fileName) with


@dsyme this file essentially encapsulates the correctness fixes. However with 15.3 now in preview we can move over to the Miscellaneous Files workspace. Read the comments on dotnet/roslyn#17263 and my comment about Miscellaneous Files in this file.

saul · 2017-05-18T18:44:44Z

@dsyme I'll port over the RunningDocumentTable changes that fix #2916 and #1944 to a smaller PR in due course 😄

dsyme · 2017-05-23T17:24:27Z

@dsyme I'll port over the RunningDocumentTable changes that fix #2916 and #1944 to a smaller PR in due course 😄

That would be fantastic, thanks!

dsyme · 2018-05-30T11:29:24Z

@saul This PR is now really old, and I will close it. Can you reopen or resubmit if you want to resurect it? thanks

Initial work into moving ComputeSourcesAndFlags off the UI thread and…

2723cb4

… running asyncronously after load

msftclas added the cla-already-signed label Apr 24, 2017

vasily-kirichenko reviewed Apr 24, 2017

View reviewed changes

majocha reviewed Apr 24, 2017

View reviewed changes

vasily-kirichenko reviewed Apr 24, 2017

View reviewed changes

majocha reviewed Apr 24, 2017

View reviewed changes

saul commented Apr 24, 2017

View reviewed changes

Refactor single-file script loading, fix Roslyn workspace syncronisat…

eedfcea

…ion (add support changing project references) Fixes dotnet#2916, fixes dotnet#1944

dsyme reviewed Apr 25, 2017

View reviewed changes

Implement review comment changes

8a1579f

dsyme mentioned this pull request Apr 29, 2017

All features stop working on any file after solution is reloaded #2943

Closed

dsyme reviewed May 8, 2017

View reviewed changes

saul mentioned this pull request May 8, 2017

use graph of option objects - fix 2793 #3002

Merged

dsyme reviewed May 9, 2017

View reviewed changes

dsyme mentioned this pull request May 10, 2017

Nothing works on ProjectSystem project #2970

Closed

saul commented May 12, 2017

View reviewed changes

msftgits removed the cla-already-signed label Sep 26, 2017

saul mentioned this pull request Feb 2, 2018

Saving a script file under a different name results in loss of colorization #1944

Closed

dsyme closed this May 30, 2018

[WIP] Improve solution load performance #2909

[WIP] Improve solution load performance #2909

Conversation

saul commented Apr 24, 2017 • edited Loading

Remaining work

saul commented Apr 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forki commented Apr 24, 2017

saul commented Apr 24, 2017

majocha Apr 24, 2017 • 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

majocha Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

saul Apr 24, 2017 • 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

forki commented Apr 24, 2017

saul commented Apr 24, 2017

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

forki commented Apr 24, 2017

saul commented Apr 24, 2017

saul commented Apr 25, 2017 • edited Loading

majocha commented Apr 25, 2017

saul commented Apr 25, 2017

saul commented Apr 25, 2017

forki commented Apr 25, 2017 via email

vasily-kirichenko commented Apr 25, 2017

vasily-kirichenko commented Apr 25, 2017

saul commented Apr 25, 2017 • edited Loading

vasily-kirichenko commented Apr 25, 2017

saul commented Apr 25, 2017 • 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

dsyme commented May 8, 2017

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

saul commented May 18, 2017

dsyme commented May 23, 2017

dsyme commented May 30, 2018

saul commented Apr 24, 2017 •

edited

Loading

majocha Apr 24, 2017 •

edited

Loading

majocha Apr 24, 2017 •

edited

Loading

saul Apr 24, 2017 •

edited

Loading

saul commented Apr 25, 2017 •

edited

Loading

saul commented Apr 25, 2017 •

edited

Loading

saul commented Apr 25, 2017 •

edited

Loading