Support python in profiling cli#235
Support python in profiling cli#235GuillaumeLagrange wants to merge 4 commits intocod-1736-use-correct-post-processing-depending-on-the-languagefrom
Conversation
Now that we have zstd compression, large file sizes should be less of an issue.
GuillaumeLagrange
changed the title
not-matthias
left a comment
not-matthias
left a comment
There was a problem hiding this comment.
We really need to make sure the stack unwinding doesn't cause regressions. The rest looks good 👍
| { | ||
| let perf_maps = pids | ||
| .iter() | ||
| .into_iter() |
There was a problem hiding this comment.
Dont forget to dedup the pids here, that's why we used HashSet
| where | ||
| I: IntoIterator<Item = libc::pid_t>, | ||
| { | ||
| for pid in pids { |
There was a problem hiding this comment.
same comment here, make sure we're iterating only over the dedup/unique pids. Otherwise we can easily run into issues where we duplicate work and/or overwrite files
| @@ -1,14 +1,13 @@ | |||
| use crate::prelude::*; | |||
| use std::collections::HashSet; | |||
| use std::fs; | |||
There was a problem hiding this comment.
Since this function is async, and the BenchmarkData::save_to function is now also async we could switch to tokio::fs instead of std::fs here.
| cmd.args(&benchmark_cmd.command[1..]); | ||
| // Use LD_PRELOAD to inject instrumentation into the child process | ||
| cmd.env("LD_PRELOAD", preload_lib_path); | ||
| cmd.env("PYTHONPERFSUPPORT", "1"); |
There was a problem hiding this comment.
nit: maybe add a quick comment that we need this so that python generates perf maps (technically it's in the commit message, but comments make it a bit more obvious)
| // Note that the higher the stack size, the larger the file, although it is mitigated | ||
| // by zstd compression | ||
| (UnwindingMode::Dwarf, Some(32 * 1024)) |
There was a problem hiding this comment.
Can you test this on pytest-codspeed and check the final report sizes? Last time when we tried it, we had some issues there.
It would also be useful to know when we could run into those limits since we're saving 4x more data now.
| Ok(()) | ||
| } | ||
|
|
||
| fn bail_if_command_spawned_subprocesses_under_valgrind(pid: u32) -> Result<()> { |
There was a problem hiding this comment.
can we have a comment here that explains a bit why and how this works (basically what we discussed in the DMs); don't spend too much time on it, just want to make sure we have an explanation when we come back to this code
2 participants
No description provided.