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

Pre-start testhosts #3666

Merged
merged 41 commits into from
Feb 23, 2023
Merged

Pre-start testhosts #3666

merged 41 commits into from
Feb 23, 2023

Conversation

nohwnd
Copy link
Member

@nohwnd nohwnd commented May 23, 2022

Description

Pre-start testhosts for non-parallel runs for the next source in row. So we don't run any tests in parallel but we have a testhost ready to pick up the next source and run it. This makes the testhost startup and adapter lookup amortize better.

Related issue

Fixes part of #3647

@nohwnd
Copy link
Member Author

nohwnd commented May 23, 2022

Not fully finished, but not very complex. And it has no impact on testhost as it is using just the existing messages. There is also no huge gap between the request start and the actual work, as with test sessions. So we don't have to worry about locking files or anything because they would have been locked anyway, because we are in "a run".

Basically we just split up the run task into two parts, DiscoveryInitialize (RunInitialize), and Discovery (Run). And then run them staggered, so we use more threads (people usually have mulitcore cpus these days), but don't run tests in parallel (people usually don't want that). So we get the benefit of always having a testhost ready, without the complications of parallel test execution, even if isolated at process level:

|----- prepare ----|---run---|
       |----- prepare ----|  |---run---|
       |----- prepare ----|            |---run---|
                             |----- prepare ----||---run---|

@cvpoienaru cvpoienaru marked this pull request as ready for review January 10, 2023 18:47
Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

I will do another pass later because I am not sure I grasped the full logic at the first review.

@cvpoienaru
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Codrin Poienaru and others added 10 commits January 11, 2023 14:04
…allelProxyDiscoveryManager.cs

Co-authored-by: Amaury Levé <[email protected]>
…allelProxyDiscoveryManager.cs

Co-authored-by: Amaury Levé <[email protected]>
…allelProxyExecutionManager.cs

Co-authored-by: Amaury Levé <[email protected]>
…allelProxyDiscoveryManager.cs

Co-authored-by: Amaury Levé <[email protected]>
@cvpoienaru
Copy link
Member

Code and tests lgtm but I'd like to have some numbers to show perf gain of the feature on small or big projects.

I'll get back to you with some perf measurements.

Also, fyi, I'd recommend you to avoid using interpolated strings when you don't use variables as it has some unneeded perf cost (e.g. $"my string" should be "my string")

For the interpolated strings, that was my bad, forgot to remove them when transforming the Debug.WriteLine into EqtTrace.Verbose. They should be fixed now.

@nohwnd
Copy link
Member Author

nohwnd commented Jan 19, 2023

I saw notification for mention here, but all I see now are outdated comments, so I guess no more assistance is needed from me right now.

@nohwnd
Copy link
Member Author

nohwnd commented Jan 23, 2023

@cvpoienaru are there any numbers please?

@nohwnd
Copy link
Member Author

nohwnd commented Feb 14, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@cvpoienaru
Copy link
Member

/azp run

1 similar comment
@cvpoienaru
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MarcoRossignoli MarcoRossignoli merged commit 05c0c4c into microsoft:main Feb 23, 2023
@MaceWindu
Copy link

Is it possible to disable this feature somehow? It currently breaks test discovery for us with multiple

Microsoft.VisualStudio.TestPlatform.ObjectModel.TestPlatformException: Could not find testhost
   at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Hosting.DotnetTestHostManager.GetTestHostProcessStartInfo(IEnumerable`1 sources, IDictionary`2 environmentVariables, TestRunnerConnectionInfo connectionInfo)
   at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManager.SetupChannel(IEnumerable`1 sources, String runSettings)
   at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyDiscoveryManager.InitializeDiscovery(DiscoveryCriteria discoveryCriteria, ITestDiscoveryEventsHandler2 eventHandler, Boolean skipDefaultAdapters)
Microsoft.VisualStudio.TestPlatform.ObjectModel.TestPlatformException: Could not find testhost
   at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Hosting.DotnetTestHostManager.GetTestHostProcessStartInfo(IEnumerable`1 sources, IDictionary`2 environmentVariables, TestRunnerConnectionInfo connectionInfo)
   at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManager.SetupChannel(IEnumerable`1 sources, String runSettings)
   at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyDiscoveryManager.InitializeDiscovery(DiscoveryCriteria discoveryCriteria, ITestDiscoveryEventsHandler2 eventHandler, Boolean skipDefaultAdapters)
System.InvalidOperationException: The provided manager was not found in any slot.
   at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ParallelOperationManager`3.ClearCompletedSlot(TManager completedManager)
   at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ParallelOperationManager`3.RunNextWork(TManager completedManager)
   at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.Parallel.ParallelProxyDiscoveryManager.HandlePartialDiscoveryComplete(IProxyDiscoveryManager proxyDiscoveryManager, Int64 totalTests, IEnumerable`1 lastChunk, Boolean isAborted)
   at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.Parallel.ParallelDiscoveryEventsHandler.HandleDiscoveryComplete(DiscoveryCompleteEventArgs discoveryCompleteEventArgs, IEnumerable`1 lastChunk)
   at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyDiscoveryManager.InitializeDiscovery(DiscoveryCriteria discoveryCriteria, ITestDiscoveryEventsHandler2 eventHandler, Boolean skipDefaultAdapters)
   at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyDiscoveryManager.DiscoverTests(DiscoveryCriteria discoveryCriteria, ITestDiscoveryEventsHandler2 eventHandler)

@cvpoienaru
Copy link
Member

@MaceWindu a quick way to disable this feature would be to set VSTEST_HOSTPRESTART_COUNT to 0.

@MaceWindu
Copy link

Looks like it doesn't help with issue. Will wait for #4469 fixed :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants