add early thread-state check and TaskBlock emission for wall-clock by kaahos · Pull Request #502 · DataDog/java-profiler

kaahos · 2026-04-24T12:31:29Z

What does this PR do?:
Improves wall-clock sampling by adding an early thread-state check so we avoid full sampling work when threads are in cheap-to-detect idle states (e.g. sleeping / condition-variable wait). Adds TLS-based park enter/exit handling so parked threads can be treated efficiently during wall-clock sampling. Introduces TaskBlock recording for blocked intervals. Centralizes TaskBlock eligibility, extends wall-clock counters and datadog.WallClockSamplingEpoch JFR fields for observability.

Motivation:
Wall-clock sampling should spend less work on threads that are idle or parked while still surfacing blocking time as TaskBlock events for analysis. This aligns native behavior with tracer instrumentation and improves visibility through counters and epoch metrics.

Additional Notes:

JVMTI MonitorWait / MonitorWaited callbacks are registered only for JDK < 21. For JDK21+, Java-level instrumentation is preferred.
linked PR: ddprof TaskBlock bridge and lock-support / object-wait instrumentations dd-trace-java#11247

How to test the change?:

./gradlew :ddprof-lib:gtestDebug_park_state_ut
./gradlew :ddprof-test:testDebug -Ptests=PrecheckTest
./gradlew :ddprof-test:testDebug -Ptests=PrecheckEfficiencyTest
./gradlew :ddprof-test:testDebug -Ptests=ParkTaskBlockTest
./gradlew :ddprof-test:testDebug -Ptests=WallclockMitigationsCombinedTest

For Datadog employees:

If this PR touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
This PR doesn't touch any of that.
JIRA: [JIRA-14534]

Unsure? Have a question? Request a review!

dd-octo-sts · 2026-04-24T13:06:39Z

CI Test Results

Run: #25802629146 | Commit: 8836c4e | Duration: 31m 44s (longest job)

✅ All 32 test jobs passed

Status Overview

JDK	glibc-aarch64/debug	glibc-amd64/debug	musl-aarch64/debug	musl-amd64/debug
8	-	✅	-	-
8-ibm	-	✅	-	-
8-j9	✅	✅	-	-
8-librca	-	-	✅	✅
8-orcl	-	✅	-	-
11	-	✅	-	-
11-j9	✅	✅	-	-
11-librca	-	-	✅	✅
17	✅	✅	-	-
17-graal	✅	✅	-	-
17-j9	✅	✅	-	-
17-librca	-	-	✅	✅
21	✅	✅	-	-
21-graal	✅	✅	-	-
21-librca	-	-	✅	✅
25	✅	✅	-	-
25-graal	✅	✅	-	-
25-librca	-	-	✅	✅

Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled

Summary: Total: 32 | Passed: 32 | Failed: 0

Updated: 2026-05-13 14:17:54 UTC

…reads

…heck for Thread.sleep tests

…ange

jbachorik · 2026-05-18T18:54:29Z

+  buf->putVar64(event->_ctx.spanId);
+  buf->putVar64(event->_ctx.rootSpanId);


We already have the generic way of capturing context. It would be better to use that instead.
It will also take care of the custom attribute, simplifying the code here.

jbachorik · 2026-05-18T18:55:44Z

+  Context context = ContextApi::snapshot();
+  context.spanId = (u64)spanId;
+  context.rootSpanId = (u64)rootSpanId;


The same - let's just use the generic way of getting and storing context in the events.

jbachorik · 2026-05-18T18:57:17Z

+    // Tracker settings are sticky across recordings. Preserve the historical
+    // table/config behavior, but allow HeapUsage recording to be enabled later
+    // (e.g. if an earlier test initialized liveness without ':L'). Once heap usage
+    // recording has been requested for this JVM lifetime, keep emitting HeapUsage
+    // even if a later recording omits the flag — avoids dropping JVM heap telemetry
+    // when liveness was turned on first without ':L'.
+    _record_heap_usage = _record_heap_usage || args._record_heap_usage;


This seems to be not really related (although valid) code change

jbachorik · 2026-05-18T19:00:06Z

+- **`datadog.WallClockSamplingEpoch`** (`T_WALLCLOCK_SAMPLE_EPOCH`) — timer aggregation snapshot;
+  appended numeric fields must remain backward-compatible for parsers that tolerate trailing varints.
+
+**JVMTI**: `MonitorWait` / `MonitorWaited` are registered only for `java_version() < 21`


For Java 21+ there are no such callbacks?

jbachorik · 2026-05-18T19:01:14Z

+## Wall-clock park TLS, TaskBlock, and OS-thread precheck
+
+### Park TLS (`ProfiledThread`)
+
+`parkEnter` / `parkExit` track Java-level `LockSupport` park intervals with span context
+snapshots for wall-clock collapsing and TaskBlock emission. The parked flag suppresses
+wall-clock signals on filtered threads when enabled.
+
+### `wallprecheck` (default `false`, opt-in)
+
+Disabled by default. When explicitly enabled with `wallprecheck=true` *and* thread filtering
+is on (`Arguments::_wall_precheck == true`), the wall-clock timer samples **HotSpot**
+`VMThread::osThreadState()` **before** sending `SIGVTALRM`. Slots must hold a non-null
+`VMThread*` from registration (`ThreadFilter::setVMThread`) for the precheck to run;
+otherwise only Java park-flag suppression applies.
+
+On JDK **21+**, `Thread.sleep` often maps to **CONDVAR_WAIT** (not only legacy **SLEEPING**),
+same broad category as condvar-based park — see `PrecheckTest` and `wallClock.cpp`.
+
+### JVMTI `MonitorWait` / TaskBlock (JDK before 21)
+
+JVMTI `MonitorWait` / `MonitorWaited` callbacks are registered only when
+`java_version() < 21` (`vmEntry.cpp`). They populate TaskBlock with a **blocker** value
+matching **`System.identityHashCode(waitedObject)`** (same convention as the JNI park path).


This does not seem to be the right place. This is about the TLS context implementation.

jbachorik · 2026-05-18T19:01:57Z

+        // wallprecheck=false: this test exercises thread filter + JFR thread identity, not sleep
+        // suppression (see PrecheckTest). With wallprecheck=true and VMThread set on the filter
+        // slot, a thread sleeping the whole window receives no wall signals.
+        return "wall=~1ms,filter=0,wallprecheck=false";


This should not be necessary - wallprecheck is false by default, right?

jbachorik · 2026-05-18T19:04:44Z


  public:
-    WallClockASGCT() : BaseWallClock(), _collapsing(false) {}
+    WallClockASGCT() : BaseWallClock(), _collapsing(false), _precheck(true) {}


Constructor default for _precheck is inverted

WallClockASGCT() initialises _precheck to true, but Arguments._wall_precheck defaults to false. If initialize(args) is ever skipped (or a caller constructs the engine without calling it), precheck is silently active, suppressing SIGVTALRM for sleeping/parked threads without user opt-in — contrary to the documented opt-in semantics.

Consider matching the Arguments default:

Suggested change

WallClockASGCT() : BaseWallClock(), _collapsing(false), _precheck(true) {}

WallClockASGCT() : BaseWallClock(), _collapsing(false), _precheck(false) {}

jbachorik · 2026-05-18T19:04:45Z

+            int slot_tid = slot.value.load(std::memory_order_acquire);
+            if (slot_tid != -1) {
+                VMThread* vm = slot.vm_thread.load(std::memory_order_acquire);
+                ProfiledThread* pt = slot.profiled_thread.load(std::memory_order_acquire);


profiled_thread is collected but never consumed

collectWithState performs an atomic acquire load of ProfiledThread* into every ThreadEntry, but no caller in the timer path (sampleThreads, wallClock.cpp) dereferences it. Carrying a raw pointer to a non-lifetime-guaranteed object through the sampling loop is a footgun — a future change that consumes entry.profiled_thread will race with ProfiledThread::release()/delete unless every site adds a lifetime guard.

Either remove the field from ThreadEntry (and the corresponding load) until there is a concrete consumer, or document the lifetime contract on the field declaration so future users add a null check before dereferencing.

jbachorik · 2026-05-18T19:04:46Z

+        : ParkedWallClockState::PARKED_SPANLESS;
+  }
+
+  inline void monitorWaitEnter(u64 start_ticks, const Context& context, u64 blocker) {


Misleading comment: _monitor_wait_active is not read from the wall-clock path

The comment claims the wall-clock path reads _monitor_wait_* fields, but the signal handler in wallClock.cpp never reads any of them. Both MonitorWait and MonitorWaited JVMTI callbacks fire on the affected (waited) thread itself, so there is no cross-thread access pattern here. Leaving the comment in place implies a memory-ordering constraint that doesn't exist and could mislead future contributors into adding unnecessary synchronisation.

Consider updating it to reflect the actual access pattern, e.g.:

Written and read only on the waited thread via JVMTI MonitorWait/MonitorWaited callbacks; no cross-thread access.

jbachorik · 2026-05-18T19:04:48Z


 void WallClockASGCT::initialize(Arguments& args) {
  _collapsing = args._wall_collapsing;
+  _precheck   = args._wall_precheck;


Initialization-ordering reliance is implicit

_precheck = args._wall_precheck correctly mirrors the requested setting, but the constructor's default of true (see wallClock.h) means there is a window between construction and initialize(args) where the field is enabled. Today this is benign because start() calls pthread_create() only after initialize() returns, but the contract is undocumented — any future code that runs the timer loop or invokes sampleThreads before initialize() will silently observe the wrong default.

No code change required if the existing initialize() → start() order is guaranteed, but a brief comment on initialize() ("must be called before start()/timer-loop entry; otherwise _precheck is unset") would lock the invariant in.

jbachorik · 2026-05-18T19:04:50Z

Nit: Either change _num_skipped_precheck_os to u64 to match putVar64, or use putVar32 consistent with the T_INT JFR metadata declaration.

jbachorik · 2026-05-18T19:05:31Z

    @Override
    protected String getProfilerCommand() {
-        return "wall=1ms";
+        return "wall=1ms,wallprecheck=false";


Not really necessary, I assume

jbachorik · 2026-05-18T19:07:48Z

+    // JVMThread::current() is the native thread self pointer. On OpenJ9/Zing it
+    // is not a HotSpot JavaThread*; only HotSpot may reinterpret it as VMThread*.
+    if (!VM::isHotspot() || JVMThread::current() == nullptr) {
+        return nullptr;
+    }


These changes are coincidental, right?

jbachorik · 2026-05-18T19:11:14Z

I did the first pass. I have some concerns about the basics - let's take it off-line and decide how to proceed. Until then you should not spend time on addressing my inline comments.

…an ids from JNI

…ibute on TaskBlock

…te::MONITOR_WAIT

… handler (once-per-run)

… blocked

…r/Entered

kaahos force-pushed the paul.fournillon/wallclock_precheck branch 6 times, most recently from da37425 to 06136e1 Compare April 27, 2026 08:55

feat(thread-filter): add VMThread* slot to ThreadFilter for OS-state …

323eed7

…reads

kaahos force-pushed the paul.fournillon/wallclock_precheck branch 3 times, most recently from 1e61333 to 4523b4a Compare April 28, 2026 15:45

feat(wall): register VMThread* in ThreadFilter at thread start and end

f80802b

kaahos force-pushed the paul.fournillon/wallclock_precheck branch 3 times, most recently from 0ad95a4 to 3e231a0 Compare April 28, 2026 21:51

kaahos added 2 commits April 29, 2026 11:18

test(wall): add PrecheckTest and PrecheckEfficiencyTest; disable prec…

10129d2

…heck for Thread.sleep tests

fix(liveness): update the _record_heap_usage flag if profiler args ch…

28856f4

…ange

kaahos force-pushed the paul.fournillon/wallclock_precheck branch from 3e231a0 to 28856f4 Compare April 29, 2026 09:19

kaahos added 4 commits April 29, 2026 16:22

feat(jfr): add TaskBlockEvent and datadog.TaskBlock metadata

ff11514

feat(jfr): record datadog.TaskBlock in FlightRecorder

0fd00b5

feat(thread): add park enter/exit state for wall-clock gating

51cd4e3

feat(wall): store ProfiledThread in thread filter for wall sampling

13fbd20

kaahos force-pushed the paul.fournillon/wallclock_precheck branch from d71b9e2 to 24fec34 Compare April 30, 2026 09:10

kaahos self-assigned this Apr 30, 2026

kaahos added 5 commits April 30, 2026 11:53

feat(wall): skip wall SIGVTALRM while thread is parked

05d9ac4

feat(api): add recordTaskBlock and park JNI entry points

dcc0810

fix(hotspot): guard VMThread access for non-HotSpot JVMs

cb97b77

test(thread): add gtests for ProfiledThread park state

430e979

test(wall): add TaskBlock and combined precheck/park coverage

81c7712

kaahos force-pushed the paul.fournillon/wallclock_precheck branch from 24fec34 to 81c7712 Compare April 30, 2026 09:54

fix: set wallprecheck default flag to false

a0dfd45

jbachorik reviewed May 18, 2026

View reviewed changes

kaahos added 15 commits May 19, 2026 09:42

fix: wallprecheck default value to false

4529440

fix: fix WallClockSamplingEpoch metadata

76d78dc

refactor(taskblock): capture context natively via ContextApi; drop sp…

14228b4

…an ids from JNI

feat(jfr): reorder datadog.TaskBlock fields to precede the context block

5189cf0

test(park): regression case for OTEP-snapshotted span and custom attr…

fe4df73

…ibute on TaskBlock

refactor(jvmti): rename MonitorWait dispatch to ObjectWait*

cc7bef4

docs: clarify JVMTI MonitorWait observes Object.wait, not OSThreadSta…

fea34f1

…te::MONITOR_WAIT

feat(wallclock): move wallprecheck filter from timer thread to signal…

b6c82ee

… handler (once-per-run)

fix: fix robustness and correctness of the tests

f157b86

fix: move once-per-run filter state to ThreadFilter::Slot

2b2ca17

fix: skip sending wall-clock signals to threads already sampled while…

a17586a

… blocked

feat: native synchronized-contention capture via MonitorContendedEnte…

9cbf303

…r/Entered

feat: delegate-monitor-events handshake for JDK 21+ agent integration

a45836e

test: fix wall-clock/TaskBlock test set

3316291

fix

8587c5e

		buf->putVar64(event->_ctx.spanId);
		buf->putVar64(event->_ctx.rootSpanId);

	WallClockASGCT() : BaseWallClock(), _collapsing(false), _precheck(true) {}
	WallClockASGCT() : BaseWallClock(), _collapsing(false), _precheck(false) {}

Conversation

kaahos commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dd-octo-sts Bot commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI Test Results

Status Overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbachorik commented May 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbachorik commented May 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kaahos commented Apr 24, 2026 •

edited

Loading

dd-octo-sts Bot commented Apr 24, 2026 •

edited

Loading