Improve solution load performance #2133

Merged
merged 7 commits into from Jan 3, 2017

Projects

None yet

6 participants

@saul
Contributor
saul commented Dec 30, 2016 edited

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

  1. ComputeSourcesAndFlags is called twice per project on load
  2. ComputeSourcesAndFlags is called twice per project on solution configuration change
  3. ComputeSourcesAndFlags is unnecessarily called for every MSBuild action (even just determining output groups). This fixes #2046.
  4. 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:
untitled 2
untitled 4

saul added some commits Dec 23, 2016
@saul saul Merge remote-tracking branch 'refs/remotes/Microsoft/master' 4c2e98a
@saul saul Merge branch 'master' of https://github.com/Microsoft/visualfsharp 9b1a4e9
@saul saul Merge remote-tracking branch 'Microsoft/master' b6f757c
@saul saul Reduce number of calls to ComputeSourcesAndFlags, add wait indicator …
…to some long running project tasks
2f2517d
@msftclas

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
Contributor
dungpa commented Dec 30, 2016

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

@vasily-kirichenko
Contributor
vasily-kirichenko commented Dec 30, 2016 edited

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

@saul
Contributor
saul commented Dec 30, 2016

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

@vasily-kirichenko
Contributor

image

image

image

@saul
Contributor
saul commented Dec 30, 2016

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

saul added some commits Dec 31, 2016
@saul saul Project close might throw an exception too, so bundle them both up an…
…d re-raise them as an aggregate exception
ade2a6a
@saul saul Added IVsSolutionBuildManager5 and IVsThreadedWaitDialogFactory to VS…
… mocks to fix tests
70c816c
@vasily-kirichenko
Contributor

@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
+open System.Runtime.InteropServices
+open Microsoft.VisualStudio.Shell.Interop
+
+type WaitDialogOptions =
@KevinRansom
KevinRansom Dec 31, 2016 Contributor

type internal WaitDialogOptions =

+ ShowMarqueeProgress : bool
+ }
+
+module WaitDialog =
@KevinRansom
KevinRansom Dec 31, 2016 Contributor

module internal WaitDialog =

@@ -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 =
@KevinRansom
KevinRansom Dec 31, 2016 Contributor

type internal ActiveCfgBatchUpdateState =

@KevinRansom
Contributor

@dotnet-bot test this please

@saul saul Hide some types/modules that were accidentally added to the public AP…
…I surface
210085c
@saul
Contributor
saul commented Dec 31, 2016

@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
Contributor
forki commented Dec 31, 2016

@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?

- | None -> ()
+ projNode.ComputeSourcesAndFlags()
+
+ if batchState = BatchWaiting then
@KevinRansom
KevinRansom Dec 31, 2016 Contributor

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?

+ // 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 <-
@KevinRansom
KevinRansom Dec 31, 2016 Contributor

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

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

Races?

+
+ interface IVsUpdateSolutionEvents4 with
+ member x.OnActiveProjectCfgChangeBatchBegin() =
+ batchState <- BatchWaiting
@KevinRansom
KevinRansom Dec 31, 2016 Contributor

Ditto

+ member x.OnActiveProjectCfgChangeBatchBegin() =
+ batchState <- BatchWaiting
+ member x.OnActiveProjectCfgChangeBatchEnd() =
+ batchState <- NonBatch
@KevinRansom
KevinRansom Dec 31, 2016 Contributor

Ditto

@saul
Contributor
saul commented Dec 31, 2016

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
Contributor

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
Contributor
saul commented Jan 1, 2017 edited

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
Contributor
KevinRansom commented Jan 2, 2017 edited

@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
Contributor
saul commented Jan 2, 2017

Thanks Kevin.

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

@vasily-kirichenko
Contributor
vasily-kirichenko commented Jan 3, 2017 edited

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
Contributor
forki commented Jan 3, 2017

image

@KevinRansom
Contributor

@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

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

7 checks passed

Ubuntu14.04 Release Build Build finished.
Details
Windows_NT Debug Build Build finished.
Details
Windows_NT Release_ci_part1 Build Build finished.
Details
Windows_NT Release_ci_part2 Build Build finished.
Details
Windows_NT Release_ci_part3 Build Build finished.
Details
Windows_NT Release_net40_no_vs Build Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment