Add unwinding validation and reporting #274
Conversation
# Conflicts: # ddprof-lib/src/main/cpp/stackFrame.h # gradle/lock.properties
Benchmarks [aarch64 wall]Parameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 18 metrics, 20 unstable metrics. |
Benchmarks [aarch64 memleak,alloc]Parameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 18 metrics, 20 unstable metrics. |
Benchmarks [aarch64 alloc]Parameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 18 metrics, 20 unstable metrics. |
Benchmarks [aarch64 cpu,wall,alloc,memleak]Parameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 18 metrics, 20 unstable metrics. |
Benchmarks [aarch64 cpu,wall]Parameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 17 metrics, 21 unstable metrics. |
Benchmarks [x86_64 cpu,wall]Parameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 14 metrics, 24 unstable metrics. |
Benchmarks [x86_64 cpu,wall,alloc,memleak]Parameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 15 metrics, 23 unstable metrics. |
Benchmarks [x86_64 memleak,alloc]Parameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 14 metrics, 24 unstable metrics. |
Benchmarks [x86_64 memleak]Parameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 14 metrics, 24 unstable metrics. |
ddprof-test/src/main/java/com/datadoghq/profiler/unwinding/UnwindingMetrics.java
Show resolved
Hide resolved
| TEXT, JSON, MARKDOWN | ||
| } | ||
| public enum Scenario { | ||
| C2_COMPILATION_TRIGGERS("C2CompilationTriggers"), |
There was a problem hiding this comment.
Could any of the scenarios interfere with the behavior of others, i.e. in the case where we run all of them?
There was a problem hiding this comment.
They should not. They are not run in parallel, and the only ones I would expect to be sensitive would be the JIT related ones. But we are JITing methods specific to that scenario, so we should be +- ok.
Of course, with this being dynamic system, it is always with a grain of salt and at least one hand waving, but so far the results look legit.
There was a problem hiding this comment.
Ok. This thread can act as a little footnote in case you do see interference-induced failures in the future.
ddprof-test/src/main/java/com/datadoghq/profiler/unwinding/UnwindingValidator.java
Outdated
Show resolved
Hide resolved
Benchmarks [x86_64 cpu]Parameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 16 metrics, 22 unstable metrics. |
Benchmarks [aarch64 cpu]Parameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 17 metrics, 21 unstable metrics. |
| const void *min_address = NO_MIN_ADDRESS, | ||
| const void *max_address = NO_MAX_ADDRESS, | ||
| const char* image_base = NULL); | ||
| const char* image_base = NULL, |
There was a problem hiding this comment.
Let's use nullptr instead.
There was a problem hiding this comment.
Let's address this in a followup PR.
This one is already pretty big and if I change NULL->nullptr I want to do it consistently and it will cause changes in 47 files 😱
I have the change ready but will post it as a separate PR, together with the update the to tooling such that spotless is enforcing nullptr instead of NULL
| @@ -1,180 +0,0 @@ | |||
| /* | |||
There was a problem hiding this comment.
Could you do git mv instead? so can spot the diffs easily.
There was a problem hiding this comment.
Next time, perhaps. Unbundling the commits and doing the manual git moves will take quite a lot of time for very little benefit in this case. The dwarf_dd.h is a wrapper around dwarf.h, meaning that I would force git to accept this as a move because the contents are very much not alike.
ddprof-lib/src/main/cpp/dwarf_dd.h
Outdated
| }; | ||
| } | ||
|
|
||
| // class DwarfParser { |
There was a problem hiding this comment.
Remove commented out code?
Benchmarks [x86_64 wall]Parameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 14 metrics, 24 unstable metrics. |
Benchmarks [x86_64 alloc]Parameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 14 metrics, 24 unstable metrics. |
Benchmarks [aarch64 memleak]Parameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 18 metrics, 20 unstable metrics. |
What does this PR do?:
This PR adds a bunch of scenarios that are quite challening for the vmstructs unwinder and measures the error rate based on various 'unknown()', 'break_' and 'invalid_' frames.
Motivation:
We want to have a way to see if our changes in the unwinding engine are actually making anything better or not.
Additional Notes:
During my work on this I had to bump the async-profiler base and then reworked the patching to be more 'sane'.
The validation is not blowing up tests (yet) but rather adds the metrics to the CI run report so one can get the glimpse on how each combination of os/arch/java version is doing.
The reports are also stored in the persisted reports so they can be downloaded.
How to test the change?:
For Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance.Unsure? Have a question? Request a review!