feat: add --memory-track-physical experimental flag - #529
Conversation
Merging this PR will not alter performance
|
44ba728 to
ea10d7b
Compare
Greptile SummaryAdds an experimental physical-memory tracking option for memory-mode executions.
Confidence Score: 4/5The PR is not safe to merge until the implemented CLI option matches the advertised The recent naming change makes clap expose Files Needing Attention: src/cli/experimental.rs
|
| Filename | Overview |
|---|---|
| src/cli/experimental.rs | Defines and reports the experimental option, but derives a CLI name that differs from the advertised --memory-track-physical interface. |
| src/cli/run/mod.rs | Correctly forwards the parsed physical-memory setting for run. |
| src/cli/exec/mod.rs | Correctly forwards the parsed physical-memory setting for exec. |
| src/executor/config.rs | Carries the setting from run-level configuration into each execution context. |
| src/executor/memory/executor.rs | Conditionally sets the fixed memtrack environment variable before launching the tracker. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
CLI["run / exec flag or environment"] --> OA["OrchestratorConfig"]
OA --> EC["ExecutorConfig"]
EC --> ME["MemoryExecutor"]
ME --> ENV["CODSPEED_MEMTRACK_TRACK_PHYSICAL=1"]
ENV --> MT["codspeed-memtrack"]
Prompt To Fix All With AI
### Issue 1
src/cli/experimental.rs:22
**Advertised flag is unavailable**
The bare `long` derives `--experimental-memory-track-physical` from the field name, while this feature is advertised as `--memory-track-physical`. Users who invoke the advertised option receive an unknown-argument error instead of enabling physical memory tracking. The warning text at line 47 also uses the derived name, so the public option and warning should both be kept aligned with the advertised interface.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (2): Last reviewed commit: "fix: silence dead-code lint on non-Linux..." | Re-trigger Greptile
Adds an experimental --memory-track-physical flag to `codspeed run` and `codspeed exec`, forwarded to the memtrack subprocess as CODSPEED_MEMTRACK_TRACK_PHYSICAL, matching the env var memtrack itself already reads (crates/memtrack/src/ebpf/tracker.rs). Falsey env values (0/false/no/off) are accepted so the flag can be set via env var without clap's strict true/false bool parsing rejecting it.
MemoryExecutor (the only reader of ExecutorConfig::memory_track_physical) is compiled only on Linux, so macOS clippy (-D warnings) flags the field as dead code.
ea10d7b to
ebc51da
Compare
What
Adds an experimental
--memory-track-physicalflag tocodspeed runandcodspeed exec. When set, it forwardsCODSPEED_MEMTRACK_TRACK_PHYSICAL=1to the memtrack subprocess, enabling therss_stattracepoint plus the folio rmap hooks for physical (resident) memory reconstruction.Why
memtrack already gates this behind the
CODSPEED_MEMTRACK_TRACK_PHYSICALenv var (crates/memtrack/src/ebpf/tracker.rs), inherited implicitly by the subprocess. This adds a discoverable, documented CLI flag under theExperimentalheading (with the standard experimental-flag warning banner), while keeping the underlying env var identical so standalone memtrack usage is unaffected.Implementation
ExperimentalArgs::experimental_memory_track_physical(--memory-track-physical, envCODSPEED_MEMTRACK_TRACK_PHYSICAL), usingFalseyValueParserso the env var's1value parses correctly (clap's default bool parser only acceptstrue/false).OrchestratorConfig/ExecutorConfigalongside the existing valgrind experimental flags (fair_sched, etc.).MemoryExecutor::build_memtrack_commandsetsCODSPEED_MEMTRACK_TRACK_PHYSICAL=1on the memtrack subprocess's environment when the flag is enabled. Default (off) behavior is unchanged, and a user-exported env var still works exactly as before (no CLI flag required).Verification
cargo check --workspace --all-targets,cargo fmt --check,cargo clippy --all-targetsall clean.cargo test --release -p codspeed-runner --lib— all cli/config tests pass (the only failures are pre-existing sudo-gated walltime/valgrind tests unrelated to this change, failing on this box due to no passwordless sudo).codspeed exec --mode memory:--memory-track-physicalprints the experimental warning banner.CODSPEED_MEMTRACK_TRACK_PHYSICAL=1(matching memtrack's own convention) also enables it.