Skip to content

fix(walltime): infer Go and Java unwinding for both profilers - #603

Open
not-matthias wants to merge 2 commits into
cod-3877-expose-stack-sampling-size-in-clifrom
cod-3861-support-samply-fp-unwinding
Open

not-matthias wants to merge 2 commits into
cod-3877-expose-stack-sampling-size-in-clifrom
cod-3861-support-samply-fp-unwinding

Conversation

@not-matthias

Copy link
Copy Markdown
Member

TLDR: Detect Go and Java commands before exec-harness serialization and use frame-pointer unwinding with both perf and samply (Linux).

  • Mixed exec groups use frame pointers if any target is detected as Go or Java; explicit overrides win.
  • Preserve backend-specific DWARF defaults and scope Go warning suppression to each benchmark.
  • Verified live Go/Java recordings, mixed groups, DWARF overrides, and warning-suppression transitions with both profilers. Library tests and Clippy pass.

Stacked on #586; review against its branch.

Fixes COD-3861

Resolve unwinding from original benchmark targets before exec-harness serialization. Use frame pointers for groups containing Go or Java, preserve explicit overrides and backend DWARF defaults, and scope Go warning suppression to each benchmark environment.
@not-matthias
not-matthias marked this pull request as ready for review November 2, 2026 18:17
Preserve suggested_unwinding unchanged and keep mixed-target aggregation and profiler-specific policy in the resolver.
@codspeed

codspeed Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

Merging this PR will not alter performance

2 warnings

⚠️ Walltime results may be unreliable

Some benchmarks used the Walltime instrument outside CodSpeed Macro Runners, which can make the results inconsistent.

Learn more about Macro Runners

See affected benchmarks

⚠️ 8 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

✅ 41 untouched benchmarks
⏩ 6 skipped benchmarks1


Comparing cod-3861-support-samply-fp-unwinding (3bd4427) with cod-3877-expose-stack-sampling-size-in-cli (f77fdc8)

Open in CodSpeed

Footnotes

  1. 6 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@greptile-apps

greptile-apps Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 4/5

[Medium impact] The change affects Linux wall-time profiling with perf and samply.

Fix exec argument detection before merging so ordinary arguments cannot select the wrong unwinding mode.

Fix All in Claude CodeFindings

  1. P1 Arguments select the wrong unwinding ▶
Fix with agent prompt
### Issue 1
src/executor/orchestrator.rs:153-157
Every exec argument is checked as an executable name. For example, `codspeed exec -m walltime -- grep go corpus.txt` selects frame-pointer unwinding because the pattern `go` reaches `suggested_unwinding` as a command. If the native program was built without frame pointers, its call stacks will be incomplete. Every other target in the same recording also gets this setting.

Keep each target's argument list intact and distinguish its executable from ordinary arguments before inferring the runtime.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

Moves unwinding inference to the orchestrator so it sees original benchmark targets and applies to both profilers.

  • Linux wall-time runs choose unwinding from the original benchmark commands.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Original benchmark targets] --> B[Infer unwinding]
    C[Explicit setting] --> D[Resolve per-run setting]
    B --> D
    D --> E[Build benchmark environment]
    E --> F{Profiler}
    F --> G[perf call graph option]
    F --> H[samply unwinding options]
Loading

Reviews (1) · Last reviewed commit: "fixup! fix(walltime): infer Go and Java ..." · Reviewed by Greptile

Comment on lines +153 to +157
BenchmarkTarget::Exec { command, .. } => Some(command),
BenchmarkTarget::Entrypoint { .. } => None,
})
.flatten()
.map(String::as_str);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Arguments select the wrong unwinding

Every exec argument is checked as an executable name. For example, codspeed exec -m walltime -- grep go corpus.txt selects frame-pointer unwinding because the pattern go reaches suggested_unwinding as a command. If the native program was built without frame pointers, its call stacks will be incomplete. Every other target in the same recording also gets this setting.

Keep each target's argument list intact and distinguish its executable from ordinary arguments before inferring the runtime.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/executor/orchestrator.rs
Line: 153-157

Comment:
**Arguments select the wrong unwinding**

Every exec argument is checked as an executable name. For example, `codspeed exec -m walltime -- grep go corpus.txt` selects frame-pointer unwinding because the pattern `go` reaches `suggested_unwinding` as a command. If the native program was built without frame pointers, its call stacks will be incomplete. Every other target in the same recording also gets this setting.

Keep each target's argument list intact and distinguish its executable from ordinary arguments before inferring the runtime.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Claude Code Fix in Codex

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.

1 participant