Skip to content

Conversation

@GuillaumeLagrange
Copy link
Contributor

@GuillaumeLagrange GuillaumeLagrange commented Nov 5, 2025

Followup of #33, but the assumption made on what getAdujstedRealTime was doing were wrong

We're also changing the default number of repetitions (round), since we believe 1 is not a good default, as the resulting box plot only has a single element by default

@GuillaumeLagrange GuillaumeLagrange force-pushed the cod-1639-fix-codspeed-cpp-walltimetotaltime-and-maybe branch 2 times, most recently from 01bad40 to 5af4562 Compare November 5, 2025 14:06
@codspeed-hq
Copy link

codspeed-hq bot commented Nov 5, 2025

CodSpeed Performance Report

Merging #34 will not alter performance

Comparing cod-1639-fix-codspeed-cpp-walltimetotaltime-and-maybe (da312ec) with main (8718448)1

Summary

✅ 196 untouched

Footnotes

  1. No successful run was found on main (da312ec) during the generation of this report, so 8718448 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@GuillaumeLagrange GuillaumeLagrange marked this pull request as ready for review November 5, 2025 14:13
Copy link
Member

@not-matthias not-matthias left a comment

Choose a reason for hiding this comment

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

Looks good overall. Can we add some tests to ensure the logic works this time?

@not-matthias not-matthias requested a review from Copilot November 5, 2025 14:39
Copy link

Copilot AI left a comment

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 refactors the walltime benchmark data collection and statistics computation to support variable iterations per round. Instead of storing pre-aggregated statistics (mean, median, stdev) from Google Benchmark, it now collects raw per-round data (iterations and total time) and computes statistics based on per-iteration times.

Key changes:

  • Changed RawWalltimeBenchmark structure to store vectors of iterations and times per round instead of pre-computed aggregates
  • Modified statistics computation to calculate per-iteration times and derive all statistics from these values
  • Increased default benchmark repetitions to 5 when CODSPEED_WALLTIME is enabled for better statistical reliability

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
core/include/codspeed.h Updated RawWalltimeBenchmark struct to store vectors of per-round iterations and times instead of pre-aggregated statistics
core/src/walltime.cpp Refactored statistics computation to work with raw per-round data, computing per-iteration statistics; added constructor to BenchmarkStats and made helper functions static
google_benchmark/src/benchmark.cc Updated data collection to store per-round iterations and times; changed default repetitions to 5 for walltime mode; minor formatting and comment improvements
.github/workflows/ci.yml Updated job step name to better reflect its purpose

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

Copy link
Member

@adriencaccia adriencaccia left a comment

Choose a reason for hiding this comment

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

We can disregard copilot's comments, since we are asserting the length of the array

…as an input

The computation is now in line with that codspeed-rust does, the
interface is ready for other frameworks that may different iterations
count from one round to another.
This allows codspeed to have more rounds and improves default
statistics, which would just be computed as 1 single element in the
boxplot since we only have access to round times.
@GuillaumeLagrange GuillaumeLagrange force-pushed the cod-1639-fix-codspeed-cpp-walltimetotaltime-and-maybe branch 3 times, most recently from fa18cdf to 035fac4 Compare November 6, 2025 10:20
@GuillaumeLagrange GuillaumeLagrange force-pushed the cod-1639-fix-codspeed-cpp-walltimetotaltime-and-maybe branch from 035fac4 to da312ec Compare November 6, 2025 10:29
@GuillaumeLagrange GuillaumeLagrange merged commit da312ec into main Nov 6, 2025
24 checks passed
@GuillaumeLagrange GuillaumeLagrange deleted the cod-1639-fix-codspeed-cpp-walltimetotaltime-and-maybe branch November 6, 2025 10:32
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.

4 participants