Skip to content

Commit b2dafa0

Browse files
authored
Fix review findings in the tracker and liveness engine
- Leak-tag pool: start() reset the free list while the preserved tracking table still had tagged entries (the table survives stop()/start()), so a new object could receive a tag another live object owns and a later double release could write past the free list. Reclaim owned tags first and keep their correlation info. - track() published a reserved slot before initializing it while shared-mode scanners (tagLeakInstances, getLiveTraceIds) could read the uninitialized malloc storage; slots now carry a release/acquire ready flag and fresh table regions start unpublished. - secondsToOOM projects BOTH the heap and the container boundary and takes the shorter time instead of picking a ring by the raw limit comparison - container usage includes native memory and siblings, so a container with a numerically larger limit can still be closer to exhaustion. - cleanup_table() claimed the GC epoch before acquiring the table lock, so a newer epoch's fold could enter the population history before an older one's; the claim now happens under the lock (the pre-lock check remains as an advisory early exit). - threadLoop's urgency ramp multiplied the budget by four on every rounded pause-target change and never restored it; the boost now applies once per urgency episode and the configured budget is restored when it ends. - The terminal restart gate now charges the finished search's accumulated safepoint cost BEFORE checking affordability, so an expensive search no longer earns one free immediate successor (restartSearch() no longer spends it itself; the pain-budget test asserts the new order). - hopLabelClassFor() deleted cls twice on the superclass-walk path. - The class-shape reconciliation loop never deleted the class-object local refs GetObjectsWithTags() returned (BFS thread - pins classes against unload). - walkStaticFieldAnchors() early breaks left later anchors' local refs undeleted; a cleanup pass now releases them. - The static-field sweep's truncation cursor resumed by a visited-count index that assumes HotSpot's LIFO FollowReferences order; it now redoes the chunk, which is order-independent (shared code must not rely on HotSpot internals). - buildDiscoveredInstanceChains() treated a cache hit from an earlier search generation as current; the generation check now mirrors the representative-refresh paths. - cacheResolvedChain() reports success so coverage accounting (found bits, resolved counts) only advances for a chain that was actually stored. - os_linux: container usage is now read from the same cgroup level that supplied the selected limit (an ancestor limit covers sibling cgroups whose usage the leaf excludes). Moves referenceChains_ut.cpp and livenessTracker_ut.cpp into this layer - they test exactly this code, and the pain-budget ordering change requires its test to land with it.
1 parent 6ee1015 commit b2dafa0

7 files changed

Lines changed: 8712 additions & 154 deletions

File tree

‎ddprof-lib/src/main/cpp/livenessTracker.cpp‎

Lines changed: 164 additions & 94 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,26 @@ bool ringThirdsStats(int head, int fill, int ring_size, int min_fill,
103103
return true;
104104
}
105105

106+
// Recent-half corroboration for a usage ring (see secondsToOOM()'s own
107+
// comment): a rising full-window trend whose most recent half is flat is a
108+
// plateaued step change, not ongoing growth. Returns true when the recent
109+
// half rises (or is too sparse to corroborate - the full-window trend then
110+
// stands alone, matching the single-ring version's have_recent_half
111+
// semantics of only rejecting on a confirmed flat recent half... inverted
112+
// here: false means "reject").
113+
template <typename Reader>
114+
bool corroborateRecentHalf(u8 head, u8 fill, int ring_size, int min_fill,
115+
Reader read) {
116+
int half_fill = fill / 2;
117+
RingThirdsStats recent_half_stats;
118+
bool have_half = ringThirdsStats(head, half_fill, ring_size, min_fill, read,
119+
&recent_half_stats);
120+
double half_delta = have_half
121+
? recent_half_stats.recent_mean - recent_half_stats.earliest_mean
122+
: 0.0;
123+
return have_half && half_delta > 0;
124+
}
125+
106126
} // namespace
107127

108128
void LivenessTracker::cleanup_table(bool forced, bool allow_resolve) {
@@ -120,22 +140,25 @@ void LivenessTracker::cleanup_table(bool forced, bool allow_resolve) {
120140
// overflow) sweep still folds one sample per genuinely new epoch instead
121141
// of either skipping it entirely or double-counting the same epoch across
122142
// repeated forced sweeps.
123-
bool is_epoch_owner = target_gc_epoch != current &&
124-
__atomic_compare_exchange_n(&_last_gc_epoch, &current, target_gc_epoch,
125-
false, __ATOMIC_RELAXED, __ATOMIC_RELAXED);
126-
127-
if (!is_epoch_owner && !forced) {
128-
// if the last processed GC epoch hasn't changed, or if we failed to update
129-
// it, there's nothing to do
130-
TEST_LOG_SUMMARY("LivenessTracker::cleanup_table early-exit: epoch unchanged and not forced");
131-
return;
132-
}
133-
134-
JNIEnv *env = VM::jni();
135-
136-
int epoch_diff = (int)(target_gc_epoch - current);
137-
138-
_table_lock.lock();
143+
//
144+
// The authoritative claim happens BELOW, under the table lock: a forced
145+
// sweep can claim a new epoch on one thread while a GC callback claims a
146+
// still-newer epoch on another, and claiming up front would let the newer
147+
// epoch's fold enter the population history before the older one's (lock
148+
// acquisition order is not claim order), folding the same epoch range
149+
// twice and skewing the trend. The check here stays as an advisory
150+
// early-exit so the common no-op call never takes the lock or the JNIEnv.
151+
u64 advisory_current = current;
152+
if (target_gc_epoch != advisory_current || forced) {
153+
JNIEnv *env = VM::jni();
154+
155+
_table_lock.lock();
156+
157+
u64 claimed = load(_last_gc_epoch);
158+
bool is_epoch_owner = target_gc_epoch != claimed &&
159+
__atomic_compare_exchange_n(&_last_gc_epoch, &claimed, target_gc_epoch,
160+
false, __ATOMIC_RELAXED, __ATOMIC_RELAXED);
161+
int epoch_diff = (int)(target_gc_epoch - claimed);
139162

140163
// Detect a class-map reset the same way
141164
// ReferenceChainTracker::resolveLoadedClasses() does (referenceChains.cpp)
@@ -260,6 +283,7 @@ void LivenessTracker::cleanup_table(bool forced, bool allow_resolve) {
260283
1.0f * (end - start) / 1000 / sz);
261284
}
262285
_table_lock.unlock();
286+
}
263287
}
264288

265289
u32 LivenessTracker::resolveKlassId(JNIEnv *env, jobject ref) {
@@ -431,6 +455,12 @@ int LivenessTracker::tagLeakInstances(jvmtiEnv *jvmti,
431455
_table_lock.lockShared();
432456
u32 sz = _table_size;
433457
for (u32 i = 0; i < sz; i++) {
458+
// Skip slots a concurrent track() reservation has not published yet
459+
// (see TrackingEntry::ready's own comment) - the lock is shared, so
460+
// the reservation is visible while its payload is still being filled.
461+
if (__atomic_load_n(&_table[i].ready, __ATOMIC_ACQUIRE) != 1) {
462+
continue;
463+
}
434464
if (_table[i].ref == nullptr) {
435465
continue;
436466
}
@@ -1340,101 +1370,94 @@ double LivenessTracker::secondsToOOM() const {
13401370
return -1;
13411371
}
13421372

1343-
// Project against whichever of the JVM heap or the container memory
1344-
// limit is tighter, rather than projecting both and comparing results -
1345-
// an unavailable container limit (bare metal, macOS, cgroups disabled) is
1346-
// treated as unbounded so it never wins this comparison. See this
1347-
// method's own comment (livenessTracker.h) for why the two are
1348-
// independent boundaries worth checking at all.
1349-
jlong effective_container_limit =
1350-
container_limit > 0 ? container_limit : std::numeric_limits<jlong>::max();
1351-
bool use_container = effective_container_limit < max_heap;
1352-
jlong limit = use_container ? container_limit : max_heap;
1353-
1373+
// Both boundaries are projected independently and the SHORTER time wins:
1374+
// picking a boundary by the raw limit comparison misses that container
1375+
// usage includes native memory, thread stacks, code cache and sibling
1376+
// cgroups, so a container whose limit is numerically LARGER than -Xmx can
1377+
// still be much closer to exhaustion than the heap itself (and vice
1378+
// versa). An unavailable container limit (bare metal, macOS, cgroups
1379+
// disabled) is treated as no boundary rather than unbounded. Both rings
1380+
// are filled by the same sampler, so they share one window; where a ring
1381+
// was never recorded (older recordings, tests that only pass used bytes)
1382+
// its projection simply does not fire.
13541383
u8 fill = loadAcquire(_heap_floor_ring_fill);
13551384
u8 head = loadAcquire(_heap_floor_ring_head);
1356-
TEST_LOG("LivenessTracker::secondsToOOM ring fill=%d head=%d source=%s limit=%lld",
1357-
(int)fill, (int)head, use_container ? "container" : "heap", (long long)limit);
1358-
1359-
RingThirdsStats byte_stats;
1360-
bool have_byte_stats = use_container
1361-
? ringThirdsStats(
1362-
head, fill, KLASS_POPULATION_RING_SIZE,
1363-
KLASS_POPULATION_MIN_FILL_FOR_TREND,
1364-
[this](int i) { return (double)load(_container_mem_ring[i]); },
1365-
&byte_stats)
1366-
: ringThirdsStats(
1367-
head, fill, KLASS_POPULATION_RING_SIZE,
1368-
KLASS_POPULATION_MIN_FILL_FOR_TREND,
1369-
[this](int i) { return (double)load(_heap_floor_ring[i]); },
1370-
&byte_stats);
1371-
if (!have_byte_stats) {
1385+
if (fill < KLASS_POPULATION_MIN_FILL_FOR_TREND) {
13721386
TEST_LOG("LivenessTracker::secondsToOOM -> -1 (INSUFFICIENT_FILL fill=%d need=%d)",
13731387
(int)fill, KLASS_POPULATION_MIN_FILL_FOR_TREND);
13741388
return -1;
13751389
}
13761390
RingThirdsStats time_stats;
1377-
// Same head/fill/min-fill gate as the byte call above, so this cannot
1378-
// actually fail once have_byte_stats passed - but the analyzer cannot
1379-
// prove that equivalence across the two readers, and reading time_stats
1380-
// uninitialized on the (impossible) failure path is exactly the
1381-
// "garbage or undefined" finding. Check the result.
13821391
if (!ringThirdsStats(
13831392
head, fill, KLASS_POPULATION_RING_SIZE,
13841393
KLASS_POPULATION_MIN_FILL_FOR_TREND,
13851394
[this](int i) { return (double)load(_heap_floor_time_ring[i]); },
13861395
&time_stats)) {
13871396
return -1;
13881397
}
1389-
1390-
double bytes_delta = byte_stats.recent_mean - byte_stats.earliest_mean;
13911398
double time_delta_ns = time_stats.recent_mean - time_stats.earliest_mean;
1392-
TEST_LOG("LivenessTracker::secondsToOOM bytes_delta=%.0f time_delta_ns=%.0f "
1393-
"earliest_mean=%.0f recent_mean=%.0f earliest_min=%.0f recent_min=%.0f",
1394-
bytes_delta, time_delta_ns,
1395-
byte_stats.earliest_mean, byte_stats.recent_mean,
1396-
byte_stats.earliest_min, byte_stats.recent_min);
1397-
if (bytes_delta <= 0 || time_delta_ns <= 0) {
1398-
TEST_LOG("LivenessTracker::secondsToOOM -> -1 (NOT_RISING bytes_delta=%.0f time_delta_ns=%.0f)",
1399-
bytes_delta, time_delta_ns);
1399+
if (time_delta_ns <= 0) {
1400+
TEST_LOG("LivenessTracker::secondsToOOM -> -1 (NOT_RISING time_delta_ns=%.0f)",
1401+
time_delta_ns);
14001402
return -1;
14011403
}
14021404

1403-
// Corroborate with a fit over just the most recent half of the window
1404-
// (HEAP_FLOOR_RECENT_HALF_MIN_FILL's own comment) - a one-time step
1405-
// change that has already plateaued still passes the full-window check
1406-
// above for as long as any of its samples remain in the window, but its
1407-
// own recent half is flat.
1408-
int half_fill = fill / 2;
1409-
RingThirdsStats recent_half_stats;
1410-
bool have_recent_half = use_container
1411-
? ringThirdsStats(
1412-
head, half_fill, KLASS_POPULATION_RING_SIZE,
1413-
HEAP_FLOOR_RECENT_HALF_MIN_FILL,
1414-
[this](int i) { return (double)load(_container_mem_ring[i]); },
1415-
&recent_half_stats)
1416-
: ringThirdsStats(
1417-
head, half_fill, KLASS_POPULATION_RING_SIZE,
1418-
HEAP_FLOOR_RECENT_HALF_MIN_FILL,
1419-
[this](int i) { return (double)load(_heap_floor_ring[i]); },
1420-
&recent_half_stats);
1421-
double recent_half_delta =
1422-
have_recent_half ? recent_half_stats.recent_mean - recent_half_stats.earliest_mean : 0;
1423-
if (!have_recent_half || recent_half_delta <= 0) {
1424-
TEST_LOG("LivenessTracker::secondsToOOM -> -1 (RECENT_HALF_FLAT "
1425-
"half_fill=%d have_recent_half=%d recent_half_delta=%.0f)",
1426-
half_fill, (int)have_recent_half, recent_half_delta);
1405+
double best_seconds = -1.0;
1406+
const char *best_source = "none";
1407+
double best_recent_mean = 0;
1408+
1409+
RingThirdsStats heap_bytes;
1410+
if (max_heap > 0 &&
1411+
ringThirdsStats(head, fill, KLASS_POPULATION_RING_SIZE,
1412+
KLASS_POPULATION_MIN_FILL_FOR_TREND,
1413+
[this](int i) { return (double)load(_heap_floor_ring[i]); },
1414+
&heap_bytes) &&
1415+
corroborateRecentHalf(head, fill, KLASS_POPULATION_RING_SIZE,
1416+
HEAP_FLOOR_RECENT_HALF_MIN_FILL,
1417+
[this](int i) { return (double)load(_heap_floor_ring[i]); })) {
1418+
double remaining = (double)max_heap - heap_bytes.recent_mean;
1419+
double secs = remaining <= 0
1420+
? 0
1421+
: (remaining * time_delta_ns) /
1422+
(heap_bytes.recent_mean - heap_bytes.earliest_mean) / 1e9;
1423+
if (best_seconds < 0 || secs < best_seconds) {
1424+
best_seconds = secs;
1425+
best_source = "heap";
1426+
best_recent_mean = heap_bytes.recent_mean;
1427+
}
1428+
}
1429+
1430+
RingThirdsStats container_bytes;
1431+
if (container_limit > 0 &&
1432+
ringThirdsStats(head, fill, KLASS_POPULATION_RING_SIZE,
1433+
KLASS_POPULATION_MIN_FILL_FOR_TREND,
1434+
[this](int i) { return (double)load(_container_mem_ring[i]); },
1435+
&container_bytes) &&
1436+
corroborateRecentHalf(head, fill, KLASS_POPULATION_RING_SIZE,
1437+
HEAP_FLOOR_RECENT_HALF_MIN_FILL,
1438+
[this](int i) { return (double)load(_container_mem_ring[i]); })) {
1439+
double remaining = (double)container_limit - container_bytes.recent_mean;
1440+
double secs = remaining <= 0
1441+
? 0
1442+
: (remaining * time_delta_ns) /
1443+
(container_bytes.recent_mean - container_bytes.earliest_mean) / 1e9;
1444+
if (best_seconds < 0 || secs < best_seconds) {
1445+
best_seconds = secs;
1446+
best_source = "container";
1447+
best_recent_mean = container_bytes.recent_mean;
1448+
}
1449+
}
1450+
1451+
if (best_seconds < 0) {
1452+
TEST_LOG("LivenessTracker::secondsToOOM -> -1 (no rising boundary "
1453+
"fill=%d heap=%lld container=%lld)",
1454+
(int)fill, (long long)max_heap, (long long)container_limit);
14271455
return -1;
14281456
}
1429-
1430-
double bytes_per_ns = bytes_delta / time_delta_ns;
1431-
double remaining_bytes = (double)limit - byte_stats.recent_mean;
1432-
if (remaining_bytes <= 0) {
1433-
// The chosen ring's own recent mean has already reached (or passed) its
1434-
// limit - exhaustion is not "in N seconds", it's now.
1435-
return 0;
1436-
}
1437-
return (remaining_bytes / bytes_per_ns) / 1e9; // ns -> seconds
1457+
TEST_LOG("LivenessTracker::secondsToOOM source=%s limit-projection=%.3fs "
1458+
"recent_mean=%.0f fill=%d",
1459+
best_source, best_seconds, best_recent_mean, (int)fill);
1460+
return best_seconds;
14381461
}
14391462

14401463
int LivenessTracker::selectLeakCandidates(KlassCandidate *out, int max) {
@@ -1786,13 +1809,37 @@ Error LivenessTracker::start(Arguments &args) {
17861809
if (err) {
17871810
return err;
17881811
}
1789-
// Initialize leak tag free list
1812+
// Initialize leak tag free list. The tracking table survives stop()/
1813+
// start() (see stop()'s own comment), and a preserved entry may still own
1814+
// a leak tag - so reclaim those first and build the free list from the
1815+
// remainder. Blindly marking every tag free would let a new object
1816+
// receive a tag another live object still owns (corrupting leak-tag
1817+
// correlation for both), and the eventual second releaseLeakTag() of the
1818+
// duplicate would push the same index twice and write past
1819+
// _leak_tag_free_list. Owned tags also keep their _leak_tag_info entries
1820+
// (erasing them would break getLeakTagInfo() correlation for the
1821+
// preserved, still-live owners).
1822+
bool tag_owned[LEAK_TAG_POOL_SIZE];
1823+
memset(tag_owned, 0, sizeof(tag_owned));
1824+
_table_lock.lock();
1825+
for (u32 i = 0; i < _table_size; i++) {
1826+
if (_table[i].leak_tag >= LEAK_TAG_BASE &&
1827+
_table[i].leak_tag < LEAK_TAG_BASE + LEAK_TAG_POOL_SIZE) {
1828+
tag_owned[_table[i].leak_tag - LEAK_TAG_BASE] = true;
1829+
}
1830+
}
1831+
_table_lock.unlock();
1832+
int free_w = 0;
17901833
for (int i = 0; i < LEAK_TAG_POOL_SIZE; i++) {
1791-
_leak_tag_free_list[i] = i;
1834+
if (tag_owned[i]) {
1835+
continue;
1836+
}
1837+
_leak_tag_free_list[free_w] = i;
17921838
_leak_tag_info[i].call_trace_id = 0;
17931839
_leak_tag_info[i].tid = 0;
1840+
free_w++;
17941841
}
1795-
_leak_tag_free_count = LEAK_TAG_POOL_SIZE;
1842+
_leak_tag_free_count = free_w;
17961843
if (!_enabled) {
17971844
// disabled
17981845
return Error::OK;
@@ -1920,6 +1967,11 @@ Error LivenessTracker::initialize(Arguments &args) {
19201967
_table = (TrackingEntry *)malloc(sizeof(TrackingEntry) * _table_cap);
19211968
if (_table != NULL) {
19221969
NativeMem::record(NM_LIVENESS, (long long)sizeof(TrackingEntry) * _table_cap);
1970+
// Uninitialized malloc storage must never look published to a
1971+
// shared-mode scanner (see TrackingEntry::ready's own comment).
1972+
for (int i = 0; i < _table_cap; i++) {
1973+
_table[i].ready = 0;
1974+
}
19231975
}
19241976

19251977
_gc_epoch = 0;
@@ -2096,6 +2148,10 @@ void LivenessTracker::track(JNIEnv *env, AllocEvent &event, jint tid,
20962148
!__sync_bool_compare_and_swap(&_table_size, idx, idx + 1));
20972149

20982150
if (idx < _table_cap) {
2151+
// Unpublish first: a previous entry at this index may still be visible
2152+
// to shared-mode scanners (their acquire load below then skips it
2153+
// instead of racing the re-fill).
2154+
__atomic_store_n(&_table[idx].ready, 0, __ATOMIC_RELEASE);
20992155
_table[idx].tid = tid;
21002156
_table[idx].time = TSC::ticks();
21012157
_table[idx].ref = ref;
@@ -2107,6 +2163,9 @@ void LivenessTracker::track(JNIEnv *env, AllocEvent &event, jint tid,
21072163
_table[idx].leak_tag = 0;
21082164
_table[idx].ctx = ContextApi::snapshot();
21092165
_table[idx].cached_klass_id = 0;
2166+
// Publish: the payload is complete - release pairs with the scanners'
2167+
// acquire loads.
2168+
__atomic_store_n(&_table[idx].ready, 1, __ATOMIC_RELEASE);
21102169
}
21112170

21122171
_table_lock.unlockShared();
@@ -2138,6 +2197,13 @@ void LivenessTracker::track(JNIEnv *env, AllocEvent &event, jint tid,
21382197
if (tmp != nullptr) {
21392198
NativeMem::record(NM_LIVENESS,
21402199
(long long)sizeof(TrackingEntry) * (newcap - _table_cap));
2200+
// Unpublish the uninitialized growth region (see
2201+
// TrackingEntry::ready's own comment); the realloc happens
2202+
// under the exclusive table lock, so no scanner can observe
2203+
// the interim.
2204+
for (int i = _table_cap; i < newcap; i++) {
2205+
tmp[i].ready = 0;
2206+
}
21412207
_table = tmp;
21422208
_table_cap = newcap;
21432209
Log::debug(
@@ -2265,6 +2331,10 @@ void LivenessTracker::getLiveTraceIds(CallTraceIdSet& out_buffer) {
22652331
// Collect call_trace_id values from all live tracking entries
22662332
for (int i = 0; i < _table_size; i++) {
22672333
TrackingEntry* entry = &_table[i];
2334+
// Skip unpublished slots (shared lock - see TrackingEntry::ready).
2335+
if (__atomic_load_n(&entry->ready, __ATOMIC_ACQUIRE) != 1) {
2336+
continue;
2337+
}
22682338
if (entry->ref != nullptr) {
22692339
out_buffer.insert(entry->call_trace_id);
22702340
}

‎ddprof-lib/src/main/cpp/livenessTracker.h‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,16 @@ typedef struct TrackingEntry {
4141
// stays valid for flush_table()'s later read of the same entry. track()
4242
// resets this to 0 for every newly tracked entry.
4343
u32 cached_klass_id;
44+
// Publication flag for the slot payload. track() holds only the shared
45+
// table lock while it reserves a slot via a _table_size CAS and then fills
46+
// it in, so a shared-mode scanner (tagLeakInstances(), getLiveTraceIds())
47+
// can observe the reserved index before the payload is written - the
48+
// table's malloc/realloc storage is uninitialized. track() stores 0 here
49+
// (release) before re-filling and 1 (release) after the payload is
50+
// complete; scanners load-acquire it and skip anything != 1. Freshly
51+
// malloc'd/realloc'd regions are zeroed at allocation time so a garbage
52+
// non-zero flag can never publish an uninitialized payload.
53+
volatile int ready;
4454
} TrackingEntry;
4555

4656
struct SubsampleRate {

0 commit comments

Comments
 (0)