Skip to content

fix(testing): incomplete reading of benchmark time in b.N loops - #43

Merged
not-matthias merged 1 commit into
mainfrom
cod-1776-go-benchmarks-report-1ns-time-for-fuego
Dec 4, 2025
Merged

not-matthias merged 1 commit into
mainfrom
cod-1776-go-benchmarks-report-1ns-time-for-fuego

Conversation

@not-matthias

Copy link
Copy Markdown
Member

No description provided.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a critical bug in the benchmark timing infrastructure where SaveMeasurement() was being called before the timer was stopped, leading to incomplete or inaccurate duration measurements in b.N loop benchmarks. The fix adds a runtime panic guard to detect this condition and reorders the method calls to ensure measurements are only saved after timers have been properly stopped.

Key changes:

  • Added a panic guard in SaveMeasurement() to prevent it from being called while timerOn is true
  • Reordered SaveMeasurement() to be called after StopTimer() in __codspeed_root_frame__runN()
  • Reordered SaveMeasurement() to be called after StopTimerWithoutMarker() in Loop()

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
testing/testing/benchmark.go Added panic guard in SaveMeasurement() and reordered timer stop/measurement save calls in runN() and Loop() to ensure timer is stopped before reading duration
testing/patches/benchmark_savemeasurement_bug.patch Patch file containing the same fixes to be applied during the fork process to Go's standard testing package
testing/fork.sh Added application of the new benchmark_savemeasurement_bug.patch to the build process

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread testing/patches/benchmark_savemeasurement_bug.patch
@codspeed

codspeed Bot commented Dec 4, 2025

Copy link
Copy Markdown

CodSpeed Performance Report

Merging #43 will degrade performances by 29.54%

Comparing cod-1776-go-benchmarks-report-1ns-time-for-fuego (5331397) with main (0ee1cab)

Summary

⚡ 1 improvement
❌ 4 regressions
✅ 43 untouched

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
❌ BenchmarkLargeSetupInLoop 9.8 ms 11.9 ms -17.82%
❌ BenchmarkSleep100ns_Loop 240 ns 300 ns -20%
❌ bench_collect_results[10000000, 10] 2.4 s 3.4 s -29.54%
❌ bench_collect_results[5000000, 25] 2.5 s 3.1 s -18.47%
⚡ bench_go_runner 130.4 s 56 s ×2.3

@adriencaccia adriencaccia left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@not-matthias

Copy link
Copy Markdown
Member Author

@not-matthias
not-matthias merged commit 5331397 into main Dec 4, 2025
21 of 22 checks passed
@not-matthias
not-matthias deleted the cod-1776-go-benchmarks-report-1ns-time-for-fuego branch December 4, 2025 10:40
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.

3 participants