fix(testing): emit markers for benchmarks using b.Loop()

Author: not-matthias

testing/fork.sh

@@ -123,6 +123,7 @@ restore_files "${CODSPEED_FILES[@]}"
 
 apply_patch "patches/benchmark_stopbenchmark_fail.patch" 10 ".."
 apply_patch "patches/benchmark_stoptimer_mitigation.patch" 10 ".."
+apply_patch "patches/benchmark_benchmarkers_bloop.patch" 10 ".."
 
 
 # Run pre-commit and format the code

testing/patches/benchmark_benchmarkers_bloop.patch

@@ -0,0 +1,40 @@
+diff --git a/testing/testing/benchmark.go b/testing/testing/benchmark.go
+index 4947e11..0040973 100644
+--- a/testing/testing/benchmark.go
++++ b/testing/testing/benchmark.go
+@@ -226,15 +226,7 @@ func (b *B) StopTimer() {
+ 	b.StopTimerWithoutMarker()
+
+ 	if timerOn {
+-		if b.startTimestamp >= endTimestamp {
+-			// This should never happen, unless we have a bug in the timer logic.
+-			panic(fmt.Sprintf("Invalid benchmark timestamps: start timestamp (%d) is greater than or equal to end timestamp (%d)", b.startTimestamp, endTimestamp))
+-		}
+-		b.startTimestamps = append(b.startTimestamps, b.startTimestamp)
+-		b.stopTimestamps = append(b.stopTimestamps, endTimestamp)
+-
+-		// Reset to prevent accidental reuse
+-		b.startTimestamp = 0
++		b.AddBenchmarkMarkers(endTimestamp)
+ 	}
+ }
+
+@@ -587,7 +579,18 @@ func (b *B) loopSlowPath() bool {
+ 		more = b.stopOrScaleBLoop()
+ 	}
+ 	if !more {
+-		b.StopTimerWithoutMarker()
++		// NOTE: We could move the endTimestamp capturing further up or even into the Loop() function
++		// but this will result in a huge performance degradation since the C FFI calls are expensive.
++		//
++		// The only downside of having this here, is that there's a small chance of perf sampling the
++		// benchmark framework code which already happens anyway because we only emit 1 pair of
++		// start/stop markers per benchmark to minimize overhead and allow full flamegraphs.
++		endTimestamp := capi.CurrentTimestamp()
++
++		// Edge case: The timer is stopped in b.Loop() which prevents any further calls to
++		// StopTimer() from adding the benchmark markers. We have to manually submit them here,
++		// once the benchmark loop is done.
++		b.AddBenchmarkMarkers(endTimestamp)
+ 		b.codspeed.instrument_hooks.StopBenchmark()
+ 		b.sendAccumulatedTimestamps()

testing/testing/benchmark.go

@@ -226,15 +226,7 @@ func (b *B) StopTimer() {
 	b.StopTimerWithoutMarker()
 
 	if timerOn {
-		if b.startTimestamp >= endTimestamp {
-			// This should never happen, unless we have a bug in the timer logic.
-			panic(fmt.Sprintf("Invalid benchmark timestamps: start timestamp (%d) is greater than or equal to end timestamp (%d)", b.startTimestamp, endTimestamp))
-		}
-		b.startTimestamps = append(b.startTimestamps, b.startTimestamp)
-		b.stopTimestamps = append(b.stopTimestamps, endTimestamp)
-
-		// Reset to prevent accidental reuse
-		b.startTimestamp = 0
+		b.AddBenchmarkMarkers(endTimestamp)
 	}
 }
 
@@ -587,7 +579,18 @@ func (b *B) loopSlowPath() bool {
 		more = b.stopOrScaleBLoop()
 	}
 	if !more {
-		b.StopTimerWithoutMarker()
+		// NOTE: We could move the endTimestamp capturing further up or even into the Loop() function
+		// but this will result in a huge performance degradation since the C FFI calls are expensive.
+		//
+		// The only downside of having this here, is that there's a small chance of perf sampling the
+		// benchmark framework code which already happens anyway because we only emit 1 pair of
+		// start/stop markers per benchmark to minimize overhead and allow full flamegraphs.
+		endTimestamp := capi.CurrentTimestamp()
+
+		// Edge case: The timer is stopped in b.Loop() which prevents any further calls to
+		// StopTimer() from adding the benchmark markers. We have to manually submit them here,
+		// once the benchmark loop is done.
+		b.AddBenchmarkMarkers(endTimestamp)
 		b.codspeed.instrument_hooks.StopBenchmark()
 		b.sendAccumulatedTimestamps()
 

testing/testing/codspeed.go

@@ -74,3 +74,16 @@ func getGitRelativePath(absPath string) string {
 func ensureBenchmarkIsStopped(b *B) {
 	b.codspeed.instrument_hooks.StopBenchmark()
 }
+
+func (b *B) AddBenchmarkMarkers(endTimestamp uint64) {
+	if b.startTimestamp >= endTimestamp {
+		// This should never happen, unless we have a bug in the timer logic.
+		panic(fmt.Sprintf("Invalid benchmark timestamps: start timestamp (%d) is greater than or equal to end timestamp (%d)", b.startTimestamp, endTimestamp))
+	}
+
+	b.startTimestamps = append(b.startTimestamps, b.startTimestamp)
+	b.stopTimestamps = append(b.stopTimestamps, endTimestamp)
+
+	// Reset to prevent accidental reuse
+	b.startTimestamp = 0
+}