fix: dont start perf unless it's not already started

[not-matthias]

No description provided.

[Copilot]

Pull Request Overview

This PR adds state management to prevent duplicate benchmark start commands and premature benchmark stop commands in the perf runner's FIFO command handler. The fix ensures that performance event tracking (perf_fifo.start_events() and perf_fifo.stop_events()) is only called when appropriate.

Key changes:

• Introduces a benchmark_started flag to track benchmark state
• Adds guards to ignore duplicate StartBenchmark commands
• Adds guards to ignore StopBenchmark commands when benchmark hasn't started

Comments suppressed due to low confidence (1)

src/run/runner/wall_time/perf/mod.rs:360

• This new edge case handling (duplicate StartBenchmark and premature StopBenchmark) lacks test coverage. Consider adding unit tests to verify:

1. Duplicate StartBenchmark commands are properly ignored
2. StopBenchmark before StartBenchmark is properly handled
3. The state transitions work correctly across multiple start/stop cycles

Other similar modules in the codebase have test coverage (e.g., perf_map.rs, debug_info.rs, unwind_data.rs in the same directory).

        let mut benchmark_started = false;
        loop {
            let perf_ping =
                tokio::time::timeout(Duration::from_secs(perf_ping_timeout), perf_fifo.ping())
                    .await;
            if let Ok(Err(_)) | Err(_) = perf_ping {
                debug!("Failed to ping perf FIFO, ending perf fifo loop");
                break;
            }
            // Perf has started successfully, we can decrease the timeout for future pings
            perf_ping_timeout = 1;

            let result = tokio::time::timeout(Duration::from_secs(5), runner_fifo.recv_cmd()).await;
            let cmd = match result {
                Ok(Ok(cmd)) => cmd,
                Ok(Err(e)) => {
                    warn!("Failed to parse FIFO command: {e}");
                    break;
                }
                Err(_) => continue,
            };
            trace!("Received command: {cmd:?}");

            match cmd {
                FifoCommand::CurrentBenchmark { pid, uri } => {
                    bench_order_by_timestamp.push((current_time(), uri));
                    bench_pids.insert(pid);

                    #[cfg(target_os = "linux")]
                    if !symbols_by_pid.contains_key(&pid) && !unwind_data_by_pid.contains_key(&pid)
                    {
                        Self::process_memory_mappings(
                            pid,
                            &mut symbols_by_pid,
                            &mut unwind_data_by_pid,
                        )?;
                    }

                    runner_fifo.send_cmd(FifoCommand::Ack).await?;
                }
                FifoCommand::StartBenchmark => {
                    if benchmark_started {
                        warn!("Received duplicate StartBenchmark command, ignoring");
                        runner_fifo.send_cmd(FifoCommand::Ack).await?;
                        continue;
                    }
                    benchmark_started = true;
                    markers.push(MarkerType::SampleStart(current_time()));

                    perf_fifo.start_events().await?;
                    runner_fifo.send_cmd(FifoCommand::Ack).await?;
                }
                FifoCommand::StopBenchmark => {
                    if !benchmark_started {
                        warn!("Received StopBenchmark command before StartBenchmark, ignoring");
                        runner_fifo.send_cmd(FifoCommand::Ack).await?;
                        continue;
                    }
                    benchmark_started = false;

                    markers.push(MarkerType::SampleEnd(current_time()));

                    perf_fifo.stop_events().await?;
                    runner_fifo.send_cmd(FifoCommand::Ack).await?;

---

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

[GuillaumeLagrange]

lgtm