Improve solution load performance #2133

saul · 2016-12-30T14:21:37Z

This PR fixes a few issues that I uncovered investigating #2107:

ComputeSourcesAndFlags is called twice per project on load
ComputeSourcesAndFlags is called twice per project on solution configuration change
ComputeSourcesAndFlags is unnecessarily called for every MSBuild action (even just determining output groups). This fixes Huge pause with high CPU load before VFT solution is started building #2046.
No wait indicator is shown when these long running tasks are freezing the UI thread

The net effect of this PR is that the following actions should have a better user experience (faster and or show wait dialogs):

"Preparing Solution..." stage of loading a solution (twice as fast)
Changing solution configuration (twice as fast)
Renaming/moving files/folders within Solution Explorer
Any action that invokes a non-compile MSBuild target

There's still more to do with #2107, in particular making ComputeSourcesAndFlags faster itself. However I'll save these for a later PR.

With regards to #4, here are what the wait dialogs look like:

…to some long running project tasks

msftclas · 2016-12-30T14:21:42Z

Hi @saul, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

dungpa · 2016-12-30T15:36:20Z

@saul What's a great PR. Thanks for doing this.

vasily-kirichenko · 2016-12-30T16:03:11Z

Yes, this it great for large solutions. A lot of tests fail though :(

saul · 2016-12-30T18:28:50Z

Thanks @vasily-kirichenko - where can I find these test failures?

vasily-kirichenko · 2016-12-30T18:34:57Z

saul · 2016-12-30T18:36:50Z

Thanks again. These look relevant - I'll get on to sorting them.

…d re-raise them as an aggregate exception

… mocks to fix tests

vasily-kirichenko · 2016-12-31T07:39:31Z

@saul BTW, they won't merge the PR until you sign the CLA.

KevinRansom

@saul
Hi, we have verification to ensure that no unintended types are made public. We have quite a high compatability bar, and internal types don't count :-)
So you added a few extra public types, just make them internal and all will be well.

to repro the failure use:

build vs vstest

KevinRansom · 2016-12-31T08:18:39Z

vsintegration/src/FSharp.ProjectSystem.FSharp/WaitDialog.fs

+open System.Runtime.InteropServices
+open Microsoft.VisualStudio.Shell.Interop
+
+type WaitDialogOptions =


type internal WaitDialogOptions =

KevinRansom · 2016-12-31T08:18:56Z

vsintegration/src/FSharp.ProjectSystem.FSharp/WaitDialog.fs

+        ShowMarqueeProgress : bool
+    }
+
+module WaitDialog =


module internal WaitDialog =

KevinRansom · 2016-12-31T08:19:25Z

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

@@ -1595,6 +1626,11 @@ namespace rec Microsoft.VisualStudio.FSharp.ProjectSystem
                member x.SetSpecificEditorProperty(_mkDocument:string, _propid:int, _value:obj ) =
                    VSConstants.E_NOTIMPL
            end
+
+    type ActiveCfgBatchUpdateState =


type internal ActiveCfgBatchUpdateState =

KevinRansom · 2016-12-31T08:29:01Z

@dotnet-bot test this please

…I surface

saul · 2016-12-31T09:27:07Z

@vasily-kirichenko it's signed, see #2135. I'm not sure why it's not updated the label on this PR.

@KevinRansom I've made those types internal as requested - all of the tests should pass now.

forki · 2016-12-31T09:37:57Z

@martinwoodward IIRC you are the one to ask when we have trouble with the CLA bots. Could you please take a look? Thanks

KevinRansom

Do we need to address threading issues here?

KevinRansom · 2016-12-31T19:36:14Z

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

-                    | None -> ()
+                    projNode.ComputeSourcesAndFlags()
+
+                    if batchState = BatchWaiting then


This does not look threadsafe.

Is there some mechanism in place that ensures this can never race, or should we use
Interlocked Compex or a lock?

KevinRansom · 2016-12-31T19:36:30Z

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

+
+            interface IVsUpdateSolutionEvents4 with
+                member x.OnActiveProjectCfgChangeBatchBegin() =
+                    batchState <- BatchWaiting


KevinRansom · 2016-12-31T19:36:39Z

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

+                member x.OnActiveProjectCfgChangeBatchBegin() =
+                    batchState <- BatchWaiting
+                member x.OnActiveProjectCfgChangeBatchEnd() =
+                    batchState <- NonBatch


KevinRansom · 2016-12-31T19:41:32Z

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

+                    // this will be called for each project, but wait dialogs cannot 'stack'
+                    // i.e. if a wait dialog is already open, subsequent calls to StartWaitDialog
+                    // will not override the current open dialog
+                    waitDialog <-


Is there a guarantee that OnBeforeActiveSolutionCfgChange can be called multiiple times simultaneously?

KevinRansom · 2016-12-31T19:42:11Z

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

                member x.OnAfterActiveSolutionCfgChange(_oldCfg, _newCfg) =
-                    match queuedWork with
-                    | Some(l) -> l |> List.iter (fun projNode -> projNode.ComputeSourcesAndFlags())
+                    match waitDialog with


saul · 2016-12-31T23:52:49Z

The code was previously thread unsafe as it appended to a list on each OnActiveProjectCfgChange (queuedWork). I don't believe there's any threading guarantees from VS, but from what I've experienced it's always called on the UI thread.

I'm happy to put locks on this but I don't think they're necessary.

KevinRansom · 2017-01-01T01:28:22Z

Given the more pervasive use of async It seems as if there may be a justification to improving the thread safety of the code that runs in VS. That it wasn't threadsafe before is probably a poor guide to whether it should be.

saul · 2017-01-01T15:27:58Z

I completely understand your concerns, but although it's not explicitly mentioned in the docs, I don't believe these events can ever be called from non-UI threads. Looking at other implementations of IVsUpdateSolutionEvents in Roslyn, they also exhibit no thread safety.

Would you be happy with calls to UIThread.MustBeCalledFromUIThread() where you're concerned about race conditions? It's probably useful to note that in the current implementation ComputeSourcesAndFlags already calls MustBeCalledFromUIThread (via a call to InvokeMSBuild which then calls DoMSBuildSubmission).

KevinRansom · 2017-01-02T01:55:58Z

@saul point me the roslyn code, I will discus it with Srivatsn, see what the deal should be with that.

@srivatsn, what is the threadsafety requirements for:

IVsUpdateSolutionEvents4 implementations?
Does VS guarantee they are called on a single thread, or that concurrency is not possible?

Thanks

Kevin

saul · 2017-01-02T10:49:00Z

Thanks Kevin.

Here's a couple of references I found in Roslyn:

dotnet/roslyn-project-system ConfigurationState.vb - calls UpdateEntriesAndNotifyPages which calls ChangeSelection which modifies member variables
dotnet/roslyn AnalyzersCommandHandler.cs - modifies member variables on callback

vasily-kirichenko · 2017-01-03T09:29:04Z

Switching between Debug and Release configurations on VisualFSharp solution: RC2 - 40 seconds (yes), this PR - 5 seconds. Absolutely awesome!

The huge pause before the building even starts after pressing Ctrl+F5 has completely gone, now the exp instance appears in about 10 seconds. Unbelievable. Great, great job!

forki · 2017-01-03T09:33:09Z

KevinRansom · 2017-01-03T21:16:52Z

@saul .... chatting with a few folk around the team including @srivatsn and @Pilchie we are confident that VS ensures that these events are handled on the UI thread. With the caveat "Until we go to CPS and then we will have to figure this stuff out again".

So ....

thanks for taking care of this

Kevin

saul added 4 commits December 23, 2016 11:34

Merge remote-tracking branch 'refs/remotes/Microsoft/master'

4c2e98a

Merge branch 'master' of https://github.com/Microsoft/visualfsharp

9b1a4e9

Merge remote-tracking branch 'Microsoft/master'

b6f757c

Reduce number of calls to ComputeSourcesAndFlags, add wait indicator …

2f2517d

…to some long running project tasks

msftclas added the cla-required label Dec 30, 2016

saul added 2 commits December 31, 2016 00:09

Project close might throw an exception too, so bundle them both up an…

ade2a6a

…d re-raise them as an aggregate exception

Added IVsSolutionBuildManager5 and IVsThreadedWaitDialogFactory to VS…

70c816c

… mocks to fix tests

KevinRansom requested changes Dec 31, 2016

View reviewed changes

Hide some types/modules that were accidentally added to the public AP…

210085c

…I surface

KevinRansom added cla-already-signed and removed cla-required labels Dec 31, 2016

KevinRansom requested changes Dec 31, 2016

View reviewed changes

vasily-kirichenko mentioned this pull request Jan 3, 2017

Huge pause with high CPU load before VFT solution is started building #2046

Closed

KevinRansom approved these changes Jan 3, 2017

View reviewed changes

KevinRansom merged commit 4cf43f3 into dotnet:master Jan 3, 2017

saul mentioned this pull request Jan 6, 2017

MsBuild causing blocking on UI thread in a variety of situations #1884

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve solution load performance #2133

Improve solution load performance #2133

saul commented Dec 30, 2016 •

edited

Loading

msftclas commented Dec 30, 2016

dungpa commented Dec 30, 2016

vasily-kirichenko commented Dec 30, 2016 •

edited

Loading

saul commented Dec 30, 2016

vasily-kirichenko commented Dec 30, 2016

saul commented Dec 30, 2016

vasily-kirichenko commented Dec 31, 2016

KevinRansom left a comment

KevinRansom Dec 31, 2016

KevinRansom Dec 31, 2016

KevinRansom Dec 31, 2016

KevinRansom commented Dec 31, 2016

saul commented Dec 31, 2016

forki commented Dec 31, 2016

KevinRansom left a comment

KevinRansom Dec 31, 2016

KevinRansom Dec 31, 2016

KevinRansom Dec 31, 2016

KevinRansom Dec 31, 2016

KevinRansom Dec 31, 2016

saul commented Dec 31, 2016

KevinRansom commented Jan 1, 2017

saul commented Jan 1, 2017 •

edited

Loading

KevinRansom commented Jan 2, 2017 •

edited

Loading

saul commented Jan 2, 2017

vasily-kirichenko commented Jan 3, 2017 •

edited

Loading

forki commented Jan 3, 2017

KevinRansom commented Jan 3, 2017

Improve solution load performance #2133

Improve solution load performance #2133

Conversation

saul commented Dec 30, 2016 • edited Loading

msftclas commented Dec 30, 2016

dungpa commented Dec 30, 2016

vasily-kirichenko commented Dec 30, 2016 • edited Loading

saul commented Dec 30, 2016

vasily-kirichenko commented Dec 30, 2016

saul commented Dec 30, 2016

vasily-kirichenko commented Dec 31, 2016

KevinRansom 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

KevinRansom commented Dec 31, 2016

saul commented Dec 31, 2016

forki commented Dec 31, 2016

KevinRansom 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

saul commented Dec 31, 2016

KevinRansom commented Jan 1, 2017

saul commented Jan 1, 2017 • edited Loading

KevinRansom commented Jan 2, 2017 • edited Loading

saul commented Jan 2, 2017

vasily-kirichenko commented Jan 3, 2017 • edited Loading

forki commented Jan 3, 2017

KevinRansom commented Jan 3, 2017

saul commented Dec 30, 2016 •

edited

Loading

vasily-kirichenko commented Dec 30, 2016 •

edited

Loading

saul commented Jan 1, 2017 •

edited

Loading

KevinRansom commented Jan 2, 2017 •

edited

Loading

vasily-kirichenko commented Jan 3, 2017 •

edited

Loading