PR feedback.

timcassell · timcassell · commit 52cabddf434d · 2025-07-13T17:43:17.000-04:00
diff --git a/src/BenchmarkDotNet/Engines/EngineJitStage.cs b/src/BenchmarkDotNet/Engines/EngineJitStage.cs
@@ -8,9 +8,8 @@
 
 namespace BenchmarkDotNet.Engines
 {
-    internal abstract class EngineJitStage(int unrollFactor, EngineParameters parameters) : EngineStage(IterationStage.Jitting, IterationMode.Workload, parameters)
+    internal abstract class EngineJitStage(EngineParameters parameters) : EngineStage(IterationStage.Jitting, IterationMode.Workload, parameters)
     {
-        protected readonly int unrollFactor = unrollFactor;
         protected readonly Action<long> dummy1Action= _ => parameters.Dummy1Action();
         protected readonly Action<long> dummy2Action= _ => parameters.Dummy2Action();
         protected readonly Action<long> dummy3Action = _ => parameters.Dummy3Action();
@@ -24,26 +23,13 @@ internal sealed class EngineFirstJitStage : EngineJitStage
         // It is not worth spending a long time in jit stage for macro-benchmarks.
         private static readonly TimeInterval MaxTieringTime = TimeInterval.FromSeconds(10);
 
-        internal bool didJitUnroll = false;
         internal bool didStopEarly = false;
         internal Measurement lastMeasurement;
 
-        private readonly Action<long> overheadAction;
-        private readonly Action<long> workloadAction;
         private readonly IEnumerator<IterationData> enumerator;
 
-        internal EngineFirstJitStage(int unrollFactor, EngineParameters parameters) : base(unrollFactor, parameters)
+        internal EngineFirstJitStage(EngineParameters parameters) : base(parameters)
         {
-            if (unrollFactor != 1)
-            {
-                overheadAction = parameters.OverheadActionUnroll;
-                workloadAction = parameters.WorkloadActionUnroll;
-            }
-            else
-            {
-                overheadAction = parameters.OverheadActionNoUnroll;
-                workloadAction = parameters.WorkloadActionNoUnroll;
-            }
             enumerator = EnumerateIterations();
         }
 
@@ -88,9 +74,9 @@ private IEnumerator<IterationData> EnumerateIterations()
         {
             ++iterationIndex;
             yield return GetDummyIterationData(dummy1Action);
-            yield return GetOverheadNoUnrollIterationData();
+            yield return GetOverheadIterationData();
             yield return GetDummyIterationData(dummy2Action);
-            yield return GetWorkloadNoUnrollIterationData();
+            yield return GetWorkloadIterationData();
             yield return GetDummyIterationData(dummy3Action);
 
             // If the jit is not tiered, we're done.
@@ -102,103 +88,33 @@ private IEnumerator<IterationData> EnumerateIterations()
             // Wait enough time for jit call counting to begin.
             MaybeSleep(JitInfo.TieredDelay);
 
-            // If the jit is configured for aggressive tiering, ignore how long it takes, and run 1 set of iterations per jit tier to fully promote the methods to tier1.
-            if (JitInfo.TieredCallCountThreshold == 1)
-            {
-                ++iterationIndex;
-                yield return GetOverheadNoUnrollIterationData();
-                yield return GetWorkloadNoUnrollIterationData();
-
-                MaybeSleep(JitInfo.BackgroundCompilationDelay);
-
-                if (JitInfo.IsDPGO)
-                {
-                    ++iterationIndex;
-                    yield return GetOverheadNoUnrollIterationData();
-                    yield return GetWorkloadNoUnrollIterationData();
-                }
-
-                MaybeSleep(JitInfo.BackgroundCompilationDelay);
-
-                yield break;
-            }
-
-            // Otherwise, attempt to promote methods to tier1, but don't spend too much time in jit stage.
+            // Attempt to promote methods to tier1, but don't spend too much time in jit stage.
             StartedClock startedClock = parameters.TargetJob.ResolveValue(InfrastructureMode.ClockCharacteristic, parameters.Resolver).Start();
 
-            ++iterationIndex;
-            yield return GetOverheadNoUnrollIterationData();
-            yield return GetWorkloadNoUnrollIterationData();
-
-            if (startedClock.GetElapsed().GetTimeValue() >= MaxTieringTime)
-            {
-                didStopEarly = true;
-                yield break;
-            }
-
-            int tier0CallCount = JitInfo.TieredCallCountThreshold - 1;
-            // Run unroll method if it's not too many invocations.
-            if (unrollFactor <= tier0CallCount)
-            {
-                tier0CallCount -= unrollFactor;
-                ++iterationIndex;
-                yield return GetOverheadUnrollIterationData();
-                yield return GetWorkloadUnrollIterationData();
-
-                didJitUnroll = unrollFactor != 1;
-
-                if (startedClock.GetElapsed().GetTimeValue() >= MaxTieringTime)
-                {
-                    didStopEarly = true;
-                    yield break;
-                }
-            }
-
-            // Run the remaining iterations without unroll.
-            for (int i = 0; i < tier0CallCount; ++i)
-            {
-                ++iterationIndex;
-                yield return GetOverheadNoUnrollIterationData();
-                yield return GetWorkloadNoUnrollIterationData();
-
-                if (startedClock.GetElapsed().GetTimeValue() >= MaxTieringTime)
-                {
-                    didStopEarly = true;
-                    yield break;
-                }
-            }
-
-            MaybeSleep(JitInfo.BackgroundCompilationDelay);
-
-            if (JitInfo.IsDPGO)
+            for (int tierCount = JitInfo.IsDPGO ? 2 : 1; tierCount >= 0; --tierCount)
             {
-                for (int i = 0; i < JitInfo.TieredCallCountThreshold; ++i)
+                for (int callCount = JitInfo.TieredCallCountThreshold; callCount >= 0; --callCount)
                 {
                     ++iterationIndex;
-                    yield return GetOverheadNoUnrollIterationData();
-                    yield return GetWorkloadNoUnrollIterationData();
+                    yield return GetOverheadIterationData();
+                    yield return GetWorkloadIterationData();
 
-                    if (startedClock.GetElapsed().GetTimeValue() >= MaxTieringTime)
+                    if ((tierCount + callCount) > 0
+                        && startedClock.GetElapsed().GetTimeValue() >= MaxTieringTime)
                     {
                         didStopEarly = true;
                         yield break;
                     }
                 }
-            }
 
-            MaybeSleep(JitInfo.BackgroundCompilationDelay);
+                MaybeSleep(JitInfo.BackgroundCompilationDelay);
+            }
         }
 
-        private IterationData GetOverheadUnrollIterationData()
-            => new(IterationMode.Overhead, IterationStage.Jitting, iterationIndex, unrollFactor, unrollFactor, () => { }, () => { }, overheadAction);
-
-        private IterationData GetWorkloadUnrollIterationData()
-            => new(IterationMode.Workload, IterationStage.Jitting, iterationIndex, unrollFactor, unrollFactor, parameters.IterationSetupAction, parameters.IterationCleanupAction, workloadAction);
-
-        private IterationData GetOverheadNoUnrollIterationData()
+        private IterationData GetOverheadIterationData()
             => new(IterationMode.Overhead, IterationStage.Jitting, iterationIndex, 1, 1, () => { }, () => { }, parameters.OverheadActionNoUnroll);
 
-        private IterationData GetWorkloadNoUnrollIterationData()
+        private IterationData GetWorkloadIterationData()
             => new(IterationMode.Workload, IterationStage.Jitting, iterationIndex, 1, 1, parameters.IterationSetupAction, parameters.IterationCleanupAction, parameters.WorkloadActionNoUnroll);
 
         private void MaybeSleep(TimeSpan timeSpan)
@@ -210,8 +126,10 @@ private void MaybeSleep(TimeSpan timeSpan)
         }
     }
 
-    internal sealed class EngineSecondJitStage(int unrollFactor, EngineParameters parameters) : EngineJitStage(unrollFactor, parameters)
+    internal sealed class EngineSecondJitStage(int unrollFactor, EngineParameters parameters) : EngineJitStage(parameters)
     {
+        private readonly int unrollFactor = unrollFactor;
+
         internal override List<Measurement> GetMeasurementList() => new(GetMaxCallCount());
 
         private static int GetMaxCallCount()
diff --git a/src/BenchmarkDotNet/Engines/EngineStage.cs b/src/BenchmarkDotNet/Engines/EngineStage.cs
@@ -33,7 +33,7 @@ internal static IEnumerable<EngineStage> EnumerateStages(EngineParameters parame
                     int minInvokeCount = parameters.TargetJob.ResolveValue(AccuracyMode.MinInvokeCountCharacteristic, parameters.Resolver);
 
                     // AOT technically doesn't have a JIT, but we run jit stage regardless because of static constructors. #2004
-                    var jitStage = new EngineFirstJitStage(unrollFactor, parameters);
+                    var jitStage = new EngineFirstJitStage(parameters);
                     yield return jitStage;
 
                     bool hasUnrollFactor = parameters.TargetJob.HasValue(RunMode.UnrollFactorCharacteristic);
@@ -58,8 +58,9 @@ internal static IEnumerable<EngineStage> EnumerateStages(EngineParameters parame
                         skipPilotStage = !pilotStage.needsFurtherPilot;
                     }
 
-                    // If the first jit stage only jitted *NoUnroll methods, now we need to jit *Unroll methods if they're going to be used.
-                    if (!RuntimeInformation.IsAot && !jitStage.didJitUnroll && unrollFactor != 1)
+                    // The first jit stage only jitted *NoUnroll methods, now we need to jit *Unroll methods if they're going to be used.
+                    // TODO: This stage can be removed after we refactor the engine/codegen to pass the clock into the delegates.
+                    if (!RuntimeInformation.IsAot && unrollFactor != 1)
                     {
                         yield return new EngineSecondJitStage(unrollFactor, parameters);
                     }
diff --git a/src/BenchmarkDotNet/Extensions/ProcessExtensions.cs b/src/BenchmarkDotNet/Extensions/ProcessExtensions.cs
@@ -150,11 +150,8 @@ internal static void SetEnvironmentVariables(this ProcessStartInfo start, Benchm
             // disable ReSharper's Dynamic Program Analysis (see https://github.com/dotnet/BenchmarkDotNet/issues/1871 for details)
             start.EnvironmentVariables["JETBRAINS_DPA_AGENT_ENABLE"] = "0";
 
-            // Configure JIT to tier up aggressively.
-            SetClrEnvironmentVariables(start, JitInfo.CallCountThresholdEnv, "1");
-            // Ideally we would set both of these env vars, but AggressiveTiering sets CallCountingDelayMs to 0, and that breaks DisassemblyDiagnoser. https://github.com/dotnet/runtime/issues/117339
+            // TODO: set CallCountingDelayMs without breaking DisassemblyDiagnoser. https://github.com/dotnet/runtime/issues/117339
             //SetClrEnvironmentVariables(start, JitInfo.CallCountingDelayMsEnv, "0");
-            //SetClrEnvironmentVariables(start, JitInfo.AggressiveTieringEnv, "1");
 
             if (!benchmarkCase.Job.HasValue(EnvironmentMode.EnvironmentVariablesCharacteristic))
                 return;
diff --git a/tests/BenchmarkDotNet.Tests/Engine/EngineFactoryTests.cs b/tests/BenchmarkDotNet.Tests/Engine/EngineFactoryTests.cs
@@ -133,37 +133,36 @@ public void BenchmarksThatRunLongerThanIterationTimeOnlyDuringFirstInvocationAre
         }
 
         [Fact]
-        public void ForJobsWithExplicitUnrollFactorTheGlobalSetupIsCalledAndMultiActionCodeGetsJitted()
-            => AssertGlobalSetupWasCalledAndMultiActionGotJitted(Job.Default.WithUnrollFactor(16));
+        public void ForJobsWithExplicitUnrollFactorTheGlobalSetupAndUnrollAreCalled()
+            => AssertGlobalSetupWasCalledAndUnrollWasRan(Job.Default.WithUnrollFactor(16));
 
         [Fact]
-        public void ForJobsThatDontRequirePilotTheGlobalSetupIsCalledAndMultiActionCodeGetsJitted()
-            => AssertGlobalSetupWasCalledAndMultiActionGotJitted(Job.Default.WithInvocationCount(100));
+        public void ForJobsThatDontRequirePilotTheGlobalSetupAndUnrollAreCalled()
+            => AssertGlobalSetupWasCalledAndUnrollWasRan(Job.Default.WithInvocationCount(100));
 
         [Fact]
         public void NonVeryTimeConsumingBenchmarksAreExecutedMoreThanOncePerIterationWithUnrollFactorForDefaultSettings()
-            => AssertGlobalSetupWasCalledAndMultiActionGotJitted(Job.Default);
+            => AssertGlobalSetupWasCalledAndUnrollWasRan(Job.Default);
 
-        private void AssertGlobalSetupWasCalledAndMultiActionGotJitted(Job job)
+        private void AssertGlobalSetupWasCalledAndUnrollWasRan(Job job)
         {
             var engineParameters = CreateEngineParameters(job);
 
             var engine = (Engines.Engine) new EngineFactory().CreateReadyToRun(engineParameters);
+            bool didRunUnroll = false;
             foreach (var stage in EngineStage.EnumerateStages(engine.Parameters))
             {
                 var stageMeasurements = stage.GetMeasurementList();
                 while (stage.GetShouldRunIteration(stageMeasurements, out var iterationData))
                 {
+                    didRunUnroll |= iterationData.unrollFactor > 1;
                     var measurement = new Measurement(0, iterationData.mode, iterationData.stage, iterationData.index, 1, 1);
                     stageMeasurements.Add(measurement);
                 }
-
-                Assert.IsType<EngineFirstJitStage>(stage);
-                var jitStage = (EngineFirstJitStage) stage;
-                Assert.True(jitStage.didJitUnroll);
-                break;
             }
 
+            Assert.True(didRunUnroll);
+
             Assert.Equal(1, timesGlobalSetupCalled);
             Assert.Equal(0, timesGlobalCleanupCalled);
 

Original file line number	Diff line number	Diff line change
`@@ -33,7 +33,7 @@ internal static IEnumerable<EngineStage> EnumerateStages(EngineParameters parame`
`33`	`33`	`int minInvokeCount = parameters.TargetJob.ResolveValue(AccuracyMode.MinInvokeCountCharacteristic, parameters.Resolver);`
`34`	`34`
`35`	`35`	`// AOT technically doesn't have a JIT, but we run jit stage regardless because of static constructors. #2004`
`36`		`- var jitStage = new EngineFirstJitStage(unrollFactor, parameters);`
	`36`	`+ var jitStage = new EngineFirstJitStage(parameters);`
`37`	`37`	`yield return jitStage;`
`38`	`38`
`39`	`39`	`bool hasUnrollFactor = parameters.TargetJob.HasValue(RunMode.UnrollFactorCharacteristic);`
`@@ -58,8 +58,9 @@ internal static IEnumerable<EngineStage> EnumerateStages(EngineParameters parame`
`58`	`58`	`skipPilotStage = !pilotStage.needsFurtherPilot;`
`59`	`59`	`}`
`60`	`60`
`61`		`- // If the first jit stage only jitted NoUnroll methods, now we need to jit Unroll methods if they're going to be used.`
`62`		`- if (!RuntimeInformation.IsAot && !jitStage.didJitUnroll && unrollFactor != 1)`
	`61`	`+ // The first jit stage only jitted NoUnroll methods, now we need to jit Unroll methods if they're going to be used.`
	`62`	`+ // TODO: This stage can be removed after we refactor the engine/codegen to pass the clock into the delegates.`
	`63`	`+ if (!RuntimeInformation.IsAot && unrollFactor != 1)`
`63`	`64`	`{`
`64`	`65`	`yield return new EngineSecondJitStage(unrollFactor, parameters);`
`65`	`66`	`}`