fix(testing): emit markers for benchmarks using b.Loop() - #42
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where benchmarks using b.Loop() were not emitting performance markers, preventing proper performance profiling. The issue occurred because b.Loop() uses StopTimerWithoutMarker() internally, leaving the timer off when runN() attempts to call StopTimer(), which then skips marker emission.
Key Changes
- Extracted marker emission logic into a new
AddBenchmarkMarkers()function to enable reuse - Added explicit marker emission in
loopSlowPath()when the benchmark loop completes - Updated the patch file to include these changes in the fork's build process
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| testing/testing/codspeed.go | Added new AddBenchmarkMarkers() helper function to encapsulate the logic for adding benchmark start/stop timestamps with validation |
| testing/testing/benchmark.go | Refactored StopTimer() to use AddBenchmarkMarkers() and added explicit marker emission in loopSlowPath() to fix missing markers for b.Loop() benchmarks |
| testing/patches/benchmark_benchmarkers_bloop.patch | Added patch file containing the benchmark.go changes to be applied during the fork build process |
| testing/fork.sh | Updated fork script to apply the new patch file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
CodSpeed Performance ReportMerging #42 will improve performances by ×2.3Comparing Summary
Benchmarks breakdown
|
Rough outline of what internally happens and why we didn't emit markers: