feat(profiler): wall-clock signal suppression for idle threads#560
feat(profiler): wall-clock signal suppression for idle threads#560kaahos wants to merge 29 commits into
Conversation
|
CI Test ResultsRun: #28039576970 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Failed Testsmusl-aarch64/debug / 25-librcaJob: View logs No detailed failure information available. Check the job logs. musl-aarch64/debug / 21-librcaJob: View logs No detailed failure information available. Check the job logs. musl-aarch64/debug / 11-librcaJob: View logs No detailed failure information available. Check the job logs. musl-aarch64/debug / 17-librcaJob: View logs No detailed failure information available. Check the job logs. musl-aarch64/debug / 8-librcaJob: View logs No detailed failure information available. Check the job logs. Summary: Total: 32 | Passed: 27 | Failed: 5 Updated: 2026-06-23 16:46:50 UTC |
3f4fe1e to
bcf405b
Compare
bcf405b to
b1cb73f
Compare
|
@copilot review this PR |
Reviewed the changes; no blocking issues found in the wall-clock precheck logic or tests. |
306aa49 to
8bb1fed
Compare
|
[Sphinx Review — HIGH · CONSENSUS]
Suggestion: Either (a) add the same once-per-run precheck logic to
|
|
[Sphinx Review — MEDIUM]
Suggestion: Either strip |
|
[Sphinx Review — LOW · CONSENSUS] The comment justifying the CAS RMW states Suggestion: Replace with: "CAS RMW to update only TYPE_MASK bits without clobbering FLAG_PARKED, which may be written concurrently by |
|
@copilot review this PR. |
|
most of the previous comments have been addressed @jbachorik! The branch now also includes the new features coming from main. |
rkennke
left a comment
There was a problem hiding this comment.
This is a nice improvement! I have some questions...
0940fbe to
3df9af6
Compare
rkennke
left a comment
There was a problem hiding this comment.
Two follow-up accuracy/design questions on the weighted-sampling logic (not blockers). Thanks for addressing all my earlier points thoroughly.
| result.unowned_weight_slot = slot; | ||
| result.unowned_weight = slot->consumeUnownedBlockedWeight(); | ||
| } else { | ||
| slot->resetUnownedBlockedSampling(); |
There was a problem hiding this comment.
Unowned path: the accumulated tail weight is discarded on a blocked→runnable transition.
shouldRecordUnownedBlockedSample() accumulates unowned_blocked_pending_weight for each suppressed signal, but that pending weight is only flushed into a real sample when the next record-decision fires (decision % kUnownedBlockedFallbackRatio == 1). When the thread goes runnable, this else branch calls resetUnownedBlockedSampling(), which zeroes the pending weight — so the suppressed intervals since the last recorded sample are dropped rather than attributed.
Concretely, at wall=1ms a ~9ms unowned block that ends before the 11th signal emits exactly one sample of weight 1, undercounting it ~9×. The "first sample of every run" guarantee still holds, but total blocked-time accounting is systematically under-reported for short/medium unowned blocks (the only production-active path, since precheck is opt-in).
Is this approximation intended? Would it be more accurate to flush the pending weight into the last sample (or emit a final weighted sample) on the runnable transition instead of discarding it?
There was a problem hiding this comment.
that's a good catch, thanks. I fixed this by preserving the last recorded unowned blocked stack and flushing the accumulated pending tail weight when the thread transitions back to runnable, instead of dropping it in resetUnownedBlockedSampling(). addressed in 5e484d0.
| if (slot->sampledThisRun() && | ||
| active_block_state == slot->lastSampledState()) { | ||
| incrementSuppressedSampledRun(); | ||
| result.suppress = true; |
There was a problem hiding this comment.
Owned path: no weight compensation for suppressed signals.
Unlike the unowned path, the owned-block branch emits one sample of weight 1 and then fully suppresses every subsequent signal for the run with no accumulated weight. So a long instrumented sleep/park is attributed only a single interval of wall time, and the rest of the blocked duration is lost from the wall-clock view — presumably to be recovered by PR2's datadog.TaskBlock event.
This is safe in this PR because the only callers of parkEnter/blockEnter are tests. Could you confirm the owned hooks won't be enabled on any production/tracer path before PR2 lands? Otherwise instrumented blocks would undercount wall time in the interim.
There was a problem hiding this comment.
I confirm, in this PR the owned hooks are not wired to production/tracer instrumentation; the only current callers are tests.
To make that harder to misuse before PR2, I made the Java wrappers package-private and moved test access behind a test-only ProfilerOwnedBlockHooks bridge. The native entry points remain private, and PR2 will be where production TaskBlock instrumentation is introduced. addressed in 3bfa844.
What does this PR do?:
Before sending
SIGVTALRM, the wall-clock timer checks whether the target thread is already in a skippable OS state and has already produced a sample during this blocking run. If so, the signal is suppressed. The first sample of every blocking run is always collected.Motivation:
High-frequency wall-clock sampling on threads spending most of their time idle generates many redundant samples of identical stacks at high CPU cost. The precheck lets us emit one representative sample per blocking run and skip the rest, recovering signal throughput for threads that are actually running.
This is the first of two PRs splitting
paul.fournillon/wallclock_precheck. This PR ships the suppression infrastructure. The follow-up PR addsdatadog.TaskBlockevent recording.Additional Notes:
How to test the change?:
New unit tests:
ddprof-lib/src/test/cpp/park_state_ut.cppddprof-lib/src/test/cpp/wallprecheck_args_ut.cppNew integration tests:
PrecheckTest: verifies suppression fires and reduces signal count.PrecheckEfficiencyTest: measures overhead reduction under sustained park.WallclockMitigationsCombinedTest: end-to-end combined mitigations scenario.Run locally with:
./gradlew :ddprof-test:test --tests '*Precheck*' --tests '*WallclockMitigations*' --tests '*WallclockTimer*'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!