FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[PROF-15268] POC: per-thread JFR-inflight counter (follow-up to #614) by r1viollet · Pull Request #629 · DataDog/java-profiler · GitHub

/ java-profiler Public

[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
ConversationCommits (3)ChecksFiles changed
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 commented Jul 3, 2026 •
edited by atlassian Bot
Loading

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

[PROF-15268] refactor(profiler): per-thread JFR-inflight counter (POC)
c27d0bd 
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 Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

CI Test Results

Run: #28648085482 | Commit: d74b5e7 | Duration: 13m 42s (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-07-03 08:39:21 UTC

dd-octo-sts Bot commented Jul 3, 2026

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)
Benchmark JDK Latest Dev Δ (dev vs latest) Issues L/D
akka-uct 21 ✅ 10314 ms (7 iters) ✅ 10135 ms (7 iters) ≈ -1.7% (±19.2%) — / —
akka-uct 25 ✅ 8752 ms (8 iters) ✅ 8932 ms (8 iters) ≈ +2.1% (±20.4%) — / —
finagle-chirper 21 ✅ 5973 ms (11 iters) ✅ 5940 ms (11 iters) ≈ -0.6% (±45.5%) ⚠️ W:2 / ⚠️ W:1
finagle-chirper 25 ✅ 5526 ms (12 iters) ✅ 5411 ms (12 iters) ≈ -2.1% (±42.4%) ⚠️ W:1 / ⚠️ W:1
fj-kmeans 21 ✅ 2834 ms (22 iters) ✅ 2747 ms (23 iters) ≈ -3.1% (±4.3%) — / —
fj-kmeans 25 ✅ 2803 ms (22 iters) ✅ 2833 ms (22 iters) ≈ +1.1% (±4.5%) — / —
future-genetic 21 ✅ 2191 ms (28 iters) ✅ 2084 ms (30 iters) 🟢 -4.9% — / —
future-genetic 25 ✅ 2076 ms (30 iters) ✅ 2140 ms (29 iters) ≈ +3.1% (±4.4%) — / —
naive-bayes 21 ✅ 1215 ms (47 iters) ✅ 1298 ms (44 iters) ≈ +6.8% (±58%) — / —
naive-bayes 25 ✅ 1016 ms (56 iters) ✅ 1016 ms (56 iters) ≈ 0% (±53.9%) — / —
reactors 21 ✅ 16142 ms (5 iters) ✅ 16550 ms (5 iters) ≈ +2.5% (±14.6%) — / —
reactors 25 ✅ 19105 ms (5 iters) ✅ 18537 ms (5 iters) ≈ -3% (±7%) — / —
Internal counter details (ddprof)

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

Benchmark JDK Dropped rec Dropped jvmti Dropped trace Skipped WC AGCT fail Unwind fail
akka-uct 21 ✅ / ✅ ✅ / ✅ ✅ / ✅ ✅ / ✅ ✅ / ✅ ✅ / ✅
akka-uct 25 ✅ / ✅ ✅ / ✅ 7 / 4 2297 / 2130 ✅ / ✅ ✅ / ✅
finagle-chirper 21 ✅ / ✅ ✅ / ✅ 1 / ✅ 8310 / ✅ ✅ / ✅ ✅ / ✅
finagle-chirper 25 ✅ / ✅ ✅ / ✅ 3 / 2 8690 / 8477 ✅ / ✅ ✅ / ✅
fj-kmeans 21 ✅ / ✅ ✅ / ✅ ✅ / ✅ ✅ / ✅ ✅ / ✅ ✅ / ✅
fj-kmeans 25 ✅ / ✅ ✅ / ✅ 2 / 8 1257 / 1274 ✅ / ✅ ✅ / ✅
future-genetic 21 ✅ / ✅ ✅ / ✅ ✅ / ✅ ✅ / ✅ ✅ / ✅ ✅ / ✅
future-genetic 25 ✅ / ✅ ✅ / ✅ 3 / 2 2879 / 2909 ✅ / ✅ ✅ / ✅
naive-bayes 21 ✅ / ✅ ✅ / ✅ ✅ / ✅ ✅ / ✅ ✅ / ✅ ✅ / ✅
naive-bayes 25 ✅ / ✅ ✅ / ✅ 4 / 3 3486 / 3453 ✅ / ✅ ✅ / ✅
reactors 21 ✅ / ✅ ✅ / ✅ ✅ / ✅ ✅ / ✅ ✅ / ✅ ✅ / ✅
reactors 25 ✅ / ✅ ✅ / ✅ ✅ / ✅ 1937 / 1886 ✅ / ✅ ✅ / ✅

datadog-prod-us1-6 Bot commented Jul 3, 2026 •
edited by datadog-official Bot
Loading

Copy link
Copy Markdown

✨ Fix all issues with BitsAI

⚠️ Warnings

🚦 3 Pipeline jobs failed

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

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

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

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 Bot commented Jul 3, 2026 •
edited
Loading

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 commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

@copilot resolve the merge conflicts in this pull request

Copilot AI added 2 commits July 9, 2026 17:03
chore: report progress - resolving merge conflicts
522fd64
fix: resolve merge conflict - port per-thread JFR-inflight additions …
63a7ec5 
…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)

Copilot AI 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.cpp → threadLocalData.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

Copilot AI requested a review from jbachorik July 9, 2026 17:06
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jbachorik Awaiting requested review from jbachorik

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL