Skip to content

Commit

Permalink
Backport recent fixes (#4632)
Browse files Browse the repository at this point in the history 
Co-authored-by: Amaury Levé <amauryleve@microsoft.com>
Fix timing issue in parallel execution (#4629)
  • Loading branch information
@nohwnd
nohwnd committed Aug 2, 2023
1 parent f6bfa29 commit 52b57f0
Show file tree
Hide file tree
Showing 11 changed files with 154 additions and 67 deletions.
3 changes: 1 addition & 2 deletions eng/Versions.props
View file
Expand Up @@ -8,7 +8,7 @@
is trying to parse that version and will consider any version with more than 4 `.` in it as invalid.
-->
<SemanticVersioningV1>true</SemanticVersioningV1>
<VersionPrefix>17.7.0</VersionPrefix>
<VersionPrefix>17.7.1</VersionPrefix>
<PreReleaseVersionLabel>release</PreReleaseVersionLabel>
</PropertyGroup>
<PropertyGroup Label="Arcade settings">
Expand Down Expand Up @@ -36,7 +36,6 @@
<MicrosoftVisualStudioEnterpriseAspNetHelper>$(MicrosoftVisualStudioDiagnosticsUtilitiesVersion)</MicrosoftVisualStudioEnterpriseAspNetHelper>
<MicrosoftVisualStudioInteropVersion>17.3.32622.426</MicrosoftVisualStudioInteropVersion>
<MicrosoftVSSDKBuildToolsVersion>17.4.2116</MicrosoftVSSDKBuildToolsVersion>
<NETStandardLibraryVersion>2.0.3</NETStandardLibraryVersion>
<NewtonsoftJsonVersion>13.0.1</NewtonsoftJsonVersion>
<RoslynBannedApiAnalyzersVersion>3.3.3</RoslynBannedApiAnalyzersVersion>
<RoslynPublicApiAnalyzersVersion>3.3.4-beta1.21554.2</RoslynPublicApiAnalyzersVersion>
Expand Down
16 changes: 8 additions & 8 deletions eng/verify-nupkgs.ps1
View file
Expand Up @@ -18,7 +18,7 @@ function Verify-Nuget-Packages {
"Microsoft.NET.Test.Sdk" = 16;
"Microsoft.TestPlatform" = 608;
"Microsoft.TestPlatform.Build" = 21;
"Microsoft.TestPlatform.CLI" = 493;
"Microsoft.TestPlatform.CLI" = 472;
"Microsoft.TestPlatform.Extensions.TrxLogger" = 35;
"Microsoft.TestPlatform.ObjectModel" = 93;
"Microsoft.TestPlatform.AdapterUtilities" = 34;
Expand All @@ -34,26 +34,26 @@ function Verify-Nuget-Packages {
$pattern = "*.$versionPrefix*.nupkg"
$nugetPackages = @(Get-ChildItem $packageDirectory -Filter $pattern -Recurse -File | Where-Object { $_.Name -notLike "*.symbols.nupkg"})

if (0 -eq $nugetPackages.Length) {
if (0 -eq $nugetPackages.Length) {
throw "No nuget packages matching $pattern were found in '$packageDirectory'."
}

$suffixes = @($nugetPackages -replace ".*?$([regex]::Escape($versionPrefix))(.*)\.nupkg", '$1' | Sort-Object -Unique)
if (1 -lt $suffixes.Length) {
if (1 -lt $suffixes.Length) {
Write-Host "There are two different suffixes matching the same version prefix: '$($suffixes -join "', '")'".

$latestNuget = $nugetPackages |
Where-Object { $_.Name -like "Microsoft.TestPlatform.ObjectModel.*" } |
Sort-Object -Property LastWriteTime -Descending |
$latestNuget = $nugetPackages |
Where-Object { $_.Name -like "Microsoft.TestPlatform.ObjectModel.*" } |
Sort-Object -Property LastWriteTime -Descending |
Select-Object -First 1

$suffix = $suffixes | Where { $latestNuget.Name.Contains("$versionPrefix$_.nupkg") }
$version = "$versionPrefix$suffix"
Write-Host "The most recently written Microsoft.TestPlatform.ObjectModel.* nuget, is $($latestNuget.Name), which has '$suffix' suffix. Selecting only packages with that suffix."

$nugetPackages = $nugetPackages | Where-Object { $_.Name -like "*$version.nupkg" }
}
else {
else {
$suffix = $suffixes[0]
$version = "$versionPrefix$suffix"
}
Expand Down
7 changes: 7 additions & 0 deletions src/DataCollectors/DumpMinitool.arm64/DumpMinitool.arm64.csproj
View file
Expand Up @@ -10,6 +10,13 @@
<RuntimeIdentifier Condition=" '$(DotNetBuildFromSource)' != 'true' ">win10-arm64</RuntimeIdentifier>
<AutoGenerateBindingRedirects>false</AutoGenerateBindingRedirects>
<ExcludeFromSourceBuild>true</ExcludeFromSourceBuild>
<!--
NETSDK1201: For projects targeting .NET 8.0 and higher, specifying a RuntimeIdentifier will no longer produce a
self contained app by default. To continue building self-contained apps, set the SelfContained property to true
or use the -\-self-contained argument.
-->
<MSBuildWarningsAsMessages>NETSDK1201</MSBuildWarningsAsMessages>
<NoWarn>$(NoWarn);NETSDK1201</NoWarn>
</PropertyGroup>

<ItemGroup>
Expand Down
7 changes: 7 additions & 0 deletions src/DataCollectors/DumpMinitool.x86/DumpMinitool.x86.csproj
View file
Expand Up @@ -10,6 +10,13 @@
<RuntimeIdentifier Condition=" '$(DotNetBuildFromSource)' != 'true' ">win7-x86</RuntimeIdentifier>
<AutoGenerateBindingRedirects>false</AutoGenerateBindingRedirects>
<ExcludeFromSourceBuild>true</ExcludeFromSourceBuild>
<!--
NETSDK1201: For projects targeting .NET 8.0 and higher, specifying a RuntimeIdentifier will no longer produce a
self contained app by default. To continue building self-contained apps, set the SelfContained property to true
or use the -\-self-contained argument.
-->
<MSBuildWarningsAsMessages>NETSDK1201</MSBuildWarningsAsMessages>
<NoWarn>$(NoWarn);NETSDK1201</NoWarn>
</PropertyGroup>

<ItemGroup>
Expand Down
7 changes: 7 additions & 0 deletions src/DataCollectors/DumpMinitool/DumpMinitool.csproj
View file
Expand Up @@ -10,6 +10,13 @@
<RuntimeIdentifier Condition=" '$(DotNetBuildFromSource)' != 'true' ">win7-x64</RuntimeIdentifier>
<AutoGenerateBindingRedirects>false</AutoGenerateBindingRedirects>
<ExcludeFromSourceBuild>true</ExcludeFromSourceBuild>
<!--
NETSDK1201: For projects targeting .NET 8.0 and higher, specifying a RuntimeIdentifier will no longer produce a
self contained app by default. To continue building self-contained apps, set the SelfContained property to true
or use the -\-self-contained argument.
-->
<MSBuildWarningsAsMessages>NETSDK1201</MSBuildWarningsAsMessages>
<NoWarn>$(NoWarn);NETSDK1201</NoWarn>
</PropertyGroup>

<ItemGroup>
Expand Down
57 changes: 51 additions & 6 deletions src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelOperationManager.cs
View file
Expand Up @@ -9,6 +9,7 @@

using Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.Parallel;
using Microsoft.VisualStudio.TestPlatform.ObjectModel;
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Client;

namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client;

Expand Down Expand Up @@ -63,6 +64,8 @@ public ParallelOperationManager(Func<TestRuntimeProviderInfo, TWorkload, TManage

private void ClearSlots(bool acceptMoreWork)
{
EqtTrace.Verbose($"ParallelOperationManager.ClearSlots: Clearing all slots. Slots should accept more work: {acceptMoreWork}");

lock (_lock)
{
_acceptMoreWork = acceptMoreWork;
Expand All @@ -76,6 +79,13 @@ private void SetOccupiedSlotCount()
{
AvailableSlotCount = _managerSlots.Count(s => !s.HasWork);
OccupiedSlotCount = _managerSlots.Count - AvailableSlotCount;

if (EqtTrace.IsVerboseEnabled)
{
EqtTrace.Verbose($"ParallelOperationManager.SetOccupiedSlotCount: Setting slot counts AvailableSlotCount = {AvailableSlotCount}, OccupiedSlotCount = {OccupiedSlotCount}.");
EqtTrace.Verbose($"Occupied slots:\n{(string.Join("\n", _managerSlots.Where(s => s.HasWork).Select((slot) => $"{slot.Index}: {GetSourcesForSlotExpensive(slot)}").ToArray()))}");

}
}

public void StartWork(
Expand All @@ -91,6 +101,7 @@ private void SetOccupiedSlotCount()
_initializeWorkload = initializeWorkload ?? throw new ArgumentNullException(nameof(initializeWorkload));
_runWorkload = runWorkload ?? throw new ArgumentNullException(nameof(runWorkload));

EqtTrace.Verbose($"ParallelOperationManager.StartWork: Starting adding {workloads.Count} workloads.");
_workloads.AddRange(workloads);

ClearSlots(acceptMoreWork: true);
Expand Down Expand Up @@ -123,7 +134,10 @@ private bool RunWorkInParallel()
// so when it is allowed to enter it will try to add more work, but we already cancelled,
// so we should not start more work.
if (!_acceptMoreWork)
{
EqtTrace.Verbose($"ParallelOperationManager.RunWorkInParallel: We don't accept more work, returning false.");
return false;
}

// We grab all empty slots.
var availableSlots = _managerSlots.Where(slot => !slot.HasWork).ToImmutableArray();
Expand All @@ -136,11 +150,10 @@ private bool RunWorkInParallel()
var workloadsToAdd = availableWorkloads.Take(amount).ToImmutableArray();

// We associate each workload to a slot, if we reached the max parallel
// level, then we will run only initalize step of the given workload.
// level, then we will run only initialize step of the given workload.
for (int i = 0; i < amount; i++)
{
var slot = availableSlots[i];
slot.HasWork = true;
var workload = workloadsToAdd[i];
slot.ShouldPreStart = occupiedSlots + i + 1 > MaxParallelLevel;

Expand All @@ -152,6 +165,13 @@ private bool RunWorkInParallel()
slot.Work = workload.Work;

_workloads.Remove(workload);

EqtTrace.Verbose($"ParallelOperationManager.RunWorkInParallel: Adding 1 workload to slot, remaining workloads {_workloads.Count}.");

// This must be set last, every loop below looks at this property,
// and they can do so from a different thread. So if we mark it as HasWork before actually assigning the work
// we can pick up the slot, but it has no associated work yet.
slot.HasWork = true;
}

slots = _managerSlots.ToArray();
Expand All @@ -172,12 +192,16 @@ private bool RunWorkInParallel()
{
startedWork++;
slot.IsRunning = true;
EqtTrace.Verbose($"ParallelOperationManager.RunWorkInParallel: Running on pre-started host: {(DateTime.Now.TimeOfDay - slot.PreStartTime).TotalMilliseconds}ms {slot.InitTask?.Status}");
if (EqtTrace.IsVerboseEnabled)
{
EqtTrace.Verbose($"ParallelOperationManager.RunWorkInParallel: Running on pre-started host for work (source) {GetSourcesForSlotExpensive(slot)}: {(DateTime.Now.TimeOfDay - slot.PreStartTime).TotalMilliseconds}ms {slot.InitTask?.Status}");
}
_runWorkload(slot.Manager!, slot.EventHandler!, slot.Work!, slot.IsPreStarted, slot.InitTask);

// We already started as many as we were allowed, jump out;
if (startedWork == MaxParallelLevel)
{
EqtTrace.Verbose($"ParallelOperationManager.RunWorkInParallel: We started {startedWork} work items, which is the max parallel level. Won't start more work.");
break;
}
}
Expand All @@ -194,14 +218,18 @@ private bool RunWorkInParallel()
{
startedWork++;
slot.IsRunning = true;
EqtTrace.Verbose("ParallelOperationManager.RunWorkInParallel: Started work on a host.");
if (EqtTrace.IsVerboseEnabled)
{
EqtTrace.Verbose($"ParallelOperationManager.RunWorkInParallel: Started host in slot number {slot.Index} for work (source): {GetSourcesForSlotExpensive(slot)}.");
}
_runWorkload(slot.Manager!, slot.EventHandler!, slot.Work!, slot.IsPreStarted, slot.InitTask);
}
}

// We already started as many as we were allowed, jump out;
if (startedWork == MaxParallelLevel)
{
EqtTrace.Verbose($"ParallelOperationManager.RunWorkInParallel: We started {startedWork} work items, which is the max parallel level. Won't start more work.");
break;
}
}
Expand All @@ -215,14 +243,19 @@ private bool RunWorkInParallel()
preStartedWork++;
slot.PreStartTime = DateTime.Now.TimeOfDay;
slot.IsPreStarted = true;
EqtTrace.Verbose("ParallelOperationManager.RunWorkInParallel: Pre-starting a host.");
if (EqtTrace.IsVerboseEnabled)
{
EqtTrace.Verbose($"ParallelOperationManager.RunWorkInParallel: Pre-starting a host for work (source): {GetSourcesForSlotExpensive(slot)}.");
}
slot.InitTask = _initializeWorkload!(slot.Manager!, slot.EventHandler!, slot.Work!);
}
}

// Return true when we started more work. Or false, when there was nothing more to do.
// This will propagate to handling of partial discovery or partial run.
return preStartedWork + startedWork > 0;
var weAddedMoreWork = preStartedWork + startedWork > 0;
EqtTrace.Verbose($"ParallelOperationManager.RunWorkInParallel: We started {preStartedWork + startedWork} work items in here, returning {weAddedMoreWork}.");
return weAddedMoreWork;
}

public bool RunNextWork(TManager completedManager)
Expand Down Expand Up @@ -258,6 +291,10 @@ private void ClearCompletedSlot(TManager completedManager)
throw new InvalidOperationException("The provided manager was found in multiple slots.");
}

if (EqtTrace.IsVerboseEnabled)
{
EqtTrace.Verbose($"ParallelOperationManager.ClearCompletedSlot: Clearing slot number {completedSlot[0].Index} with work (source): {GetSourcesForSlotExpensive(completedSlot[0])}.");
}
var slot = completedSlot[0];
slot.PreStartTime = TimeSpan.Zero;
slot.Work = default(TWorkload);
Expand All @@ -273,8 +310,14 @@ private void ClearCompletedSlot(TManager completedManager)
}
}

private static string GetSourcesForSlotExpensive(ParallelOperationManager<TManager, TEventHandler, TWorkload>.Slot slot)
{
return string.Join(", ", (slot.Work as DiscoveryCriteria)?.Sources ?? (slot.Work as TestRunCriteria)?.Sources ?? Array.Empty<string>());
}

public void DoActionOnAllManagers(Action<TManager> action, bool doActionsInParallel = false)
{
EqtTrace.Verbose($"ParallelOperationManager.DoActionOnAllManagers: Calling an action on all managers.");
// We don't need to lock here, we just grab the current list of
// slots that are occupied (have managers) and run action on each one of them.
var managers = _managerSlots.Where(slot => slot.HasWork).Select(slot => slot.Manager).ToImmutableArray();
Expand Down Expand Up @@ -320,11 +363,13 @@ private static void DoManagerAction(Action action)

internal void StopAllManagers()
{
EqtTrace.Verbose($"ParallelOperationManager.StopAllManagers: Stopping all managers.");
ClearSlots(acceptMoreWork: false);
}

public void Dispose()
{
EqtTrace.Verbose($"ParallelOperationManager.Dispose: Disposing all managers.");
ClearSlots(acceptMoreWork: false);
}

Expand Down
6 changes: 6 additions & 0 deletions src/Microsoft.TestPlatform.CrossPlatEngine/Client/Parallel/ParallelProxyDiscoveryManager.cs
View file
Expand Up @@ -287,6 +287,12 @@ private Task InitializeDiscoverTestsOnConcurrentManager(IProxyDiscoveryManager p
bool initialized,
Task? task)
{
// If we do the scheduling incorrectly this will get null. It should not happen, but it has happened before.
if (discoveryCriteria == null)
{
throw new ArgumentNullException(nameof(discoveryCriteria));
}

// Kick off another discovery task for the next source
Task.Run(() =>
{
Expand Down

0 comments on commit 52b57f0

Please sign in to comment.