Deduplicate walltime perf symbols, unwind data and debug info across pids #240
Conversation
GuillaumeLagrange
changed the title
GuillaumeLagrange
force-pushed
the
cod-2138-deduplicate-perf-maps-and-unwind_data
branch
from
99e0ea4 to
1a32faa
Compare
This fixes a regression introduced in 8b37208. We now filter pids using `bench_pids`, except for `exec-harness` integrations, where we take all pids.
- Store pid-agnostic data in a file or json map under the key
`{file_name}-{rest_of_path_hash}.{extension}` for each elf
- For each pid, store pid specific data, mostly the computed
load_bias from where each module was loaded into memory at
runtime, alongside a key to retrieve the pid-agnostic data
This way, we only write to disk relevant parts of the information.
GuillaumeLagrange
force-pushed
the
cod-2138-deduplicate-perf-maps-and-unwind_data
branch
from
ecc5dde to
784aa11
Compare
…refactor The use case for this is quite scoped, and it will most likely get removed later, but for now, it helps compute two consecutive runs, one with the current version of the runner, and one post refactor, and highlight the differences in the report data. It is expected to be used with ASLR disabled, otherwise all load_bias will be randomized across runs.
GuillaumeLagrange
force-pushed
the
cod-2138-deduplicate-perf-maps-and-unwind_data
branch
from
15316f4 to
9305a03
Compare
Also add a rebuild trigger to make it easier to run GITHUB_ACTIONS=1 cargo test` locally. We could have a better trigger, but this will do for now.
GuillaumeLagrange
force-pushed
the
cod-2138-deduplicate-perf-maps-and-unwind_data
branch
from
a3617af to
7f65690
Compare
not-matthias
left a comment
not-matthias
left a comment
There was a problem hiding this comment.
I think the struct naming can be clarified a bit, I was struggling quite a lot to understand how they map together. Maybe also using custom types for the key that's used in the hashmaps.
The overall logic looks good! Next round should be quick
|
|
||
| /// Per-pid debug info references, mapping PID to list of debug info index + load bias | ||
| #[serde(default, skip_serializing_if = "HashMap::is_empty")] | ||
| pub debug_info_pid_mappings_by_pid: HashMap<i32, Vec<DebugInfoPidMapping>>, |
There was a problem hiding this comment.
let's use libc::pid_t here
There was a problem hiding this comment.
Le'ts also do it for the two other fields below
| avma_range: 4194304..4558848, | ||
| base_avma: 4194304, |
There was a problem hiding this comment.
nit: can we have a custom Debug impl to always have hex values here?
|
|
||
| /// Per-pid mounting info for symbols, referencing the deduplicated symbols by path. | ||
| #[derive(Debug)] | ||
| pub struct SymbolPidMappingWithFullPath { |
There was a problem hiding this comment.
This is quite a long/complicated name 😅 Do we need the WithFullPath?
Maybe ModuleSymbols for the static structs that are shared, then maybe ProcessModuleSymbols for the in-process loaded symbols with the load bias + path?
Same for unwind data: We have static data which is shared across different processes (UnwindDataV3) and then specific data that depends on the process (just the load bias) which could be called ProcessUnwindData or LoadedUnwindData
| // NOTE: We still read the ELF file for every mapping even if we've already seen this path. | ||
| // The V3 data will be deduplicated below, but there's a possible optimization to skip | ||
| // the ELF read entirely when the path is already known, by caching the necessary data to | ||
| // compute load_bias. |
There was a problem hiding this comment.
side note: Since we're using memmap2 it might already be fast enough (except the copying of the DWARF data). but let's keep it in mind for the future
|
|
||
| let load_bias = elf_helper::compute_load_bias( | ||
| Ok(Self { | ||
| load_bias: 0, |
There was a problem hiding this comment.
wait, shouldn't we remove this then?
| @@ -0,0 +1,900 @@ | |||
| /// Compares walltime profiling output between old and new runner formats. | |||
There was a problem hiding this comment.
I'm not sure if it make sense to keep this, since we won't need it anymore after this PR has been merged, right?
| // Force a rebuild of the test target to be able to run the full test suite locally just by | ||
| // setting GITHUB_ACTIONS=1 in the environment. | ||
| // This is because `test_with` is evaluated at build time | ||
| println!("cargo::rerun-if-env-changed=GITHUB_ACTIONS"); |
There was a problem hiding this comment.
Do we really need this? I thought test_with has a runtime check
2 participants
Since the real deduplicator source of truth is actually the path of the elf file on disk, I settled on
it allows for the filename to be determined by the actual path, without having to either nest all the files, or have to sanitize paths to file name and end up in awkward file-name length limitations
Here's what it looks like on a 1000 iterations report of one simple program