Skip to content

Improve samply's stack unwinding, add support to dump node's inlined information#415

Open
GuillaumeLagrange wants to merge 3 commits into
mainfrom
cod-2821-v8-maglevturbofan-inlining-loses-nested-inline-information
Open

Improve samply's stack unwinding, add support to dump node's inlined information#415
GuillaumeLagrange wants to merge 3 commits into
mainfrom
cod-2821-v8-maglevturbofan-inlining-loses-nested-inline-information

Conversation

@GuillaumeLagrange

Copy link
Copy Markdown
Contributor

No description provided.

@codspeed-hq

codspeed-hq Bot commented Jun 18, 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.

✅ 7 untouched benchmarks


Comparing cod-2821-v8-maglevturbofan-inlining-loses-nested-inline-information (44904cf) with main (7b4f51c)

Open in CodSpeed

@greptile-apps

greptile-apps Bot commented Jun 18, 2026

Copy link
Copy Markdown

Greptile Summary

This PR refactors how isolation (systemd-run --scope) and privilege escalation (sudo) are applied to benchmark commands, extracting wrap_with_isolation_privilege so every code path goes through a single, consistent privilege layer. It also adds V8 inline-log support to the Samply profiler so inlined JavaScript frames can be expanded in profiles.

  • wrap_with_isolation is now called unconditionally in executor.rs before handing the command to any profiler; each profiler then wraps around the already-isolated command and applies its own privilege escalation via the new wrap_with_isolation_privilege helper.
  • CODSPEED_V8_LOG is set on the Samply command builder so the V8 runtime can drop per-process code logs that Samply reads back to expand inlined frames; on macOS this env reaches the runtime via normal inheritance, but on Linux the systemd-run --scope isolation boundary means the inner process may not see it (noted in the existing review thread).
  • The requires_isolation() method (always true) is removed from the Profiler trait since isolation is now always applied unconditionally upstream.

Confidence Score: 5/5

The isolation/privilege refactoring is structurally sound and behaviorally equivalent to the old code on all platforms; no new correctness regressions were introduced.

The command-building pipeline is cleanly refactored: wrap_with_isolation is applied unconditionally before any profiler wraps, and wrap_with_isolation_privilege consolidates sudo handling symmetrically across all paths. The final command shape (sudo -- [profiler] -- systemd-run -- bash) is identical to what was produced before the refactor. The CODSPEED_V8_LOG concern on Linux was already flagged in the existing review thread. No new defects were found.

src/executor/wall_time/profiler/samply/mod.rs — the CODSPEED_V8_LOG env variable and whether it reaches the benchmark process inside the systemd-run scope on Linux.

Important Files Changed

Filename Overview
src/executor/wall_time/executor.rs Refactored command-building pipeline: isolation is now applied unconditionally before profilers, and privilege escalation is delegated to the new wrap_with_isolation_privilege helper for both the profiler and no-profiler paths.
src/executor/wall_time/isolation.rs Adds wrap_with_isolation_privilege (Linux: delegates to wrap_with_sudo; non-Linux: no-op) alongside the existing wrap_with_isolation, with correct #[cfg] guards on both platforms.
src/executor/wall_time/profiler/samply/mod.rs Adds CODSPEED_V8_LOG env for V8 inline-frame expansion and moves privilege escalation into wrap_command via wrap_with_isolation_privilege. On macOS the env reaches the runtime via normal inheritance; on Linux, systemd-run --scope prevents the inner process from seeing env vars not baked into the env-file.
src/executor/wall_time/profiler/perf/mod.rs Replaces direct wrap_with_sudo call with the new wrap_with_isolation_privilege abstraction; no behavioral change on Linux.
src/executor/wall_time/profiler/mod.rs Removes the requires_isolation() trait method (always true); the invariant is now enforced structurally in executor.rs.
Cargo.lock Updates rev pins for framehop, linux-perf-data, and linux-perf-event-reader; all remain rev-pinned as required.
crates/samply-codspeed Submodule pointer bumped to pull in upstream samply improvements for stack unwinding.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant E as executor.rs (run)
    participant W as walltime_bench_cmd
    participant I as wrap_with_isolation
    participant P as profiler.wrap_command
    participant IP as wrap_with_isolation_privilege

    E->>W: build bench_cmd (bash + env-file)
    W-->>E: CommandBuilder [bash script]
    E->>I: wrap_with_isolation(cmd)
    I-->>E: [systemd-run --scope -- bash script]

    alt Profiler active (Perf or Samply)
        E->>P: wrap_command([systemd-run -- bash])
        P-->>P: wrap with profiler binary
        P->>IP: wrap_with_isolation_privilege
        IP-->>P: [sudo -- profiler -- systemd-run -- bash]
        P-->>E: final cmd
    else No profiler
        E->>IP: wrap_with_isolation_privilege([systemd-run -- bash])
        IP-->>E: [sudo -- systemd-run -- bash]
    end
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant E as executor.rs (run)
    participant W as walltime_bench_cmd
    participant I as wrap_with_isolation
    participant P as profiler.wrap_command
    participant IP as wrap_with_isolation_privilege

    E->>W: build bench_cmd (bash + env-file)
    W-->>E: CommandBuilder [bash script]
    E->>I: wrap_with_isolation(cmd)
    I-->>E: [systemd-run --scope -- bash script]

    alt Profiler active (Perf or Samply)
        E->>P: wrap_command([systemd-run -- bash])
        P-->>P: wrap with profiler binary
        P->>IP: wrap_with_isolation_privilege
        IP-->>P: [sudo -- profiler -- systemd-run -- bash]
        P-->>E: final cmd
    else No profiler
        E->>IP: wrap_with_isolation_privilege([systemd-run -- bash])
        IP-->>E: [sudo -- systemd-run -- bash]
    end
Loading

Reviews (2): Last reviewed commit: "feat: bump samply" | Re-trigger Greptile

Comment thread src/executor/wall_time/profiler/samply/mod.rs

@not-matthias not-matthias left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm, small comments. lmk if you have any other opinons on them.

}

#[cfg(target_os = "linux")]
pub fn wrap_with_isolation_privilege(cmd: CommandBuilder) -> Result<CommandBuilder> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should'nt we also call wrap_with_isolation here? From the method name, i'd assume it's setting up both isolation and sudo permissions.

Comment on lines +117 to +121
// Directory where the profiled runtime drops its V8 code log (one
// `codspeed-v8-<pid>.log` per process); samply reads them back per
// jitdump pid to expand inlined frames. Both the writer (the runtime)
// and the reader (samply) see this through the inherited environment.
cmd_builder.env("CODSPEED_V8_LOG", profile_folder);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't this be something samply does, rather than the runner?

I get that we're uploading the logs in this case, but maybe we don't really need it as the other profiling information is also not uploaded.

@GuillaumeLagrange GuillaumeLagrange force-pushed the cod-2821-v8-maglevturbofan-inlining-loses-nested-inline-information branch from d140595 to 44904cf Compare June 19, 2026 16:06
@GuillaumeLagrange GuillaumeLagrange force-pushed the cod-2821-v8-maglevturbofan-inlining-loses-nested-inline-information branch from 44904cf to be8229f Compare June 19, 2026 16:06
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