Restore priming - #713
Conversation
Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||
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>
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;
CI Test ResultsRun: #30897840382 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-08-04 10:11:40 UTC |
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>
This comment has been minimized.
This comment has been minimized.
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 f4e9eec · Any feedback? Reach out in #deveng-pr-agent |
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
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);
}
Benchmark Results (commit fceea70)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/128719860 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10321 ms (21 iters) | ✅ 10192 ms (21 iters) | ≈ -1.2% (±10.9%) | — / — |
| akka-uct | 25 | ✅ 8886 ms (24 iters) | ✅ 8832 ms (24 iters) | ≈ -0.6% (±9.6%) | — / — |
| finagle-chirper | 21 | ✅ 5957 ms (33 iters) | ✅ 5974 ms (33 iters) | ≈ +0.3% (±25.3%) | |
| finagle-chirper | 25 | ✅ 5519 ms (36 iters) | ✅ 5498 ms (36 iters) | ≈ -0.4% (±24.8%) | |
| fj-kmeans | 21 | ✅ 2786 ms (67 iters) | ✅ 2671 ms (69 iters) | 🟢 -4.1% | — / — |
| future-genetic | 25 | ✅ 2047 ms (91 iters) | ✅ 2011 ms (93 iters) | ≈ -1.8% (±2.6%) | — / — |
| naive-bayes | 21 | ✅ 1284 ms (134 iters) | ✅ 1311 ms (131 iters) | ≈ +2.1% (±32.7%) | — / — |
| naive-bayes | 25 | ✅ 1025 ms (167 iters) | ✅ 1020 ms (168 iters) | ≈ -0.5% (±31.3%) | — / — |
| reactors | 21 | ✅ 16732 ms (15 iters) | ✅ 16102 ms (15 iters) | ≈ -3.8% (±7.1%) | — / — |
| reactors | 25 | ✅ 18448 ms (15 iters) | ✅ 18579 ms (15 iters) | ≈ +0.7% (±4.5%) | — / — |
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 | ✅ / ✅ | ✅ / ✅ | 7 / 3 | 1976 / 1941 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 2 | 2265 / 2224 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 8312 / 8889 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 8627 / 8192 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 2 | 1269 / 1259 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 2 | 1274 / 1254 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 2948 / 2844 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 3 | 3523 / 3540 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 4 | 3486 / 3482 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 3 | 1673 / 1585 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 3 / ✅ | 1903 / 1821 | ✅ / ✅ | ✅ / ✅ |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
… into zgu/thread_priming
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:63
ThreadLocalDataPool::unclaimcurrently uses placement-new to reset the slot, which clears_misc_flags(includingFLAG_CLAIMED) via non-atomic stores and before decrementing_used. This can race withclaim_acquire()(undefined behavior due to atomic/non-atomic access to the same word) and can also causeclaim()to spuriously reject acquisitions when_usedis still at capacity even though a slot has been freed.
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.h:183
JVMSupport::initialize()needs to gate priming based on theProfiledThread::_current_threadTLS key index (the one used by_current_thread.set()inacquire_current()), butProfiledThreadcurrently doesn't expose that key. Adding a small accessor keepsThreadLocalencapsulated and allows the priming check to target the correct pthread key.
// be interrupted by signals
// This method is used for initializing ProfiledThread for known JNI entry points,
// in case that the Java threads were started before thread creation interceptor
// is fully initialized, so that ProfiledThreads were not setup for the threads.
static ProfiledThread* initCurrentThreadSignalSafe();
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (5)
ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:63
ThreadLocalDataPool::unclaimoverwrites an existingProfiledThreadwith placement-new without running the destructor.ProfiledThreadowns heap allocations viaUnwindFailures(see unwindStats.h), so this leaks memory each time a slot is released, and also makes the slot eligible for reuse while its internal state is being rewritten.
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
ThreadLocalDataPool::acquirereconstructsProfiledThreadwith placement-new. This constructor transitively allocates (e.g.,UnwindFailuresdoesnew[]), so callingProfiledThread::acquire_current()from signal contexts can reintroduce malloc/new in the signal handler path, defeating the purpose of priming and potentially deadlocking/crashing.
ProfiledThread* ThreadLocalDataPool::acquire(int tid) {
ThreadLocalDataPool* pool = __atomic_load_n(&_pool, __ATOMIC_ACQUIRE);
if (pool == nullptr) {
return nullptr;
} else {
ProfiledThread* t = pool->claim(tid);
if (t != nullptr) {
new (t)ProfiledThread(tid, true /* claimed */);
}
return t;
ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:27
- The pool pre-constructs
capacityProfiledThreadobjects. EachProfiledThreadconstructsUnwindFailures, which allocates two large heap arrays, soinitialize()will allocate on the order of tens of MB (e.g., ~64 * ~290KB just forUnwindFailures) and do a lot of zeroing work at startup.
This issue also appears on line 74 of the same file.
_threads = reinterpret_cast<ProfiledThread*>(p);
for (uint64_t index = 0; index < capacity; index++) {
new (&_threads[index]) ProfiledThread(0);
}
NativeMem::record(NM_THREAD_LOCAL, malloc_size);
ddprof-lib/src/main/cpp/threadLocalData.h:131
- The assert message in
unclaim()is misleading: it fires when the slot is not claimed, but the message reads as if it already is claimed.
inline void unclaim() {
assert(isClaimed() && "Slot has been claimed");
__atomic_fetch_and(&_misc_flags, ~FLAG_CLAIMED, __ATOMIC_RELEASE);
ddprof-lib/src/main/cpp/jvmSupport.cpp:44
- Priming/pooling is now enabled from
JVMSupport::initialize(), but there are no accompanying C++ unit tests asserting (a) thesupportPriming()gate behavior and (b) thatProfiledThread::acquire_current()stays allocation-free/signal-safe when priming is enabled. There are existing gtests in this module (e.g.,ddprof-lib/src/test/cpp/threadLocal_ut.cpp,signalSafety_ut.cpp) that could cover this.
if (ProfiledThread::supportPriming()) {
ThreadLocalDataPool::initialize();
}
Benchmark Results (commit 9a69529)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/128733319 Commit: ✅ Within expected boundariesNo significant runtime deltas (all within run-to-run noise) and no internal-counter outliers. Runtime details (per benchmark × JDK)
Internal counter details (ddprof)ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (5)
ddprof-lib/src/main/cpp/threadLocalData.h:192
ProfiledThread::current()/acquire_current()are declaredinlinein threadLocalData.h but their definitions live in a separate header (threadLocalData.inline.h). Many translation units in this repo still include only "threadLocalData.h" and callProfiledThread::current()(e.g., profiler.cpp, wallClock.cpp, javaApi.cpp). This split makes linkage/inlining brittle and can lead to undefined references depending on compiler/linker settings. Consider either (a) moving thecurrent()definition back into threadLocalData.h (it doesn’t depend on the pool), and keeping onlyacquire_current()in the inline header, or (b) providing a non-inline out-of-line definition in threadLocalData.cpp and removing the inline-only definition.
// 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.cpp:32
supportPriming()is called fromJVMSupport::initialize()beforeProfiledThread::isThreadKeyValid()is checked. The current implementation asserts the key is valid, which can trip in debug builds (and in release builds it can still read/compare an invalid key). Safer to treat an invalid key as “priming unsupported” and returnfalse.
bool ProfiledThread::supportPriming() {
// Key must be valid
assert(_current_thread.isKeyValid());
if (OS::isMusl()) {
return true;
} else {
return _current_thread.key() < PTHREAD_KEY_2NDLEVEL_SIZE;
}
}
ddprof-lib/src/main/cpp/threadLocalData.cpp:121
resetClaimed()doesn’t fully reinitialize the reusedProfiledThreadslot. In particular it leavesThreadLocalData::_unwinding_Javaunchanged and resets_filter_slot_idto0even though the constructor uses-1as the “unregistered” sentinel. This can leak per-thread state across slot reuse (e.g., permanently disable AsyncGetCallTrace for a future thread, or treat it as already registered in the thread filter).
void ProfiledThread::resetClaimed(int tid) {
_jmp_buf = nullptr;
_pc = 0;
_sp = 0;
_span_id = 0;
ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:54
ThreadLocalDataPool::claim()mixes signedintindices with_capacity(auint64_t).int start_pos = tid % _capacity;and the subsequent modulo arithmetic can produce implementation-defined results iftidis negative and also risks truncation if the pool capacity is ever increased. Using an unsigned index type avoids these issues and keeps the modulo math well-defined.
int start_pos = tid % _capacity;
int index = start_pos;
do {
if (_threads[index].claim_acquire()) {
return &_threads[index];
ddprof-lib/src/main/cpp/threadLocalDataPool.h:20
_capacityis a 64-bit value but_usedis a 16-bit counter (volatile uint16_t) that’s manipulated via__atomic_fetch_add. This makes the pool fragile if capacity is ever configured above 65535 (counter overflow) andvolatiledoesn’t add correctness for atomic operations. Consider usingstd::atomic<uint32_t>/std::atomic<uint64_t>(or at least a wider non-volatile integer) for_usedand aligning the types used in the atomic ops.
const uint64_t _capacity;
volatile uint16_t _used;
ProfiledThread* _threads;
Benchmark Results (commit 3b1f48c)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/128743773 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10402 ms (21 iters) | ✅ 10326 ms (21 iters) | ≈ -0.7% (±11%) | — / — |
| akka-uct | 25 | ✅ 8812 ms (24 iters) | ✅ 8893 ms (24 iters) | ≈ +0.9% (±10.3%) | — / — |
| finagle-chirper | 21 | ✅ 6006 ms (33 iters) | ✅ 5966 ms (33 iters) | ≈ -0.7% (±24.8%) | |
| finagle-chirper | 25 | ✅ 5462 ms (36 iters) | ✅ 5449 ms (36 iters) | ≈ -0.2% (±24.1%) | |
| fj-kmeans | 21 | ✅ 2734 ms (68 iters) | ✅ 2667 ms (69 iters) | ≈ -2.5% (±2.6%) | — / — |
| fj-kmeans | 25 | ✅ 2772 ms (67 iters) | ✅ 2815 ms (66 iters) | ≈ +1.6% (±2.7%) | — / — |
| future-genetic | 21 | ✅ 2035 ms (91 iters) | ✅ 2105 ms (88 iters) | 🔴 +3.4% | — / — |
| future-genetic | 25 | ✅ 2007 ms (93 iters) | ✅ 2092 ms (89 iters) | 🔴 +4.2% | — / — |
| naive-bayes | 25 | ✅ 978 ms (175 iters) | ✅ 977 ms (174 iters) | ≈ -0.1% (±31.7%) | — / — |
| reactors | 21 | ✅ 16274 ms (15 iters) | ✅ 15699 ms (16 iters) | ≈ -3.5% (±8.2%) | — / — |
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 / 4 | 2084 / 2002 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 2191 / 2309 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 5 / 3 | 8843 / 8350 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 7960 / 8069 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 1248 / 1246 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 1231 / 1281 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 4 | 2908 / 2939 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 2960 / 2887 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 5 | 3431 / 3469 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 1563 / 1544 | ✅ / ✅ | ✅ / ✅ |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.
Suppressed comments (5)
ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:20
- If the backing
mallocfails, the constructor returns without recording theThreadLocalDataPoolobject allocation inNativeMem(but the object itself was still allocated vianew). This skews NM_THREAD_LOCAL accounting on low-memory paths.
const size_t malloc_size = capacity * sizeof(ProfiledThread);
void* p = malloc(malloc_size);
if (p == nullptr) {
return;
}
ddprof-lib/src/main/cpp/threadLocalData.h:133
- The assert message in
unclaim()contradicts the condition: it asserts the slot is claimed, but the message reads as if that's the failure case. This makes debugging assertion failures confusing.
inline void unclaim() {
assert(isClaimed() && "Slot has been claimed");
__atomic_fetch_and(&_misc_flags, ~FLAG_CLAIMED, __ATOMIC_RELEASE);
ddprof-lib/src/main/cpp/threadLocalData.cpp:32
supportPriming()relies onassert(_current_thread.isKeyValid()), which is compiled out in release builds. On musl this currently returnstrueeven if the pthread key is invalid, and the glibc-specific key-index check is applied on non-Linux platforms as well. This can enable priming when the prerequisite (a valid key / expected pthread implementation behavior) is not met.
bool ProfiledThread::supportPriming() {
// Key must be valid
assert(_current_thread.isKeyValid());
if (OS::isMusl()) {
return true;
ddprof-lib/src/main/cpp/threadLocalData.cpp:113
resetClaimed()initializes_filter_slot_idto0, butProfiledThread’s constructor uses-1as the default/unset value. SinceThreadFilter::slotForId()treats negative IDs as invalid, resetting to0can incorrectly associate a reclaimed slot with thread-filter slot 0.
_misc_flags = FLAG_CLAIMED;
_park_block_token = 0;
_filter_slot_id = 0;
_init_window = 0;
ddprof-lib/src/main/cpp/threadLocalDataPool.cpp:36
ThreadLocalDataPoolrecords NM_THREAD_LOCAL in the constructor, but the destructor frees_threadswithout decrementing the correspondingNativeMemaccounting. If the pool is ever destroyed (tests, future shutdown path), NM_THREAD_LOCAL will be permanently inflated.
ThreadLocalDataPool::~ThreadLocalDataPool() {
if (_threads != nullptr) {
for (uint64_t index = 0; index < _capacity; index++) {
_threads[index].~ProfiledThread();
}
Benchmark Results (commit 01d4b42)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/128746444 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10291 ms (21 iters) | ✅ 10266 ms (21 iters) | ≈ -0.2% (±12%) | — / — |
| akka-uct | 25 | ✅ 8824 ms (24 iters) | ✅ 8867 ms (24 iters) | ≈ +0.5% (±10.1%) | — / — |
| finagle-chirper | 21 | ✅ 5915 ms (33 iters) | ✅ 5952 ms (33 iters) | ≈ +0.6% (±25.5%) | |
| finagle-chirper | 25 | ✅ 5465 ms (36 iters) | ✅ 5448 ms (36 iters) | ≈ -0.3% (±24.2%) | |
| fj-kmeans | 21 | ✅ 2704 ms (69 iters) | ✅ 2673 ms (69 iters) | ≈ -1.1% (±2.6%) | — / — |
| fj-kmeans | 25 | ✅ 2853 ms (66 iters) | ✅ 2785 ms (67 iters) | ≈ -2.4% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2071 ms (89 iters) | ✅ 2037 ms (90 iters) | ≈ -1.6% (±2.6%) | — / — |
| future-genetic | 25 | ✅ 2102 ms (89 iters) | ✅ 2018 ms (92 iters) | 🟢 -4% | — / — |
| naive-bayes | 21 | ✅ 1304 ms (131 iters) | ✅ 1317 ms (130 iters) | ≈ +1% (±32.7%) | — / — |
| naive-bayes | 25 | ✅ 1011 ms (170 iters) | ✅ 1007 ms (170 iters) | ≈ -0.4% (±31.4%) | — / — |
| reactors | 21 | ✅ 15853 ms (15 iters) | ✅ 15986 ms (16 iters) | ≈ +0.8% (±7.6%) | — / — |
| reactors | 25 | ✅ 18436 ms (15 iters) | ✅ 18803 ms (15 iters) | ≈ +2% (±4.1%) | — / — |
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 | 2048 / 1876 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 2283 / 2247 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 8 / 3 | 8368 / 8166 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 3 | 8164 / 8487 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 1 | 1279 / 1254 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 1286 / 1300 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 3010 / 2875 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 2915 / 2930 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 4 / 8 | 3486 / 3466 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 4 / 2 | 3507 / 3492 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 1572 / 1742 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 1824 / 1932 | ✅ / ✅ | ✅ / ✅ |
jbachorik
left a comment
jbachorik
left a comment
There was a problem hiding this comment.
🗿 🤖 🔴
Sphinx Review found 3 critical/high severity finding(s) that must be addressed.
| return ctx; | ||
| } | ||
|
|
||
| void ProfiledThread::resetClaimed(int tid) { |
There was a problem hiding this comment.
🗿 🤖 🔴
[Sphinx Review — HIGH] ProfiledThread::resetClaimed() resets most per-thread fields on pool-slot recycle but never resets _in_critical_section or _unwind_failures. If the previous slot occupant was interrupted mid-CriticalSection by a checkFault() siglongjmp (which bypasses the CriticalSection destructor, a pattern already documented elsewhere in this PR via SIGNAL_HANDLER_UNWIND_AFTER_LONGJMP), the flag can be left stuck at true; the next, unrelated thread that claims the recycled slot inherits it and can never successfully enter its own critical section (tryEnterCriticalSection()'s CAS never succeeds). _unwind_failures similarly carries stale counts into the new tenant.
Suggestion: Add _in_critical_section = false; and _unwind_failures.clear(); to resetClaimed() so a reused slot starts from the same defaults as a freshly-constructed ProfiledThread.
| _recording_epoch = 0; | ||
| _misc_flags = FLAG_CLAIMED; | ||
| _park_block_token = 0; | ||
| _filter_slot_id = 0; |
There was a problem hiding this comment.
🗿 🤖 🔴
[Sphinx Review — HIGH] ProfiledThread::resetClaimed() sets _filter_slot_id = 0 instead of the -1 "unassigned" sentinel used by the constructor and by Profiler::onThreadEnd(). Since ThreadFilter::slotForId() only rejects slot_id < 0 (slot 0 is a real, valid slot), a freshly pool-claimed ProfiledThread will report ownership of filter slot 0, causing WallClock::prepareWallPrecheck() and other filterSlotId() readers to inspect/mutate another thread's real filter/block-run state instead of treating this thread as unregistered.
Suggestion: Reset _filter_slot_id to -1 in resetClaimed(), matching the constructor default and the sentinel convention used by ThreadFilter::slotForId.
| return _current_thread.get(); | ||
| } | ||
|
|
||
| ProfiledThread* ProfiledThread::acquire_current() { |
There was a problem hiding this comment.
🗿 🤖 🔴
[Sphinx Review — HIGH] ProfiledThread::acquire_current() performs a non-atomic check-then-claim-then-publish sequence (current() -> ThreadLocalDataPool::acquire() -> _current_thread.set()) without blocking signals, unlike initCurrentThreadSignalSafe() which wraps the equivalent sequence in a SignalBlocker specifically because it "cannot be interrupted by signals". A different signal (e.g. SIGVTALRM interrupting a SIGPROF handler, since OS::installSignalHandler's sa_mask is empty) can run the same sequence in the gap between acquire() and set(), claim a second pool slot, and publish it first; when the outer handler resumes it unconditionally overwrites TLS with its own slot, permanently orphaning the inner handler's slot in the fixed-capacity (64) pool.
Suggestion: Re-check current() immediately before _current_thread.set() and, if a nested signal already published a slot, release the just-claimed slot back to the pool via ThreadLocalDataPool::release() instead of overwriting the TLS value; or wrap claim+publish in a SignalBlocker.
| return _current_thread.get(); | ||
| } | ||
|
|
||
| ProfiledThread* ProfiledThread::acquire_current() { |
There was a problem hiding this comment.
🗿 🤖 🔴
[Sphinx Review — MEDIUM] acquire_current() is defined in threadLocalData.inline.h, which every TU that previously only needed ProfiledThread::current() was switched to include instead of threadLocalData.h -- pulling in ThreadLocalDataPool.h as a transitive dependency for ~13 TUs that never call acquire_current() (only stackWalker.cpp and hotspotSupport.cpp do). This leaks an internal recycling-pool implementation detail into the include graph of every ProfiledThread consumer.
Suggestion: Keep ProfiledThread::current() available via threadLocalData.h alone (it doesn't need ThreadLocalDataPool), and only pull in threadLocalData.inline.h / threadLocalDataPool.h from the 2 TUs that actually call acquire_current().
| NativeMem::record(NM_THREAD_LOCAL, malloc_size + sizeof(ThreadLocalDataPool)); | ||
| } | ||
|
|
||
| ThreadLocalDataPool::~ThreadLocalDataPool() { |
There was a problem hiding this comment.
🗿 🤖 🔴
NIT
[Sphinx Review — LOW] ThreadLocalDataPool::~ThreadLocalDataPool() is never invoked anywhere in the codebase (no delete _pool/delete pool call site exists); the destructor is dead code given the singleton is intentionally never torn down.
Suggestion: No action needed if the singleton's process-lifetime intent is documented (see the sibling finding); otherwise consider removing the unused destructor or adding a comment noting it exists only for symmetry/testability.
| __atomic_fetch_and(&_misc_flags, ~FLAG_CLAIMED, __ATOMIC_RELEASE); | ||
| } | ||
|
|
||
| inline bool claim_acquire() { |
There was a problem hiding this comment.
🗿 🤖 🔴
[Sphinx Review — LOW] New methods claim_acquire() and acquire_current() use snake_case, breaking the camelCase naming convention used consistently elsewhere in ProfiledThread/ThreadLocalDataPool (initCurrentThread, currentTid, isThreadKeyValid, tryEnterCriticalSection, etc.).
Suggestion: Rename to claimAcquire() and acquireCurrent() to match the established camelCase convention in this class.
|
|
||
| // PTHREAD_KEY_2NDLEVEL_SIZE is an internal macro set to 32 in the GNU C Library (glibc) NPTL | ||
| // implementation. Slot indexes less than PTHREAD_KEY_2NDLEVEL_SIZE are pre-allocated. | ||
| static constexpr int PTHREAD_KEY_2NDLEVEL_SIZE = 32; |
There was a problem hiding this comment.
🗿 🤖 🔴
[Sphinx Review — MEDIUM] supportPriming() hardcodes glibc's private NPTL implementation constant PTHREAD_KEY_2NDLEVEL_SIZE=32 as the sole signal-safety proxy deciding whether TLS priming is safe, with no static_assert, runtime probe, or libc-version guard. A future glibc revision or a non-glibc/non-musl libc could silently flip the heuristic either way (unsafely enabling pthread_setspecific allocation from a signal handler, or unnecessarily disabling the safety net).
Suggestion: Add a compile-time or startup sanity check tying the constant to the actual glibc version in use, or gate it behind GLIBC and a version check so unrecognized libcs degrade safely (return false) instead of assuming glibc-compatible behavior.
| _wall_epoch = 0; | ||
| _call_trace_id = 0; | ||
| _recording_epoch = 0; | ||
| _misc_flags = FLAG_CLAIMED; |
There was a problem hiding this comment.
🗿 🤖 🔴
[Sphinx Review — MEDIUM] resetClaimed() writes _misc_flags = FLAG_CLAIMED; as a plain, non-atomic assignment, while every other access to this exact field (isClaimed/unclaim/claim_acquire) goes through _atomic* builtins with explicit ordering. This breaks the field's established atomic-only access contract; no current caller races on this instant, but it defeats TSan-clean reasoning about the field and is a latent hazard for any future concurrent reader added near slot-claim time.
Suggestion: Use __atomic_store_n(&_misc_flags, FLAG_CLAIMED, __ATOMIC_RELEASE); to match the memory-ordering discipline used elsewhere for this field.
| if (OS::isMusl()) { | ||
| return true; | ||
| } else { | ||
| return _current_thread.key() < PTHREAD_KEY_2NDLEVEL_SIZE; |
There was a problem hiding this comment.
🗿 🤖 🔴
[Sphinx Review — LOW] No test in this PR would detect return _current_thread.key() < PTHREAD_KEY_2NDLEVEL_SIZE; mutated to <= in ProfiledThread::supportPriming(). This would incorrectly return true for a pthread key at exactly 32 (the out-of-bounds boundary).
Suggestion: Add a test exercising supportPriming() with a mocked/forced key value at the boundary (31 should return true, 32 should return false on glibc systems).
| return false; | ||
| } | ||
|
|
||
| if (ProfiledThread::supportPriming()) { |
There was a problem hiding this comment.
🗿 🤖 🔴
[Sphinx Review — MEDIUM] ThreadLocalDataPool::initialize() only runs when supportPriming() heuristically approves (musl, or a glibc pthread-key index < 32 -- an artifact of key-allocation order unrelated to this profiler, e.g. more likely to be false on late/dynamic attach to an already-running JVM). Before this diff, StackWalker::walkFP/walkDwarf and HotspotSupport::walkVM/walkJavaStack tolerated ProfiledThread::current()==nullptr and still produced a (less-protected) stack sample; now they call acquire_current() and unconditionally drop the sample when it is null. On deployments where supportPriming() is false, the new safety net never engages, so native/signal-only threads that used to produce samples now have them silently dropped, folded into the same SAMPLES_DROPPED_THREAD_LOCAL counter as the pre-existing "no TLS" case.
Suggestion: Either initialize the pool unconditionally (sized/tuned for non-primed environments) or add a distinct counter for "sample dropped because priming unsupported" so this regression is observable in telemetry.
|
🗿 🤖 🔴 The following 3 findings apply to code/behavior outside this diff's hunks and can't be anchored as inline comments: [Sphinx Review — LOW] Suggestion: Document explicitly that these test helpers must only be used with [Sphinx Review — MEDIUM] No test in this PR exercises Suggestion: Add focused unit tests: (1) claim [Sphinx Review — MEDIUM] The " Suggestion: Factor the null-check-and-count-drop sequence into a small helper (e.g. a variant of |
What does this PR do?:
Motivation:
Additional Notes:
How to test the change?:
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!