Improve samply's stack unwinding, add support to dump node's inlined information#415
Conversation
Merging this PR will not alter performance
|
Greptile SummaryThis PR refactors how isolation (
Confidence Score: 5/5The 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: src/executor/wall_time/profiler/samply/mod.rs — the Important Files Changed
|
not-matthias
left a comment
There was a problem hiding this comment.
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> { |
There was a problem hiding this comment.
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.
| // 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); |
There was a problem hiding this comment.
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.
d140595 to
44904cf
Compare
44904cf to
be8229f
Compare
No description provided.