feat: add samply profiler support by not-matthias · Pull Request #338 · CodSpeedHQ/codspeed

not-matthias · 2026-05-05T14:15:18Z

No description provided.

codspeed-hq · 2026-05-05T14:18:29Z

Merging this PR will not alter performance

✅ 4 untouched benchmarks

_{Comparing cod-2599-runner-migrate-perf-to-new-profiler-agnostic-data-format (c7f0146) with main (fd8701d)}

GuillaumeLagrange

Switching to github mid review, have to publish 😬

GuillaumeLagrange · 2026-05-07T15:26:58Z

+    ) -> anyhow::Result<CommandBuilder>;
+
+    /// The benchmarked process signaled the start of a measured region.
+    async fn on_start_benchmark(&mut self) -> anyhow::Result<()> {


Could we make use of the fact that we are changing stuff here to clear up the name of the markers?
We've discussed this multiple time, and it's quite a huge clusterfuck

The current names with markers is this

/// It expects the data to be structured like this: /// - SampleStart /// - BenchmarkStart /// - N samples /// - BenchmarkEnd /// - SampleEnd

But we also have the fifo commands StartBenchmark and StopBenchmark which actually push SampleStart/End markers

What I propose for this is just a rename that would make everything a bit clearer

The external marker, currently named SampleStart should be named ProfilerSamplingStart, or something that conveys the idea that we are starting and stopping the profiler's sampling functionality, and we should rename the FifoCommand::StartBenchmark to reflect that. Another naming possibility could be BenchmarkRegion if we want to be a bit farther from the implementation detail of starting/stopping the profiler

The internal marker would be clearer if it was something like RoundStart RoundEnd, as in practice it's what is being done.

WE SHOULD BE ABSOLUTELY EXTREMELY CAREFUL TO MAKE SURE IT DOES NOT IMPACT THE SERIALIZED IDs, it's only meant to be a naming clarification.

WDYT?

Wdyt about this:

pub enum MarkerType { SampleStart(u64), SampleEnd(u64), RoundStart(u64), RoundEnd(u64), }

and then rename the commands to StartProfiler/StopProfiler/PingProfiler

GuillaumeLagrange

first pass done, let me know if you want to sync on some points

GuillaumeLagrange · 2026-05-07T15:37:04Z


 pub struct WallTimeExecutor {
-    perf: Option<PerfRunner>,
+    profiler: Option<Box<dyn Profiler>>,


Since it wont really change at runtime and is computed from static cfg asserts anyway, wouldnt it be better to have a type generic profiler rather than a Box<dyn> ?

We could, but then we'd have to represent the case where we dont' have a profiler (e.g. on Windows). We could have a NoopProfiler that does nothing, but I don't feel like this properly fits what we want to represent.

And the overhead of Box shouldn't be an issue because we're not really calling this in a hot path where the performance matters. So I'd leave it as-is, lmk if you disagree

GuillaumeLagrange · 2026-05-07T16:06:39Z

-        let mut samply_builder = CommandBuilder::new("samply");
+        // samply is bundled into this binary as the `samply` subcommand;
+        // re-exec ourselves so we don't depend on a system install.
+        let current_exe = std::env::current_exe()


If we define the subcommands that are invoked by the CLI, we could have this as a helper that would just take the enum value and build the command with args etc

I'm not quite sure I get what you mean, because we need to pass it as a string to be able to run codspeed samply .... Could you give me an exaxmple?

Relocate src/executor/wall_time/perf/ to src/executor/wall_time/profiler/perf/ in preparation for additional profiler backends (samply).

Output filename also changes from perf.metadata to walltime.metadata. This is a wire-breaking change that requires a coordinated server rollout to read the new filename.

PerfRunner becomes PerfProfiler implementing the Profiler trait. The RunnerFifo loop moves out of the perf module and into WallTimeExecutor, which now drives the trait callbacks (on_start_benchmark / on_stop_benchmark / on_ping / GetIntegrationMode) generically. Profiler::wrap stashes the perf control fifo and output path on the profiler instance; finalize harvests the perf.data artifacts and writes walltime.metadata. The OnceCell<BenchmarkData> bridge is gone — fifo data and timestamps are passed directly into finalize. Executor::run now takes &mut self to allow profilers to hold per-run state on the trait object. This cascades through run_executor, orchestrator, and the Memory/Valgrind executors (no behavioral change for those — they don't mutate self).

…BLED Introduce --enable-profiler / CODSPEED_PROFILER_ENABLED as the canonical flag now that walltime profiling is profiler-agnostic. The legacy --enable-perf / CODSPEED_PERF_ENABLED is kept as a hidden alias and emits a deprecation warning when used, so existing configurations keep working.

Tokio's OpenOptions::read_write is Linux-only, but the O_RDWR trick works on every Unix and is required on macOS to avoid open() deadlocks when both FIFO ends are opened before the peer process is spawned.

Mirror the clock used elsewhere in CodSpeed on macOS (mach_absolute_time with cached mach_timebase_info) so runner and benchmark process timestamps come from the exact same source. Adds mach2 as a macOS-only dependency.

Move the Linux profiling sysctl setup out of the perf profiler and into a shared walltime profiler helper. This lets both perf and samply prepare kernel symbol access and non-root profiling consistently while preserving the existing sudo-based behavior.

Use the samply crate directly instead of a standalone binary. Adds a `codspeed samply` subcommand that forwards args to samply's CLI and dispatches to `do_record_action`. The wall_time profiler re-execs the current binary, removing the need for a PATH-installed samply.

Enabling perf JIT support on macOS produces many unresolved addresses in stack traces. Force it off until we understand why.

Rename `StartBenchmark`/`StopBenchmark` to `StartProfiler`/`StopProfiler` and `BenchmarkStart`/`BenchmarkEnd` markers to `RoundStart`/`RoundEnd` to match the profiler-agnostic data format.

not-matthias force-pushed the cod-2599-runner-migrate-perf-to-new-profiler-agnostic-data-format branch from 468b6c8 to 0e6e4ea Compare May 5, 2026 14:16

not-matthias requested a review from GuillaumeLagrange May 6, 2026 16:50

not-matthias added 3 commits May 6, 2026 18:50

feat(walltime): add Profiler trait abstraction

009bff1

refactor(walltime): rename IntegrationMode::Perf to Walltime

2fcc338

refactor(walltime): rename FifoCommand::PingPerf to PingProfiler

c288d7d

not-matthias force-pushed the cod-2599-runner-migrate-perf-to-new-profiler-agnostic-data-format branch 3 times, most recently from b55c9b2 to 9a2ab1b Compare May 7, 2026 13:37

GuillaumeLagrange requested changes May 7, 2026

View reviewed changes

GuillaumeLagrange reviewed May 7, 2026

View reviewed changes

Comment thread src/executor/wall_time/perf/mod.rs Outdated

GuillaumeLagrange requested changes May 7, 2026

View reviewed changes

not-matthias force-pushed the cod-2599-runner-migrate-perf-to-new-profiler-agnostic-data-format branch 2 times, most recently from a4336d4 to 021df91 Compare May 8, 2026 14:20

not-matthias added 2 commits May 8, 2026 16:20

refactor(walltime): move perf module under profiler/

c075c46

Relocate src/executor/wall_time/perf/ to src/executor/wall_time/profiler/perf/ in preparation for additional profiler backends (samply).

refactor(walltime): rename PerfMetadata to WalltimeMetadata

260c151

Output filename also changes from perf.metadata to walltime.metadata. This is a wire-breaking change that requires a coordinated server rollout to read the new filename.

not-matthias force-pushed the cod-2599-runner-migrate-perf-to-new-profiler-agnostic-data-format branch from 021df91 to 0fdb0de Compare May 8, 2026 14:36

not-matthias requested a review from GuillaumeLagrange May 8, 2026 16:32

not-matthias added 7 commits May 8, 2026 18:35

fix(fifo): use O_RDWR to open FIFOs on all Unix platforms

ae87abb

Tokio's OpenOptions::read_write is Linux-only, but the O_RDWR trick works on every Unix and is required on macOS to avoid open() deadlocks when both FIFO ends are opened before the peer process is spawned.

fix(walltime): use mach_absolute_time for FIFO timestamps on macOS

e108a33

Mirror the clock used elsewhere in CodSpeed on macOS (mach_absolute_time with cached mach_timebase_info) so runner and benchmark process timestamps come from the exact same source. Adds mach2 as a macOS-only dependency.

feat(walltime): add samply profiler for macOS

af25ae3

not-matthias force-pushed the cod-2599-runner-migrate-perf-to-new-profiler-agnostic-data-format branch from 2140160 to dd3a1e3 Compare May 8, 2026 16:37

not-matthias added 3 commits May 8, 2026 18:52

fix(walltime): disable PYTHON_PERF_JIT_SUPPORT on macOS

604f152

Enabling perf JIT support on macOS produces many unresolved addresses in stack traces. Force it off until we understand why.

refactor(walltime): rename Profiler::wrap to wrap_command

a7ef859

refactor: rename Benchmark FIFO commands/markers to Profiler/Round

c7f0146

Rename `StartBenchmark`/`StopBenchmark` to `StartProfiler`/`StopProfiler` and `BenchmarkStart`/`BenchmarkEnd` markers to `RoundStart`/`RoundEnd` to match the profiler-agnostic data format.

not-matthias force-pushed the cod-2599-runner-migrate-perf-to-new-profiler-agnostic-data-format branch 2 times, most recently from dd3a1e3 to c7f0146 Compare May 8, 2026 16:52

Conversation

not-matthias commented May 5, 2026

Uh oh!

codspeed-hq Bot commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Uh oh!

GuillaumeLagrange left a comment

Choose a reason for hiding this comment

Uh oh!

GuillaumeLagrange May 7, 2026

Choose a reason for hiding this comment

Uh oh!

not-matthias May 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GuillaumeLagrange left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GuillaumeLagrange May 7, 2026

Choose a reason for hiding this comment

Uh oh!

not-matthias May 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GuillaumeLagrange May 7, 2026

Choose a reason for hiding this comment

Uh oh!

not-matthias May 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codspeed-hq Bot commented May 5, 2026 •

edited

Loading