Don't use ArtifactsPath for wasm and monoaotllvm.

MonoPublisher inherits from DotNetCliBuilderBase. Don't repeat sequential builds.
dotnet · Apr 17, 2024 · 236f5eb · 236f5eb
1 parent 44a79b0
commit 236f5eb
Show file tree

Hide file tree

Showing 8 changed files with 70 additions and 54 deletions.
diff --git a/src/BenchmarkDotNet/Running/BenchmarkRunnerClean.cs b/src/BenchmarkDotNet/Running/BenchmarkRunnerClean.cs
@@ -373,28 +373,23 @@ private static ImmutableArray<ValidationError> Validate(params BenchmarkRunInfo[
 
         private static Dictionary<BuildPartition, BuildResult> BuildInParallel(ILogger logger, string rootArtifactsFolderPath, BuildPartition[] buildPartitions, in StartedClock globalChronometer, EventProcessor eventProcessor)
         {
-            var parallelPartitions = new List<BuildPartition[]>();
-            var sequentialPartitions = new List<BuildPartition>();
+            var parallelPartitions = new List<(BuildPartition buildPartition, IToolchain toolchain, bool isSequential)[]>();
+            var sequentialPartitions = new List<(BuildPartition, IToolchain)>();
             foreach (var partition in buildPartitions)
             {
-                // If dotnet sdk does not support ArtifactsPath, it's unsafe to build the same project in parallel.
-                // We cannot rely on falling back to sequential after failure, because the builds can be corrupted.
-                // Therefore we have to build them sequentially. #2425
-                if (partition.RepresentativeBenchmarkCase.GetToolchain().Builder is DotNetCliBuilderBase dotnetBuilder
-                    && !DotNetCliCommandExecutor.DotNetSdkSupportsArtifactsPath(dotnetBuilder.CustomDotNetCliPath))
-                {
-                    sequentialPartitions.Add(partition);
-                }
+                // It's guaranteed that all the benchmarks in single partition have same toolchain.
+                var toolchain = partition.RepresentativeBenchmarkCase.GetToolchain();
+                // #2425
+                if (toolchain.IsSafeToBuildInParallel())
+                    parallelPartitions.Add([(partition, toolchain, false)]);
                 else
-                {
-                    parallelPartitions.Add([partition]);
-                }
+                    sequentialPartitions.Add((partition, toolchain));
             }
             if (sequentialPartitions.Count > 0)
             {
                 // We build the sequential partitions in parallel with the other partitions to complete as fast as possible.
                 // Place them first to make sure they are started first.
-                parallelPartitions.Insert(0, [.. sequentialPartitions]);
+                parallelPartitions.Insert(0, sequentialPartitions.Select(x => (x.Item1, x.Item2, true)).ToArray());
             }
 
             logger.WriteLineHeader($"// ***** Building {parallelPartitions.Count} exe(s) in Parallel{(sequentialPartitions.Count <= 1 ? "" : $", {sequentialPartitions.Count} sequentially")}   *****");
@@ -403,37 +398,43 @@ private static ImmutableArray<ValidationError> Validate(params BenchmarkRunInfo[
 
             var beforeParallelBuild = globalChronometer.GetElapsed();
 
-            var buildResults = parallelPartitions
+            var parallelBuildResults = parallelPartitions
                 .AsParallel()
                 .SelectMany(partitions => partitions
-                    .Select(buildPartition => (Partition: buildPartition, Result: Build(buildPartition, rootArtifactsFolderPath, buildLogger))))
+                    .Select(tuple => (Partition: tuple.buildPartition,
+                        Result: Build(tuple.buildPartition, tuple.toolchain, rootArtifactsFolderPath, buildLogger),
+                        WasSequential: tuple.isSequential)
+                    )
+                )
                 .AsSequential() // Ensure that build completion events are processed sequentially
                 .Select(build =>
                 {
                     // If the generation was successful, but the build was not, we will try building sequentially
                     // so don't send the OnBuildComplete event yet.
-                    if (buildPartitions.Length <= 1 || !build.Result.IsGenerateSuccess || build.Result.IsBuildSuccess)
+                    if (build.WasSequential || buildPartitions.Length <= 1 || !build.Result.IsGenerateSuccess || build.Result.IsBuildSuccess)
                         eventProcessor.OnBuildComplete(build.Partition, build.Result);
 
                     return build;
                 })
-                .ToDictionary(build => build.Partition, build => build.Result);
+                .ToArray();
 
             var afterParallelBuild = globalChronometer.GetElapsed();
 
             logger.WriteLineHeader($"// ***** Done, took {GetFormattedDifference(beforeParallelBuild, afterParallelBuild)}   *****");
 
-            if (buildPartitions.Length <= 1 || !buildResults.Values.Any(result => result.IsGenerateSuccess && !result.IsBuildSuccess))
+            var buildResults = parallelBuildResults.ToDictionary(build => build.Partition, build => build.Result);
+
+            if (parallelPartitions.Count <= 1 || !parallelBuildResults.Any(build => !build.WasSequential && build.Result.IsGenerateSuccess && !build.Result.IsBuildSuccess))
                 return buildResults;
 
             logger.WriteLineHeader("// ***** Failed to build in Parallel, switching to sequential build   *****");
 
-            foreach (var buildPartition in buildPartitions)
+            foreach (var (buildPartition, toolchain, _) in parallelPartitions.Skip(1).Select(arr => arr[0]))
             {
                 if (buildResults[buildPartition].IsGenerateSuccess && !buildResults[buildPartition].IsBuildSuccess)
                 {
                     if (!buildResults[buildPartition].TryToExplainFailureReason(out string _))
-                        buildResults[buildPartition] = Build(buildPartition, rootArtifactsFolderPath, buildLogger);
+                        buildResults[buildPartition] = Build(buildPartition, toolchain, rootArtifactsFolderPath, buildLogger);
 
                     eventProcessor.OnBuildComplete(buildPartition, buildResults[buildPartition]);
                 }
@@ -449,10 +450,8 @@ static string GetFormattedDifference(ClockSpan before, ClockSpan after)
                 => (after.GetTimeSpan() - before.GetTimeSpan()).ToFormattedTotalTime(DefaultCultureInfo.Instance);
         }
 
-        private static BuildResult Build(BuildPartition buildPartition, string rootArtifactsFolderPath, ILogger buildLogger)
+        private static BuildResult Build(BuildPartition buildPartition, IToolchain toolchain, string rootArtifactsFolderPath, ILogger buildLogger)
         {
-            var toolchain = buildPartition.RepresentativeBenchmarkCase.GetToolchain(); // it's guaranteed that all the benchmarks in single partition have same toolchain
-
             var generateResult = toolchain.Generator.GenerateProject(buildPartition, buildLogger, rootArtifactsFolderPath);
 
             try

diff --git a/src/BenchmarkDotNet/Toolchains/DotNetCli/DotNetCliBuilder.cs b/src/BenchmarkDotNet/Toolchains/DotNetCli/DotNetCliBuilder.cs
@@ -10,6 +10,7 @@ namespace BenchmarkDotNet.Toolchains.DotNetCli
     public abstract class DotNetCliBuilderBase : IBuilder
     {
         public string? CustomDotNetCliPath { get; protected set; }
+        internal bool UseArtifactsPathIfSupported { get; private protected set; }
         public abstract BuildResult Build(GenerateResult generateResult, BuildPartition buildPartition, ILogger logger);
     }
 
@@ -25,6 +26,13 @@ public DotNetCliBuilder(string targetFrameworkMoniker, string? customDotNetCliPa
             LogOutput = logOutput;
         }
 
+        internal DotNetCliBuilder(string? customDotNetCliPath, bool logOutput, bool useArtifactsPathIfSupported)
+        {
+            CustomDotNetCliPath = customDotNetCliPath;
+            LogOutput = logOutput;
+            UseArtifactsPathIfSupported = useArtifactsPathIfSupported;
+        }
+
         public override BuildResult Build(GenerateResult generateResult, BuildPartition buildPartition, ILogger logger)
         {
             BuildResult buildResult = new DotNetCliCommand(
@@ -36,7 +44,7 @@ public override BuildResult Build(GenerateResult generateResult, BuildPartition
                     Array.Empty<EnvironmentVariable>(),
                     buildPartition.Timeout,
                     logOutput: LogOutput)
-                .RestoreThenBuild();
+                .RestoreThenBuild(UseArtifactsPathIfSupported);
             if (buildResult.IsBuildSuccess &&
                 buildPartition.RepresentativeBenchmarkCase.Job.Environment.LargeAddressAware)
             {

diff --git a/src/BenchmarkDotNet/Toolchains/DotNetCli/DotNetCliCommand.cs b/src/BenchmarkDotNet/Toolchains/DotNetCli/DotNetCliCommand.cs
@@ -53,7 +53,7 @@ public DotNetCliCommand WithCliPath(string cliPath)
             => new (cliPath, Arguments, GenerateResult, Logger, BuildPartition, EnvironmentVariables, Timeout, logOutput: LogOutput);
 
         [PublicAPI]
-        public BuildResult RestoreThenBuild()
+        public BuildResult RestoreThenBuild(bool useArtifactsPathIfSupported = true)
         {
             DotNetCliCommandExecutor.LogEnvVars(WithArguments(null));
 
@@ -72,12 +72,12 @@ public BuildResult RestoreThenBuild()
             if (BuildPartition.ForcedNoDependenciesForIntegrationTests)
             {
                 var restoreResult = DotNetCliCommandExecutor.Execute(WithArguments(
-                    GetRestoreCommand(GenerateResult.ArtifactsPaths, BuildPartition, GetWithArtifactsPath(), $"{Arguments} --no-dependencies", "restore-no-deps", excludeOutput: true)));
+                    GetRestoreCommand(GenerateResult.ArtifactsPaths, BuildPartition, GetWithArtifactsPath(useArtifactsPathIfSupported), $"{Arguments} --no-dependencies", "restore-no-deps", excludeOutput: true)));
                 if (!restoreResult.IsSuccess)
                     return BuildResult.Failure(GenerateResult, restoreResult.AllInformation);
 
                 return DotNetCliCommandExecutor.Execute(WithArguments(
-                    GetBuildCommand(GenerateResult.ArtifactsPaths, BuildPartition, GetWithArtifactsPath(), $"{Arguments} --no-restore --no-dependencies", "build-no-restore-no-deps", excludeOutput: true)))
+                    GetBuildCommand(GenerateResult.ArtifactsPaths, BuildPartition, GetWithArtifactsPath(useArtifactsPathIfSupported), $"{Arguments} --no-restore --no-dependencies", "build-no-restore-no-deps", excludeOutput: true)))
                     .ToBuildResult(GenerateResult);
             }
             else
@@ -93,7 +93,7 @@ public BuildResult RestoreThenBuild()
         }
 
         [PublicAPI]
-        public BuildResult RestoreThenBuildThenPublish()
+        public BuildResult RestoreThenBuildThenPublish(bool useArtifactsPathIfSupported = true)
         {
             DotNetCliCommandExecutor.LogEnvVars(WithArguments(null));
 
@@ -105,14 +105,14 @@ public BuildResult RestoreThenBuildThenPublish()
             // so when users go with custom build configuration, we must perform full publish
             // which will internally restore and build for the right configuration
             if (BuildPartition.IsCustomBuildConfiguration)
-                return Publish().ToBuildResult(GenerateResult);
+                return Publish(useArtifactsPathIfSupported).ToBuildResult(GenerateResult);
 
-            var restoreResult = Restore();
+            var restoreResult = Restore(useArtifactsPathIfSupported);
             if (!restoreResult.IsSuccess)
                 return BuildResult.Failure(GenerateResult, restoreResult.AllInformation);
 
             // We use the implicit build in the publish command. We stopped doing a separate build step because we set the --output.
-            return PublishNoRestore().ToBuildResult(GenerateResult);
+            return PublishNoRestore(useArtifactsPathIfSupported).ToBuildResult(GenerateResult);
         }
 
         public DotNetCliCommandResult AddPackages()
@@ -129,28 +129,28 @@ public DotNetCliCommandResult AddPackages()
             return DotNetCliCommandResult.Success(executionTime, stdOutput.ToString());
         }
 
-        private bool GetWithArtifactsPath() => DotNetCliCommandExecutor.DotNetSdkSupportsArtifactsPath(CliPath);
+        private bool GetWithArtifactsPath(bool useArtifactsPathIfSupported) => useArtifactsPathIfSupported && DotNetCliCommandExecutor.DotNetSdkSupportsArtifactsPath(CliPath);
 
-        public DotNetCliCommandResult Restore()
+        public DotNetCliCommandResult Restore(bool useArtifactsPathIfSupported = true)
             => DotNetCliCommandExecutor.Execute(WithArguments(
-                GetRestoreCommand(GenerateResult.ArtifactsPaths, BuildPartition, GetWithArtifactsPath(), Arguments, "restore")));
+                GetRestoreCommand(GenerateResult.ArtifactsPaths, BuildPartition, GetWithArtifactsPath(useArtifactsPathIfSupported), Arguments, "restore")));
 
-        public DotNetCliCommandResult Build()
+        public DotNetCliCommandResult Build(bool useArtifactsPathIfSupported = true)
             => DotNetCliCommandExecutor.Execute(WithArguments(
-                GetBuildCommand(GenerateResult.ArtifactsPaths, BuildPartition, GetWithArtifactsPath(), Arguments, "build")));
+                GetBuildCommand(GenerateResult.ArtifactsPaths, BuildPartition, GetWithArtifactsPath(useArtifactsPathIfSupported), Arguments, "build")));
 
-        public DotNetCliCommandResult BuildNoRestore()
+        public DotNetCliCommandResult BuildNoRestore(bool useArtifactsPathIfSupported = true)
             => DotNetCliCommandExecutor.Execute(WithArguments(
-                GetBuildCommand(GenerateResult.ArtifactsPaths, BuildPartition, GetWithArtifactsPath(), $"{Arguments} --no-restore", "build-no-restore")));
+                GetBuildCommand(GenerateResult.ArtifactsPaths, BuildPartition, GetWithArtifactsPath(useArtifactsPathIfSupported), $"{Arguments} --no-restore", "build-no-restore")));
 
-        public DotNetCliCommandResult Publish()
+        public DotNetCliCommandResult Publish(bool useArtifactsPathIfSupported = true)
             => DotNetCliCommandExecutor.Execute(WithArguments(
-                GetPublishCommand(GenerateResult.ArtifactsPaths, BuildPartition, GetWithArtifactsPath(), Arguments, "publish")));
+                GetPublishCommand(GenerateResult.ArtifactsPaths, BuildPartition, GetWithArtifactsPath(useArtifactsPathIfSupported), Arguments, "publish")));
 
         // PublishNoBuildAndNoRestore was removed because we set --output in the build step. We use the implicit build included in the publish command.
-        public DotNetCliCommandResult PublishNoRestore()
+        public DotNetCliCommandResult PublishNoRestore(bool useArtifactsPathIfSupported = true)
             => DotNetCliCommandExecutor.Execute(WithArguments(
-                GetPublishCommand(GenerateResult.ArtifactsPaths, BuildPartition, GetWithArtifactsPath(), $"{Arguments} --no-restore", "publish-no-restore")));
+                GetPublishCommand(GenerateResult.ArtifactsPaths, BuildPartition, GetWithArtifactsPath(useArtifactsPathIfSupported), $"{Arguments} --no-restore", "publish-no-restore")));
 
         internal static IEnumerable<string> GetAddPackagesCommands(BuildPartition buildPartition)
             => GetNuGetAddPackageCommands(buildPartition.RepresentativeBenchmarkCase, buildPartition.Resolver);

diff --git a/src/BenchmarkDotNet/Toolchains/DotNetCli/DotNetCliPublisher.cs b/src/BenchmarkDotNet/Toolchains/DotNetCli/DotNetCliPublisher.cs
@@ -16,6 +16,7 @@ public class DotNetCliPublisher : DotNetCliBuilderBase
             CustomDotNetCliPath = customDotNetCliPath;
             ExtraArguments = extraArguments;
             EnvironmentVariables = environmentVariables;
+            UseArtifactsPathIfSupported = true;
         }
 
         private string? ExtraArguments { get; }
@@ -31,6 +32,6 @@ public override BuildResult Build(GenerateResult generateResult, BuildPartition
                     buildPartition,
                     EnvironmentVariables,
                     buildPartition.Timeout)
-                .RestoreThenBuildThenPublish();
+                .RestoreThenBuildThenPublish(UseArtifactsPathIfSupported);
     }
 }
diff --git a/src/BenchmarkDotNet/Toolchains/Mono/MonoPublisher.cs b/src/BenchmarkDotNet/Toolchains/Mono/MonoPublisher.cs
@@ -7,7 +7,7 @@
 
 namespace BenchmarkDotNet.Toolchains.Mono
 {
-    public class MonoPublisher : IBuilder
+    public class MonoPublisher : DotNetCliBuilderBase
     {
         public MonoPublisher(string customDotNetCliPath)
         {
@@ -16,15 +16,14 @@ public MonoPublisher(string customDotNetCliPath)
 
             // /p:RuntimeIdentifiers is set explicitly here because --self-contained requires it, see https://github.com/dotnet/sdk/issues/10566
             ExtraArguments = $"--self-contained -r {runtimeIdentifier} /p:UseMonoRuntime=true /p:RuntimeIdentifiers={runtimeIdentifier}";
+            UseArtifactsPathIfSupported = true;
         }
 
-        private string CustomDotNetCliPath { get; }
-
         private string ExtraArguments { get; }
 
         private IReadOnlyList<EnvironmentVariable> EnvironmentVariables { get; }
 
-        public BuildResult Build(GenerateResult generateResult, BuildPartition buildPartition, ILogger logger)
+        public override BuildResult Build(GenerateResult generateResult, BuildPartition buildPartition, ILogger logger)
             => new DotNetCliCommand(
                     CustomDotNetCliPath,
                     ExtraArguments,
@@ -33,6 +32,6 @@ public BuildResult Build(GenerateResult generateResult, BuildPartition buildPart
                     buildPartition,
                     EnvironmentVariables,
                     buildPartition.Timeout)
-                .Publish().ToBuildResult(generateResult);
+                .Publish(UseArtifactsPathIfSupported).ToBuildResult(generateResult);
     }
 }
diff --git a/src/BenchmarkDotNet/Toolchains/MonoAotLLVM/MonoAotLLVMToolChain.cs b/src/BenchmarkDotNet/Toolchains/MonoAotLLVM/MonoAotLLVMToolChain.cs
@@ -24,8 +24,8 @@ public static IToolchain From(NetCoreAppSettings netCoreAppSettings)
                     netCoreAppSettings.CustomRuntimePack,
                     netCoreAppSettings.AOTCompilerPath,
                     netCoreAppSettings.AOTCompilerMode),
-                new DotNetCliBuilder(netCoreAppSettings.TargetFrameworkMoniker,
-                    netCoreAppSettings.CustomDotNetCliPath),
+                // We have no tests for this toolchain, so not including ArtifactsPath in case it fails the same as wasm.
+                new DotNetCliBuilder(netCoreAppSettings.CustomDotNetCliPath, logOutput: false, useArtifactsPathIfSupported: false),
                 new Executor(),
                 netCoreAppSettings.CustomDotNetCliPath);
 

diff --git a/src/BenchmarkDotNet/Toolchains/MonoWasm/WasmToolchain.cs b/src/BenchmarkDotNet/Toolchains/MonoWasm/WasmToolchain.cs
@@ -47,10 +47,11 @@ public static IToolchain From(NetCoreAppSettings netCoreAppSettings)
                         netCoreAppSettings.PackagesPath,
                         netCoreAppSettings.CustomRuntimePack,
                         netCoreAppSettings.AOTCompilerMode == MonoAotLLVM.MonoAotCompilerMode.wasm),
-                    new DotNetCliBuilder(netCoreAppSettings.TargetFrameworkMoniker,
-                        netCoreAppSettings.CustomDotNetCliPath,
+                    new DotNetCliBuilder(netCoreAppSettings.CustomDotNetCliPath,
                         // aot builds can be very slow
-                        logOutput: netCoreAppSettings.AOTCompilerMode == MonoAotLLVM.MonoAotCompilerMode.wasm),
+                        logOutput: netCoreAppSettings.AOTCompilerMode == MonoAotLLVM.MonoAotCompilerMode.wasm,
+                        // Building wasm with ArtifactsPath set fails for some reason (haven't figured out why yet).
+                        useArtifactsPathIfSupported: false),
                     new WasmExecutor(),
                     netCoreAppSettings.CustomDotNetCliPath);
     }

diff --git a/src/BenchmarkDotNet/Toolchains/ToolchainExtensions.cs b/src/BenchmarkDotNet/Toolchains/ToolchainExtensions.cs
@@ -182,5 +182,13 @@ private static IToolchain GetToolchain(RuntimeMoniker runtimeMoniker)
                     throw new ArgumentOutOfRangeException(nameof(runtimeMoniker), runtimeMoniker, "RuntimeMoniker not supported");
             }
         }
+
+        internal static bool IsSafeToBuildInParallel(this IToolchain toolchain)
+            // Toolchains that do not use dotnet sdk are assumed to be safe to build in parallel.
+            // This might not be true for custom toolchains, but the old behavior was to build everything in parallel, so it's at least not a regression.
+            => toolchain.Builder is not DotNetCliBuilderBase dotnetBuilder
+                // If the builder does not use ArtifactsPath, or the dotnet sdk does not support it, it's unsafe to build the same project in parallel.
+                // We cannot rely on falling back to sequential after failure, because the builds can be corrupted. #2425
+                || (dotnetBuilder.UseArtifactsPathIfSupported && DotNetCliCommandExecutor.DotNetSdkSupportsArtifactsPath(dotnetBuilder.CustomDotNetCliPath));
     }
 }