Skip to content

feat(cli): add walltime unwinding mode and stack sample size options - #586

Open
not-matthias wants to merge 2 commits into
mainfrom
cod-3877-expose-stack-sampling-size-in-cli
Open

not-matthias wants to merge 2 commits into
mainfrom
cod-3877-expose-stack-sampling-size-in-cli

Conversation

@not-matthias

@not-matthias not-matthias commented Oct 13, 2026 •

Copy link
Copy Markdown
Member

Summary

  • Add --walltime-unwinding-mode / CODSPEED_WALLTIME_UNWINDING_MODE and --walltime-stack-sample-size / CODSPEED_WALLTIME_STACK_SAMPLE_SIZE.
  • Keep --perf-unwinding-mode / CODSPEED_PERF_UNWINDING_MODE as a hidden, deprecated alias. The new option wins when both are set.
  • perf: use the explicit stack size instead of the inferred one (default stays 8192 bytes).
  • samply (Linux): forward --unwinding-mode and --unwinding-stack-size.

Notes

@not-matthias
not-matthias marked this pull request as ready for review October 13, 2026 15:45
@codspeed

codspeed Bot commented Oct 13, 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-3877-expose-stack-sampling-size-in-cli (f77fdc8) with main (d14d95d)

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 Oct 13, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

[Medium impact] Renames profiler configuration options and adds stack sample size tuning.

The reviewed changes appear safe to merge once the author’s stated samply prerequisite is met; the warning and test feedback are non-blocking.

Fix All in Claude CodeFindings

  1. P2 Deprecation warning never appears ▶
  2. P2 Option rules lack tests ▶
Fix with agent prompt
### Issue 1
src/cli/shared.rs:258-260
Both `codspeed run` and `codspeed exec` call `resolve_unwinding_mode()` while building the config, before `Orchestrator::new()` installs the logger. This drops the warning, so users of `--perf-unwinding-mode` or `CODSPEED_PERF_UNWINDING_MODE` never learn about the replacement. Emit it after logger setup or use the existing startup-warning approach.

### Issue 2
src/cli/shared.rs:254-261
The new precedence and stack-size overrides have no tests. Add focused cases for the old option alone, both options together, flag/environment combinations, and an explicit size overriding both the default and Python's inferred size. These are the main promises of this change; tests would catch later edits that silently send the wrong value to the profiler.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

---

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

Summary

Adds walltime unwinding and stack-size options to run and exec.

  • Walltime profiler settings now choose unwinding mode and stack size.
  • The perf profiler now uses the requested DWARF stack size.
  • Linux samply runs now use the chosen unwinding settings.

Acknowledgment: not-matthias explicitly deferred bundled samply support and instructed that this PR wait for CodSpeedHQ/samply-codspeed#12. The known rejection of the new flags is not reported again.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A["run / exec flags and environment"] --> B["WalltimeRunArgs"]
  B --> C["resolve_unwinding_mode: new option wins"]
  B --> D["walltime_stack_sample_size"]
  C --> E["OrchestratorConfig"]
  D --> E
  E --> F["ExecutorConfig"]
  F --> G["perf: --call-graph"]
  F --> H["Linux samply: --unwinding-mode / --stack-size"]
Loading

Reviews (1) · Last reviewed commit: "feat(cli): add walltime unwinding mode a..." · Reviewed by Greptile

Comment thread src/cli/shared.rs Outdated
Comment thread src/cli/shared.rs Outdated

@GuillaumeLagrange GuillaumeLagrange left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

some few quirks to make sure we handle properly

Comment thread src/cli/shared.rs Outdated
Comment thread src/cli/shared.rs Outdated
Comment thread src/cli/shared.rs Outdated
Comment thread src/executor/config.rs Outdated
Comment thread src/executor/wall_time/profiler/samply/mod.rs Outdated
Comment thread src/executor/wall_time/profiler/perf/mod.rs Outdated
@not-matthias
not-matthias force-pushed the cod-3877-expose-stack-sampling-size-in-cli branch 6 times, most recently from 2b0a27a to e9dafa8 Compare October 29, 2026 16:27
Add --walltime-unwinding-mode (CODSPEED_WALLTIME_UNWINDING_MODE) to choose the call-stack unwinding for both walltime profilers, perf and samply. It accepts fp, dwarf, or dwarf,<bytes>, where <bytes> sets the DWARF stack sample size.

--perf-unwinding-mode / CODSPEED_PERF_UNWINDING_MODE stays as a hidden, deprecated alias with the same values so existing setups keep working; the new option wins when both are set.

perf: an explicit value replaces the unwinding inferred from the command, and a warning is logged when it differs from the suggested setting for Java, Go, or Python. samply: on Linux, forward the options as --unwinding-mode and --stack-size.
Bump samply-codspeed to the version that adds --unwinding-stack-size and pass the size under that name. Add an executor test that records with an explicit DWARF stack size through the bundled samply.
@not-matthias
not-matthias force-pushed the cod-3877-expose-stack-sampling-size-in-cli branch from e9dafa8 to f77fdc8 Compare October 29, 2026 16:30
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