Skip to content

[PROF-15268] POC: per-thread JFR-inflight counter (follow-up to #614) - #629

Draft
r1viollet wants to merge 3 commits into
mainfrom
r1viollet/per-thread-inflight-poc
Draft

[PROF-15268] POC: per-thread JFR-inflight counter (follow-up to #614)#629
r1viollet wants to merge 3 commits into
mainfrom
r1viollet/per-thread-inflight-poc

Conversation

@r1viollet

@r1violletr1viollet commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

POC follow-up to #614. Moves the SignalInflight counter from a single global atomic to per-thread storage on ProfiledThread, to see if the alternative discussed in PROF-15268 holds up.

How

  • ProfiledThread gets an _jfr_inflight field (owner-thread atomic RMW, cross-thread ACQUIRE read from drain) and an intrusive _registry_next pointer.
  • A spinlock-protected registry head lives on ProfiledThread. initCurrentThread / freeKey / current() insert and remove.
  • SignalInflight::enter/exit use the current ProfiledThread when available and fall back to the existing global counter otherwise.
  • SignalInflight::drain() iterates the registry summing per-thread counters plus the fallback.

What this fixes vs #614

  • Cache-line contention on the counter is gone on the fast path — each thread writes only its own memory.
  • Sets up the machinery to close the documented J9 longjmp leak: a follow-up commit can reset the current thread's counter in segvHandler/busHandler before chaining. Not done in this POC.

What this does not do

  • No segvHandler/busHandler hooks yet — the J9 longjmp leak is still open until that follow-up.
  • Fallback global counter kept for threads that fire signals before initCurrentThread runs.

Local testing

buildDebug, compileRelease --rerun-tasks, ShutdownTest, JavaProfilerTest, CollapsingSleepTest, SmokeWallTest (all cstack modes). All green.

For Datadog employees

  • This PR doesn't touch any of that.
  • JIRA: PROF-15268

POC follow-up to #614 evaluating whether moving the SignalInflight
counter from a global atomic to per-thread storage on ProfiledThread
is a viable alternative.
- ProfiledThread: adds _jfr_inflight (atomic RMW on owner-thread write,
ACQUIRE-read from drain), _registry_next intrusive pointer, and a
spinlock-protected registry head. initCurrentThread / freeKey /
current() insert / remove around the pthread_key lifecycle.
- SignalInflight: enter/exit prefer the current ProfiledThread's per-
thread counter; fall back to the existing global counter for threads
that fire signals before initCurrentThread runs. drain() iterates the
registry summing per-thread counters plus the fallback.
Cache-line contention on the counter is eliminated on the fast path.
The J9 longjmp leak documented in signalInflight.h is not yet closed
by this commit (needs segvHandler/busHandler hooks to reset the current
thread's slot before chaining) — left for a follow-up if the design
survives review.
Verified: buildDebug, compileRelease --rerun-tasks, ShutdownTest,
JavaProfilerTest, CollapsingSleepTest, SmokeWallTest (all cstack modes).
@dd-octo-sts

Copy link
Copy Markdown
Contributor

CI Test Results

Run:#28648085482 | Commit:d74b5e7 | Duration: 13m 42s (longest job)

All 32 test jobs passed

Status Overview

JDKglibc-aarch64/debugglibc-amd64/debugmusl-aarch64/debugmusl-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-07-03 08:39:21 UTC

@dd-octo-sts

Copy link
Copy Markdown
Contributor

Benchmark Results (commit c27d0bd)

Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/122534027 Commit: c27d0bdc7e863d2710ce3999d8e48d8ae372428f

⚠️ Significant outliers

  • 🟢 future-genetic (JDK 21): runtime -4.9% (2191→2084 ms)
Runtime details (per benchmark × JDK)
BenchmarkJDKLatestDevΔ (dev vs latest)Issues L/D
akka-uct21✅ 10314 ms (7 iters)✅ 10135 ms (7 iters)≈ -1.7% (±19.2%)— / —
akka-uct25✅ 8752 ms (8 iters)✅ 8932 ms (8 iters)≈ +2.1% (±20.4%)— / —
finagle-chirper21✅ 5973 ms (11 iters)✅ 5940 ms (11 iters)≈ -0.6% (±45.5%)⚠️ W:2 / ⚠️ W:1
finagle-chirper25✅ 5526 ms (12 iters)✅ 5411 ms (12 iters)≈ -2.1% (±42.4%)⚠️ W:1 / ⚠️ W:1
fj-kmeans21✅ 2834 ms (22 iters)✅ 2747 ms (23 iters)≈ -3.1% (±4.3%)— / —
fj-kmeans25✅ 2803 ms (22 iters)✅ 2833 ms (22 iters)≈ +1.1% (±4.5%)— / —
future-genetic21✅ 2191 ms (28 iters)✅ 2084 ms (30 iters)🟢 -4.9%— / —
future-genetic25✅ 2076 ms (30 iters)✅ 2140 ms (29 iters)≈ +3.1% (±4.4%)— / —
naive-bayes21✅ 1215 ms (47 iters)✅ 1298 ms (44 iters)≈ +6.8% (±58%)— / —
naive-bayes25✅ 1016 ms (56 iters)✅ 1016 ms (56 iters)≈ 0% (±53.9%)— / —
reactors21✅ 16142 ms (5 iters)✅ 16550 ms (5 iters)≈ +2.5% (±14.6%)— / —
reactors25✅ 19105 ms (5 iters)✅ 18537 ms (5 iters)≈ -3% (±7%)— / —
Internal counter details (ddprof)

ddprof internal counters, latest / dev (✅ = 0, · = unavailable):

BenchmarkJDKDropped recDropped jvmtiDropped traceSkipped WCAGCT failUnwind fail
akka-uct21✅ / ✅✅ / ✅✅ / ✅✅ / ✅✅ / ✅✅ / ✅
akka-uct25✅ / ✅✅ / ✅7 / 42297 / 2130✅ / ✅✅ / ✅
finagle-chirper21✅ / ✅✅ / ✅1 / ✅8310 / ✅✅ / ✅✅ / ✅
finagle-chirper25✅ / ✅✅ / ✅3 / 28690 / 8477✅ / ✅✅ / ✅
fj-kmeans21✅ / ✅✅ / ✅✅ / ✅✅ / ✅✅ / ✅✅ / ✅
fj-kmeans25✅ / ✅✅ / ✅2 / 81257 / 1274✅ / ✅✅ / ✅
future-genetic21✅ / ✅✅ / ✅✅ / ✅✅ / ✅✅ / ✅✅ / ✅
future-genetic25✅ / ✅✅ / ✅3 / 22879 / 2909✅ / ✅✅ / ✅
naive-bayes21✅ / ✅✅ / ✅✅ / ✅✅ / ✅✅ / ✅✅ / ✅
naive-bayes25✅ / ✅✅ / ✅4 / 33486 / 3453✅ / ✅✅ / ✅
reactors21✅ / ✅✅ / ✅✅ / ✅✅ / ✅✅ / ✅✅ / ✅
reactors25✅ / ✅✅ / ✅✅ / ✅1937 / 1886✅ / ✅✅ / ✅

@datadog-prod-us1-6

datadog-prod-us1-6Bot commented Jul 3, 2026

Copy link
Copy Markdown

Pipelines

Fix all issues with BitsAI

⚠️ Warnings

🚦 3 Pipeline jobs failed

DataDog/java-profiler | reliability-chaos-aarch64: [profiler+tracer, tcmalloc, 25.0.3-tem] View in DatadogGitLab

DataDog/java-profiler | reliability-chaos-aarch64: [profiler, tcmalloc, 21.0.3-tem] View in DatadogGitLab

DataDog/java-profiler | reliability-chaos-amd64: [profiler, tcmalloc, 21.0.3-tem] View in DatadogGitLab

Useful? React with 👍 / 👎

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 63a7ec5 | Docs | Datadog PR Page | Give us feedback!

@dd-octo-sts

dd-octo-stsBot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Reliability & Chaos Results

1 failure(s) detected Pipeline: https://gitlab.ddbuild.io/DataDog/java-profiler/-/pipelines/123834391

❌ chaos: profiler tcmalloc amd64 21 0 3 temXchaos
Chaos harness crashed (RC=124)

@jbachorik

Copy link
Copy Markdown
Collaborator

@copilot resolve the merge conflicts in this pull request

CopilotAI added 2 commits July 9, 2026 17:03
…from deleted thread.h to threadLocalData.h
PR #615 (origin/main) renamed thread.cpp to threadLocalData.cpp and deleted
thread.h, absorbing ProfiledThread into threadLocalData.h. Our branch had
modified thread.h to add per-thread JFR-inflight tracking.
Resolution:
- Delete thread.h (accept deletion from main)
- Port _registry_head/_registry_lock static members to threadLocalData.h
- Port _jfr_inflight/_registry_next instance members to threadLocalData.h
- Port enterJfrInflight/exitJfrInflight/jfrInflight methods to threadLocalData.h
- Port forEachRegistered/registryInsert/registryRemove declarations to threadLocalData.h
- Update signalInflight.cpp to include threadLocalData.h instead of thread.h
- threadLocalData.cpp already has implementation (correctly auto-merged)

CopilotAI commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

Merge conflicts resolved. The conflict arose because PR #615 (landed in origin/main) renamed thread.cppthreadLocalData.cpp and deleted thread.h, absorbing ProfiledThread into threadLocalData.h. Our branch had modified thread.h to add the per-thread JFR-inflight tracking machinery.

Resolution (commit fix: resolve merge conflict):

  • Deleted thread.h (accepting main's deletion)
  • Ported _registry_head/_registry_lock static members, _jfr_inflight/_registry_next instance members, and all the enterJfrInflight/exitJfrInflight/jfrInflight/forEachRegistered/registryInsert/registryRemove declarations into threadLocalData.h
  • Updated signalInflight.cpp to #include "threadLocalData.h" instead of "thread.h"
  • threadLocalData.cpp had already been correctly auto-merged with the registry implementation

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@r1viollet@jbachorik