Skip to content

feat: add --memory-track-physical experimental flag - #529

Merged
not-matthias merged 2 commits into
mainfrom
cod-3500-expose-rss-physical-memory-tracking-via-experimental-flag
Sep 7, 2026
Merged

feat: add --memory-track-physical experimental flag#529
not-matthias merged 2 commits into
mainfrom
cod-3500-expose-rss-physical-memory-tracking-via-experimental-flag

Conversation

@not-matthias

Copy link
Copy Markdown
Member

What

Adds an experimental --memory-track-physical flag to codspeed run and codspeed exec. When set, it forwards CODSPEED_MEMTRACK_TRACK_PHYSICAL=1 to the memtrack subprocess, enabling the rss_stat tracepoint plus the folio rmap hooks for physical (resident) memory reconstruction.

Why

memtrack already gates this behind the CODSPEED_MEMTRACK_TRACK_PHYSICAL env var (crates/memtrack/src/ebpf/tracker.rs), inherited implicitly by the subprocess. This adds a discoverable, documented CLI flag under the Experimental heading (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, env CODSPEED_MEMTRACK_TRACK_PHYSICAL), using FalseyValueParser so the env var's 1 value parses correctly (clap's default bool parser only accepts true/false).
  • Threaded through OrchestratorConfig / ExecutorConfig alongside the existing valgrind experimental flags (fair_sched, etc.).
  • MemoryExecutor::build_memtrack_command sets CODSPEED_MEMTRACK_TRACK_PHYSICAL=1 on 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-targets all 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).
  • Manually verified via codspeed exec --mode memory:
    • --memory-track-physical prints the experimental warning banner.
    • CODSPEED_MEMTRACK_TRACK_PHYSICAL=1 (matching memtrack's own convention) also enables it.
    • Omitting both leaves the flag off (default).

@codspeed-hq

codspeed-hq Bot commented Sep 7, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

✅ 17 untouched benchmarks


Comparing cod-3500-expose-rss-physical-memory-tracking-via-experimental-flag (ebc51da) with main (a4bd056)

Open in CodSpeed

@not-matthias
not-matthias force-pushed the cod-3500-expose-rss-physical-memory-tracking-via-experimental-flag branch from 44ba728 to ea10d7b Compare September 7, 2026 09:50
@not-matthias
not-matthias marked this pull request as ready for review September 7, 2026 10:01
@greptile-apps

greptile-apps Bot commented Sep 7, 2026

Copy link
Copy Markdown

Greptile Summary

Adds an experimental physical-memory tracking option for memory-mode executions.

  • Propagates the parsed setting through orchestrator and executor configuration.
  • Sets CODSPEED_MEMTRACK_TRACK_PHYSICAL=1 on the memtrack subprocess when enabled.
  • Adds the option to experimental-feature warnings, but currently exposes a different CLI name than the documented interface.

Confidence Score: 4/5

The PR is not safe to merge until the implemented CLI option matches the advertised --memory-track-physical interface.

The recent naming change makes clap expose --experimental-memory-track-physical, so invoking the documented option fails before physical tracking can be enabled. The earlier tracker-version thread was manually resolved after not-matthias stated that a supporting release would be available soon and therefore is not treated as outstanding.

Files Needing Attention: src/cli/experimental.rs

Important Files Changed

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"]
Loading

Fix all with Greploop Fix All in Claude Code Fix All in Codex

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

Comment thread src/executor/memory/executor.rs
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.
@not-matthias
not-matthias force-pushed the cod-3500-expose-rss-physical-memory-tracking-via-experimental-flag branch from ea10d7b to ebc51da Compare September 7, 2026 12:22
Comment thread src/cli/experimental.rs
@not-matthias
not-matthias merged commit ebc51da into main Sep 7, 2026
48 checks passed
@not-matthias
not-matthias deleted the cod-3500-expose-rss-physical-memory-tracking-via-experimental-flag branch September 7, 2026 12:43
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.

2 participants