From 7e267003b102687da2a7b5428b0d4893118d292e Mon Sep 17 00:00:00 2001 From: Paul Fournillon Date: Fri, 23 Oct 2026 14:23:18 +0200 Subject: [PATCH 1/3] Fix reference-chain revisits and leak candidate admission --- ddprof-lib/src/main/cpp/livenessTracker.cpp | 31 ++++-- .../src/main/cpp/referenceChainAnchors.cpp | 31 ++++-- .../src/main/cpp/referenceChainTraversal.cpp | 24 ++++- .../src/main/cpp/referenceChainWalk.cpp | 10 +- ddprof-lib/src/main/cpp/referenceChains.h | 24 +++-- .../src/test/cpp/livenessTracker_ut.cpp | 78 ++++++++++++++ .../src/test/cpp/referenceChainsBfsTests.inc | 100 ++++++++++++++++++ .../test/cpp/referenceChainsTestAccessors.h | 8 ++ 8 files changed, 274 insertions(+), 32 deletions(-) diff --git a/ddprof-lib/src/main/cpp/livenessTracker.cpp b/ddprof-lib/src/main/cpp/livenessTracker.cpp index 538854d9f6..f3b921dd95 100644 --- a/ddprof-lib/src/main/cpp/livenessTracker.cpp +++ b/ddprof-lib/src/main/cpp/livenessTracker.cpp @@ -882,15 +882,30 @@ void LivenessTracker::insertThreadGen(KlassCountScratch &scratch, return; } } + int idx; if (scratch.thread_count < KlassCountScratch::MAX_THREADS_PER_KLASS) { - int idx = scratch.thread_count++; - scratch.threads[idx].tid = tid; - scratch.threads[idx].ages[0] = age; - scratch.threads[idx].age_count = 1; - scratch.threads[idx].count = 1; // this object is the tid's first this epoch - } - // else: thread table full — additional threads are not tracked, but - // the oldest[] array still captures instances from all threads. + idx = scratch.thread_count++; + } else { + // Keeping only the first tids hides later, larger one-shot cohorts behind + // early small allocations. Replace the smallest observed retained set; + // ties prefer keeping the tid with more distinct surviving ages. + idx = 0; + for (int i = 1; i < scratch.thread_count; i++) { + const auto &candidate = scratch.threads[i]; + const auto &victim = scratch.threads[idx]; + if (candidate.count < victim.count || + (candidate.count == victim.count && candidate.age_count < victim.age_count)) { + idx = i; + } + } + } + // Count only objects observed since this tid entered the scratch. Inherit + // neither the evicted tid's count nor its ages: both feed qualification. + // Reappearing tids can be undercounted at capacity, never overcounted. + scratch.threads[idx].tid = tid; + scratch.threads[idx].ages[0] = age; + scratch.threads[idx].age_count = 1; + scratch.threads[idx].count = 1; } void LivenessTracker::accumulateKlassCount(u32 klass_id, jlong age, diff --git a/ddprof-lib/src/main/cpp/referenceChainAnchors.cpp b/ddprof-lib/src/main/cpp/referenceChainAnchors.cpp index f46158e822..5ceaf81321 100644 --- a/ddprof-lib/src/main/cpp/referenceChainAnchors.cpp +++ b/ddprof-lib/src/main/cpp/referenceChainAnchors.cpp @@ -148,11 +148,11 @@ ReferenceChainTracker::collectStaticFieldAnchorsForRotation(int max_count) { // fair pick vector here and is skipped at consumption time via fresh_kept_tags). for (size_t i = 0; i < idx_size; i++) { jlong tag = _static_anchor_index[i]; + jlong own_class_tag = i < _static_anchor_own_class_tags.size() + ? _static_anchor_own_class_tags[i] : 0; FrontierEntry entry{}; if (!frontier->lookupLocked(tag, &entry) || - entry.parent_tag != 0 || - (entry.root_kind != (u8)JVMTI_HEAP_REFERENCE_STATIC_FIELD && - entry.root_kind != (u8)JVMTI_HEAP_REFERENCE_JNI_GLOBAL) || + !isSelectableAnchor(entry, own_class_tag) || (entry.state != FrontierEntryState::FRONTIER && entry.state != FrontierEntryState::EXPANDED) || isQueuedForRotation(tag)) { @@ -201,15 +201,15 @@ ReferenceChainTracker::collectStaticFieldAnchorsForRotation(int max_count) { } pos = (size_t)(it - _static_anchor_index.begin()); } + jlong own_class_tag = pos < _static_anchor_own_class_tags.size() + ? _static_anchor_own_class_tags[pos] : 0; FrontierEntry entry{}; if (!frontier->lookupLocked(tag, &entry) || - entry.parent_tag != 0 || - (entry.root_kind != (u8)JVMTI_HEAP_REFERENCE_STATIC_FIELD && - entry.root_kind != (u8)JVMTI_HEAP_REFERENCE_JNI_GLOBAL) || + !isSelectableAnchor(entry, own_class_tag) || (entry.state != FrontierEntryState::FRONTIER && entry.state != FrontierEntryState::EXPANDED) || isQueuedForRotation(tag) || entry.leak_tag != 0) { - continue; // dead/demoted/queued/leak-tier: first look spent, not + continue; // dead/ineligible/queued/leak-tier: first look spent, not // fresh-kept (the leak tier selects it via the scan if it is leak-tagged) } bool keep = false; // container or not-yet-classified rides the @@ -333,6 +333,23 @@ void ReferenceChainTracker::addToStaticAnchorIndex(jlong tag, root_kind != (u8)JVMTI_HEAP_REFERENCE_JNI_GLOBAL) { return; } + indexAnchorForRotation(tag, own_class_tag); +} + +bool ReferenceChainTracker::isSelectableAnchor(const FrontierEntry &entry, + jlong own_class_tag) const { + if (entry.parent_tag == 0 && + (entry.root_kind == (u8)JVMTI_HEAP_REFERENCE_STATIC_FIELD || + entry.root_kind == (u8)JVMTI_HEAP_REFERENCE_JNI_GLOBAL)) { + return true; + } + auto shape = _class_shape_cache.find(own_class_tag); + return shape != _class_shape_cache.end() && + shape->second == (u8)AnchorClassShape::CONTAINER; +} + +void ReferenceChainTracker::indexAnchorForRotation(jlong tag, + jlong own_class_tag) { // Dedup: a push at first admission and another at upgrade would double-add. if (!_static_anchor_index_tags.insert(tag).second) { return; diff --git a/ddprof-lib/src/main/cpp/referenceChainTraversal.cpp b/ddprof-lib/src/main/cpp/referenceChainTraversal.cpp index ceb9fb1292..26c49231c9 100644 --- a/ddprof-lib/src/main/cpp/referenceChainTraversal.cpp +++ b/ddprof-lib/src/main/cpp/referenceChainTraversal.cpp @@ -135,6 +135,18 @@ jvmtiIterationControl JNICALL ReferenceChainTracker::heapRootCallback( default: break; } + // Remember local holders before expansion can reparent them into a deeper + // chain. Known collections still need bounded descend walks after that change + // to observe replacement backing storage. Classification runs outside this + // callback; unknown types are indexed but do not qualify for a walk yet. + if (isTransientRootKind(translated_root_kind) && class_tag != 0 && + *tag_ptr > 0 && !isLeakTag(*tag_ptr)) { + auto shape = ctx->tracker->_class_shape_cache.find(class_tag); + if (shape == ctx->tracker->_class_shape_cache.end() || + shape->second == (u8)AnchorClassShape::CONTAINER) { + ctx->tracker->indexAnchorForRotation(*tag_ptr, class_tag); + } + } return JVMTI_ITERATION_CONTINUE; } @@ -363,7 +375,12 @@ void ReferenceChainTracker::runPassManualWalk(jvmtiEnv *jvmti, JNIEnv *jni, std::vector static_anchor_fifo_drained; drainStaticAnchorFifo(STATIC_ANCHOR_FIFO_DRAIN, static_anchor_fifo_drained); for (const AtRiskAnchor &at_risk : static_anchor_fifo_drained) { - static_anchor_tags.push_back(at_risk.tag); + // A reparented collection can also have been selected by the persistent + // container tier. The combined selection is bounded by 48 + 16 entries. + if (std::find(static_anchor_tags.begin(), static_anchor_tags.end(), + at_risk.tag) == static_anchor_tags.end()) { + static_anchor_tags.push_back(at_risk.tag); + } } if (rotation_tags.empty() && leak_accumulation_tags.empty() && stale_expanded_tags.empty() && static_anchor_tags.empty()) { @@ -1044,6 +1061,11 @@ bool ReferenceChainTracker::runPass(jvmtiEnv *jvmti, JNIEnv *jni, if (manual_first_pass) { _search_started = true; store(_search_start_ns, OS::nanotime()); + // Register the candidates that triggered this search before walking. The + // first pass can drain the frontier; without a watched klass it would then + // complete and release its tags before the loop's post-pass poll can + // reconstruct any chain or schedule candidate-scoped walks. + pollWatchedTargets(jvmti, jni); } // Root/stack-ref enumeration alone never discovers a root's transitive children diff --git a/ddprof-lib/src/main/cpp/referenceChainWalk.cpp b/ddprof-lib/src/main/cpp/referenceChainWalk.cpp index 5acd946065..ec4500f626 100644 --- a/ddprof-lib/src/main/cpp/referenceChainWalk.cpp +++ b/ddprof-lib/src/main/cpp/referenceChainWalk.cpp @@ -446,9 +446,9 @@ jint JNICALL ReferenceChainTracker::heapReferenceCallback( // This new path is deeper - replace the shallow root-attached entry with the deeper // chain-attached entry. u32 referrer_klass = ctx->tracker->classTags()->resolve(class_tag); - // Pre-read the CURRENT shape: if improveChain() below succeeds, this root-attached durable - // entry is about to be replaced with a deeper chain-attached one - i.e. it is leaving the - // population collectStaticFieldAnchorsForRotation() can select, at exactly this moment. + // Pre-read the CURRENT shape: improveChain() can remove this entry's + // durable-root eligibility. A known collection remains in the container + // tier; other holders need the FIFO's first look below. FrontierEntry pre_improve_entry{}; bool was_root_attached_durable = ctx->frontier->lookup(*tag_ptr, &pre_improve_entry) && @@ -464,8 +464,8 @@ jint JNICALL ReferenceChainTracker::heapReferenceCallback( *tag_ptr, ctx->resolved_chain_tags, ctx->resolved_chain_tag_count); if (was_root_attached_durable) { - // Demotion push: the replaced entry's static/JNI-global attribution was its only - // anchor-tier eligibility, and it is gone now. + // Ensure a first look even when class-shape reconciliation has not + // yet classified this reparented holder as a collection. ctx->tracker->pushAtRiskStaticAnchor( *tag_ptr, ctx->tracker->classTags()->resolve(class_tag)); } diff --git a/ddprof-lib/src/main/cpp/referenceChains.h b/ddprof-lib/src/main/cpp/referenceChains.h index bfde3616ab..316fd4f905 100644 --- a/ddprof-lib/src/main/cpp/referenceChains.h +++ b/ddprof-lib/src/main/cpp/referenceChains.h @@ -434,10 +434,8 @@ class ReferenceChainTracker { } } _priority_expand_set; - // A static holder richly referenced from the running graph is EXCLUDED from the static-anchor tier forever once its - // frontier entry is chain-attached (first admitted via a non-root path, or demoted by - // improveChain) - maybeUpgradeRootAttachedRootKind() refuses entries with parent_tag != 0 by - // design, so collectStaticFieldAnchorsForRotation() (parent_tag == 0 filter) can never select it. + // Chain-attached holders without a known collection shape need a first + // descend walk when a static edge or reparenting removes their root eligibility. struct AtRiskAnchor { jlong tag; u32 klass_id; @@ -457,13 +455,14 @@ class ReferenceChainTracker { std::unordered_map _static_anchor_fifo_klass_counts; static constexpr u32 STATIC_ANCHOR_ATRISK_PER_KLASS_CAP = 64; - // Index of root-attached STATIC_FIELD/JNI_GLOBAL frontier entries, so - // collectStaticFieldAnchorsForRotation() iterates O(anchors) instead of scanning the full - // frontier table O(frontier_size). + // Index of durable roots and observed local holders. Known collections remain + // eligible after reparenting, so replacing their storage is still observed. + // Non-collection local holders are excluded after classification. + // collectStaticFieldAnchorsForRotation() iterates these anchors rather than + // scanning the full frontier table. std::vector _static_anchor_index; - // Parallel to _static_anchor_index: the OWN class tag of each anchor object (the class of the - // static field's VALUE, not the holder class). + // Parallel to _static_anchor_index: the runtime class tag of each indexed holder. std::vector _static_anchor_own_class_tags; // Dedup companion for _static_anchor_index (O(1) membership; the population is ~28k on a real JVM @@ -933,14 +932,17 @@ class ReferenceChainTracker { int *edges_admitted, bool *truncated, bool *frontier_cap_hit, u64 *safepoint_ns); - // Selects root-attached durable entries with a tiered cursor: leak-tagged, fresh, container-shaped, - // then everything else fairly. + // Selects durable roots and indexed collections (including reparented ones) + // with a tiered cursor: leak-tagged, fresh, containers, then other roots fairly. std::vector collectStaticFieldAnchorsForRotation(int max_count); + bool isSelectableAnchor(const FrontierEntry &entry, jlong own_class_tag) const; // At-risk anchor push: deduped, capped, and quota-limited per klass. void pushAtRiskStaticAnchor(jlong tag, u32 klass_id); // Add `tag` to _static_anchor_index if its root_kind is a durable anchor-tier kind (STATIC_FIELD // or JNI_GLOBAL). void addToStaticAnchorIndex(jlong tag, jlong own_class_tag, u8 root_kind); + // Shared deduplicated index insertion for durable roots and local holders. + void indexAnchorForRotation(jlong tag, jlong own_class_tag); // True iff `klass` implements java/util/Collection or java/util/Map, directly or transitively // (superclass chain + interfaces of every visited class, depth-bounded, visited set to survive diff --git a/ddprof-lib/src/test/cpp/livenessTracker_ut.cpp b/ddprof-lib/src/test/cpp/livenessTracker_ut.cpp index 5b6e10f90f..688961c772 100644 --- a/ddprof-lib/src/test/cpp/livenessTracker_ut.cpp +++ b/ddprof-lib/src/test/cpp/livenessTracker_ut.cpp @@ -1093,6 +1093,22 @@ TEST_F(SelectLeakCandidatesTest, TopKlassesByGenerationCountEmptyTableReturnsZer // Reaches the private state onGC()'s container-usage gate depends on. class LivenessTrackerTestAccessor { public: + // Exercise the production per-thread accumulator and fold without synthetic + // tid histories. Each pair describes a one-cohort allocating thread. + static void recordThreadCohorts( + LivenessTracker *t, int slot, + const std::vector> &cohorts) { + LivenessTracker::KlassCountScratch scratch{}; + for (const auto &cohort : cohorts) { + for (u32 i = 0; i < cohort.second; i++) { + t->insertThreadGen(scratch, cohort.first, /*age=*/1); + } + } + t->_table_lock.lock(); + t->recordTidTrendSamplesLocked(slot, scratch); + t->_table_lock.unlock(); + } + // Runs one onGC() with the given _container_limit_resolved value and returns the // container usage it recorded. Restores the touched fields afterwards. static u64 containerUsageRecordedByOnGC(LivenessTracker *t, bool limit_resolved) { @@ -1185,6 +1201,68 @@ class LivenessTrackerTestAccessor { } }; +TEST_F(SelectLeakCandidatesTest, LateAllocatingThreadCanQualifyAfterScratchFills) { + LivenessTracker *tracker = LivenessTracker::instance(); + std::vector> cohorts; + for (int tid = 1; tid <= 16; tid++) { + cohorts.emplace_back(tid, 1); + } + // The larger, later worker must not be permanently hidden behind the first + // 16 workers. All its objects share one GC age, as in the external cache test. + cohorts.emplace_back(99, 12); + for (int epoch = 1; epoch <= 20; epoch++) { + int slot; + bool created; + tracker->klassPopulationRecordForTest(1, epoch, epoch, &slot, &created); + LivenessTrackerTestAccessor::recordThreadCohorts(tracker, slot, cohorts); + } + KlassCandidate out[5]; + ASSERT_EQ(1, tracker->selectLeakCandidates(out, 5)); + ASSERT_EQ(1, out[0].qualifying_tid_count); + EXPECT_EQ(99, out[0].qualifying_tids[0]); +} + +TEST_F(SelectLeakCandidatesTest, ScratchOverflowDoesNotInventRetainedInstances) { + LivenessTracker *tracker = LivenessTracker::instance(); + std::vector> cohorts; + for (int tid = 1; tid <= 40; tid++) { + // Below the retained-count bar, with a flat one-age history: replacing + // another tid must never transfer its counts to the incoming worker. + cohorts.emplace_back(tid, 7); + } + for (int epoch = 1; epoch <= 20; epoch++) { + int slot; + bool created; + tracker->klassPopulationRecordForTest(1, epoch, epoch, &slot, &created); + LivenessTrackerTestAccessor::recordThreadCohorts(tracker, slot, cohorts); + } + KlassCandidate out[5]; + EXPECT_EQ(0, tracker->selectLeakCandidates(out, 5)); +} + +TEST_F(SelectLeakCandidatesTest, ScratchOverflowPreservesStrongEarlyThreads) { + LivenessTracker *tracker = LivenessTracker::instance(); + std::vector> cohorts; + for (int tid = 1; tid <= 16; tid++) { + cohorts.emplace_back(tid, 12); + } + for (int tid = 17; tid <= 40; tid++) { + cohorts.emplace_back(tid, 1); + } + for (int epoch = 1; epoch <= 20; epoch++) { + int slot; + bool created; + tracker->klassPopulationRecordForTest(1, epoch, epoch, &slot, &created); + LivenessTrackerTestAccessor::recordThreadCohorts(tracker, slot, cohorts); + } + KlassCandidate out[5]; + ASSERT_EQ(1, tracker->selectLeakCandidates(out, 5)); + ASSERT_GT(out[0].qualifying_tid_count, 0); + for (int i = 0; i < out[0].qualifying_tid_count; i++) { + EXPECT_LE(out[0].qualifying_tids[i], 16); + } +} + TEST(LivenessTrackerSessionTest, ResetSessionClearsOnlyRecordingGates) { LivenessTracker *tracker = LivenessTracker::instance(); LivenessTrackerTestAccessor::setSessionGatesForTest( diff --git a/ddprof-lib/src/test/cpp/referenceChainsBfsTests.inc b/ddprof-lib/src/test/cpp/referenceChainsBfsTests.inc index 6295e26850..5073fea572 100644 --- a/ddprof-lib/src/test/cpp/referenceChainsBfsTests.inc +++ b/ddprof-lib/src/test/cpp/referenceChainsBfsTests.inc @@ -5,6 +5,7 @@ #include "referenceChainsTestAccessors.h" #include "referenceChainsMockHeap.h" +#include "referenceChainInternal.h" #include "tsc.h" @@ -282,6 +283,105 @@ TEST_F(ReferenceChainsBfsTest, DiscoversObjectRetainedOnlyByStaticField) { tracker->stop(); } +// A qualifying population must be watched before the first walk can decide the +// search is complete. Polling only afterwards loses the frontier and tags. +TEST_F(ReferenceChainsBfsTest, FirstPassWatchesQualifiedLeakBeforeCompleting) { + Arguments args; + ASSERT_FALSE(args.parse("referencechains=true:budget=1000")); + ReferenceChainTracker *tracker = ReferenceChainTracker::instance(); + ASSERT_FALSE(tracker->start(args)); + LivenessTracker *liveness = LivenessTracker::instance(); + liveness->klassPopulationResetForTest(); + liveness->setGcGenerationsForTest(true); + liveness->classMapGenerationSetForTest( + Profiler::instance()->classMap()->generation()); + for (int epoch = 1; epoch <= 20; epoch++) { + int slot; + bool created; + liveness->klassPopulationRecordForTest(1, epoch, epoch, &slot, &created); + liveness->tidTrendRecordForTest(1, 4242, epoch, epoch); + } + // Production mints this from the sampled class; the gtest population seam + // has no live JVM from which to resolve it. + liveness->klassPopulationSetStableClassTagForTest(1, -41); + int root = addNode(); + script = {{JVMTI_HEAP_REFERENCE_JNI_GLOBAL, -1, root, -1}}; + + // Do not explicitly poll: this is the ordering exercised by the production + // first pass, including a walk that drains its entire ordinary queue. + EXPECT_TRUE(tracker->runPass(&mock_jvmti, &mock_jni)); + EXPECT_EQ(SearchState::RUNNING, tracker->searchState()); + EXPECT_EQ(1, ReferenceChainsTestAccessor::candidateCountForTest()); + EXPECT_NE(0, node_tags[root]); + + liveness->klassPopulationResetForTest(); + liveness->setGcGenerationsForTest(false); + tracker->stop(); +} + +TEST_F(ReferenceChainsBfsTest, RootContainerRemainsWalkableAfterReparentingAndMutation) { + Arguments args; + ASSERT_FALSE(args.parse("referencechains=true:hops=64:budget=1000")); + ReferenceChainTracker *tracker = ReferenceChainTracker::instance(); + ASSERT_FALSE(tracker->start(args)); + int holder = addNode(); + int parent = addNode(); + int table = addNode(); + int payload = addNode(); + node_tags[holder] = tracker->nextTag(); + node_tags[parent] = tracker->nextTag(); + const jlong container_class_tag = -101; + ReferenceChainsTestAccessor::primeClassShapeForTest(-102, false); + for (int node : {holder, parent}) { + ASSERT_TRUE(ReferenceChainsTestAccessor::insertFrontierEntry( + tracker->frontierTable(), node_tags[node], 0, 0, + FrontierEntryState::EXPANDED, JVMTI_HEAP_REFERENCE_STACK_LOCAL)); + } + ReferenceChainPassContext ctx{}; + ctx.tracker = tracker; + ctx.frontier = tracker->frontierTable(); + ctx.hop_cap = 64; + ctx.budget = 1000; + for (int repeat = 0; repeat < 2; repeat++) { + ReferenceChainsTestAccessor::reportHeapRootForTest( + JVMTI_HEAP_ROOT_STACK_LOCAL, container_class_tag, + &node_tags[holder], &ctx); + ReferenceChainsTestAccessor::reportHeapRootForTest( + JVMTI_HEAP_ROOT_STACK_LOCAL, -102, &node_tags[parent], &ctx); + } + // Classification runs outside the heap callback. Unknown transient roots + // must not get a descend walk before being confirmed as containers. + EXPECT_TRUE(ReferenceChainsTestAccessor::collectStaticFieldAnchorsForRotationForTest(4).empty()); + ReferenceChainsTestAccessor::primeClassShapeForTest(container_class_tag, true); + // A deeper retaining path replaces the initial local-root attribution. + // The container still needs repeated walks when its backing storage changes. + ASSERT_TRUE(tracker->frontierTable()->improveChain( + node_tags[holder], node_tags[parent], 0, 1, 0)); + ReferenceChainsTestAccessor::clearPriorityExpand(); + auto selected = ReferenceChainsTestAccessor::collectStaticFieldAnchorsForRotationForTest(4); + EXPECT_EQ(std::vector{node_tags[holder]}, selected); + int edges = 0; + ReferenceChainsTestAccessor::walkStaticFieldAnchorsForTest( + &mock_jvmti, &mock_jni, selected, 1000, &edges); + + // Mutate AFTER that first walk. No second root callback re-admits the + // holder, and the fresh-anchor lane has already consumed its first look. + const jlong leak_tag = ReferenceChainsTestAccessor::leakTagBase(); + node_tags[payload] = leak_tag; + script = { + {JVMTI_HEAP_REFERENCE_FIELD, holder, table, -1}, + {JVMTI_HEAP_REFERENCE_ARRAY_ELEMENT, table, payload, -1}, + }; + selected = ReferenceChainsTestAccessor::collectStaticFieldAnchorsForRotationForTest(4); + EXPECT_EQ(std::vector{node_tags[holder]}, selected); + ReferenceChainsTestAccessor::walkStaticFieldAnchorsForTest( + &mock_jvmti, &mock_jni, selected, 1000, &edges); + EXPECT_GT(tags_ever_assigned[table], 0); + EXPECT_GT(tags_ever_assigned[payload], 0); + EXPECT_NE(leak_tag, node_tags[payload]); + tracker->stop(); +} + // Regression test for the resolveLoadedClasses() scan-skip guard: it must compare `class_count != // _last_resolved_class_count`, not `class_count > _last_resolved_class_count`. TEST_F(ReferenceChainsBfsTest, ResolveLoadedClassesRescansAfterClassCountShrinksAndPartiallyRegrows) { diff --git a/ddprof-lib/src/test/cpp/referenceChainsTestAccessors.h b/ddprof-lib/src/test/cpp/referenceChainsTestAccessors.h index a7045e8582..b159e250bf 100644 --- a/ddprof-lib/src/test/cpp/referenceChainsTestAccessors.h +++ b/ddprof-lib/src/test/cpp/referenceChainsTestAccessors.h @@ -799,6 +799,14 @@ class ReferenceChainsTestAccessor { t->_priority_expand.end()); } + // Report a root without running a whole pass, so its scheduling effect can + // be checked independently of the ordinary rotation collectors. + static jvmtiIterationControl reportHeapRootForTest( + jvmtiHeapRootKind kind, jlong class_tag, jlong *tag, void *context) { + return ReferenceChainTracker::heapRootCallback(kind, class_tag, 0, tag, + context); + } + // Simulates a tag left in _priority_expand by a prior pass's truncated expandFrontier() // batch without a full expandFrontier()/JVMTI round-trip. static void pushPriorityExpand(jlong tag) { From 4112fbe0fb7543040d09d340fbef3456b29ce6d2 Mon Sep 17 00:00:00 2001 From: Paul Fournillon Date: Fri, 23 Oct 2026 15:57:02 +0200 Subject: [PATCH 2/3] Index durable holders and stabilize reference-chain test prerequisites --- ddprof-lib/src/main/cpp/javaApi.cpp | 3 +- ddprof-lib/src/main/cpp/livenessTracker.cpp | 13 +- ddprof-lib/src/main/cpp/livenessTracker.h | 15 ++- .../src/main/cpp/referenceChainTraversal.cpp | 6 + .../src/main/cpp/referenceChainWalk.cpp | 9 +- ddprof-lib/src/main/cpp/referenceChains.h | 2 +- .../src/test/cpp/livenessTracker_ut.cpp | 47 +++++++ .../test/cpp/referenceChainsAnchorTests.inc | 115 ++++++++++++++++++ .../test/cpp/referenceChainsTestAccessors.h | 8 ++ .../referencechains/LeakingCacheScenario.java | 18 ++- 10 files changed, 221 insertions(+), 15 deletions(-) diff --git a/ddprof-lib/src/main/cpp/javaApi.cpp b/ddprof-lib/src/main/cpp/javaApi.cpp index 59ab1ec452..79de11b68b 100644 --- a/ddprof-lib/src/main/cpp/javaApi.cpp +++ b/ddprof-lib/src/main/cpp/javaApi.cpp @@ -1131,7 +1131,8 @@ Java_com_datadoghq_profiler_JavaProfiler_seedKlassPopulationSample0( int evicted_count = 0; LivenessTracker::instance()->klassPopulationRecordForTest( (u32)klassId, (u32)count, (u64)epoch, &slot, &created, evicted, - &evicted_count, KlassPopulationEntry::MAX_REPRESENTATIVES_PER_KLASS); + &evicted_count, KlassPopulationEntry::MAX_REPRESENTATIVES_PER_KLASS, + /*synthetic_sample=*/true); for (int i = 0; i < evicted_count; i++) { if (evicted[i] != nullptr) { env->DeleteWeakGlobalRef(evicted[i]); diff --git a/ddprof-lib/src/main/cpp/livenessTracker.cpp b/ddprof-lib/src/main/cpp/livenessTracker.cpp index f3b921dd95..12db40e5fa 100644 --- a/ddprof-lib/src/main/cpp/livenessTracker.cpp +++ b/ddprof-lib/src/main/cpp/livenessTracker.cpp @@ -965,7 +965,8 @@ void LivenessTracker::accumulateKlassCount(u32 klass_id, jlong age, jweak LivenessTracker::recordKlassPopulationSampleLocked( u32 klass_id, u32 count, u64 epoch, int *out_slot, bool *out_created, - jweak *out_evicted, int *out_evicted_count, int max_evicted) { + jweak *out_evicted, int *out_evicted_count, int max_evicted, + bool synthetic_sample) { // Linear scan is fine: MAX_KLASS_POPULATION_ENTRIES is small enough that a // full scan is cheap, the same shape NativeSocketSampler's fd LRU // (NativeSocketSampler's fd cache) and this class's own cleanup_table() @@ -1014,6 +1015,7 @@ jweak LivenessTracker::recordKlassPopulationSampleLocked( _klass_population[slot].ring_head = 0; _klass_population[slot].ring_fill = 0; _klass_population[slot].consecutive_positive = 0; + _klass_population[slot].synthetic = false; _klass_population[slot].cached_slope = 0.0; // A reused (evicted) slot's previous class's per-tid trends must not // leak onto the new one, same as the fields above. @@ -1026,6 +1028,15 @@ jweak LivenessTracker::recordKlassPopulationSampleLocked( } KlassPopulationEntry &entry = _klass_population[slot]; + if (entry.synthetic && !synthetic_sample) { + // Real GC folds must not replace a JNI test fixture's seeded ramp with + // the sampled cohort's generation count. Keep its LRU epoch current. + entry.last_updated_epoch = epoch; + *out_slot = slot; + *out_created = created; + return evicted_ref; + } + entry.synthetic = entry.synthetic || synthetic_sample; entry.count_ring[entry.ring_head] = count; entry.ring_head = (u8)((entry.ring_head + 1) % KLASS_POPULATION_RING_SIZE); if (entry.ring_fill < KLASS_POPULATION_RING_SIZE) { diff --git a/ddprof-lib/src/main/cpp/livenessTracker.h b/ddprof-lib/src/main/cpp/livenessTracker.h index 20afe41418..24b45f0e89 100644 --- a/ddprof-lib/src/main/cpp/livenessTracker.h +++ b/ddprof-lib/src/main/cpp/livenessTracker.h @@ -107,6 +107,9 @@ typedef struct KlassPopulationEntry { // without this counter it gets reported as a leak candidate almost as // often as a real leak does. u8 consecutive_positive; + // Explicit JNI test seeds own their class trend, as synthetic TidTrend + // histories do. Ordinary allocation-derived histories leave this false. + bool synthetic; // Slope (regression value at the ring's newest sample minus the value at // its oldest sample - see ringWindowStats, livenessTracker.cpp) as of the // last push, computed and cached by hasQualifyingGrowth() alongside @@ -698,7 +701,8 @@ class alignas(alignof(SpinLock)) LivenessTracker { int *out_slot, bool *out_created, jweak *out_evicted = nullptr, int *out_evicted_count = nullptr, - int max_evicted = 0); + int max_evicted = 0, + bool synthetic_sample = false); // Drains _klass_count_scratch into _klass_population for the epoch that // just finished, minting a fresh representative jweak (from each entry's @@ -1260,18 +1264,23 @@ class alignas(alignof(SpinLock)) LivenessTracker { _klass_population_size--; } + // Only explicit JNI fixture seeds set synthetic_sample: real GC folds + // preserve their ramp. The default keeps ordinary algorithm-test histories + // subject to production updates and decay. jweak klassPopulationRecordForTest(u32 klass_id, u32 count, u64 epoch, int *out_slot, bool *out_created, jweak *out_evicted = nullptr, int *out_evicted_count = nullptr, - int max_evicted = 0) { + int max_evicted = 0, + bool synthetic_sample = false) { _table_lock.lock(); klass_id = resolveTestKlassAliasLocked(klass_id); jweak evicted = recordKlassPopulationSampleLocked(klass_id, count, epoch, out_slot, out_created, out_evicted, out_evicted_count, - max_evicted); + max_evicted, + synthetic_sample); _table_lock.unlock(); return evicted; } diff --git a/ddprof-lib/src/main/cpp/referenceChainTraversal.cpp b/ddprof-lib/src/main/cpp/referenceChainTraversal.cpp index 26c49231c9..8a22cb7432 100644 --- a/ddprof-lib/src/main/cpp/referenceChainTraversal.cpp +++ b/ddprof-lib/src/main/cpp/referenceChainTraversal.cpp @@ -135,6 +135,12 @@ jvmtiIterationControl JNICALL ReferenceChainTracker::heapRootCallback( default: break; } + // Record durable roots even when the holder already had a frontier entry: + // neither first admission nor a retained chain path may skip the index. + if (*tag_ptr > 0 && !isLeakTag(*tag_ptr)) { + ctx->tracker->addToStaticAnchorIndex(*tag_ptr, class_tag, + translated_root_kind); + } // Remember local holders before expansion can reparent them into a deeper // chain. Known collections still need bounded descend walks after that change // to observe replacement backing storage. Classification runs outside this diff --git a/ddprof-lib/src/main/cpp/referenceChainWalk.cpp b/ddprof-lib/src/main/cpp/referenceChainWalk.cpp index ec4500f626..c70d5284ef 100644 --- a/ddprof-lib/src/main/cpp/referenceChainWalk.cpp +++ b/ddprof-lib/src/main/cpp/referenceChainWalk.cpp @@ -491,13 +491,14 @@ jint JNICALL ReferenceChainTracker::heapReferenceCallback( *tag_ptr, ctx->resolved_chain_tags, ctx->resolved_chain_tag_count); } else if (reference_kind == JVMTI_HEAP_REFERENCE_STATIC_FIELD) { - // The upgrade refused (maybeUpgradeRootAttachedRootKind returns false for parent_tag != 0 - // by design), so this STATIC_FIELD edge just proved an at-risk static attachment the anchor - // tier's parent_tag == 0 filter can never see: a holder already admitted as a non-root - // child (find-anchor-holder-eviction). + // The sweep reached a holder first admitted through another object. + // Keep its static attachment indexed: confirmed collections must remain + // eligible after the FIFO's one-time descend walk has been consumed. FrontierEntry entry{}; if (ctx->frontier->lookup(*tag_ptr, &entry) && entry.parent_tag != 0) { + ctx->tracker->addToStaticAnchorIndex( + *tag_ptr, class_tag, (u8)reference_kind); ctx->tracker->pushAtRiskStaticAnchor( *tag_ptr, ctx->tracker->classTags()->resolve(class_tag)); } diff --git a/ddprof-lib/src/main/cpp/referenceChains.h b/ddprof-lib/src/main/cpp/referenceChains.h index 316fd4f905..cc7bc88d5e 100644 --- a/ddprof-lib/src/main/cpp/referenceChains.h +++ b/ddprof-lib/src/main/cpp/referenceChains.h @@ -455,7 +455,7 @@ class ReferenceChainTracker { std::unordered_map _static_anchor_fifo_klass_counts; static constexpr u32 STATIC_ANCHOR_ATRISK_PER_KLASS_CAP = 64; - // Index of durable roots and observed local holders. Known collections remain + // Index of holders observed at durable root edges or local roots. Known collections remain // eligible after reparenting, so replacing their storage is still observed. // Non-collection local holders are excluded after classification. // collectStaticFieldAnchorsForRotation() iterates these anchors rather than diff --git a/ddprof-lib/src/test/cpp/livenessTracker_ut.cpp b/ddprof-lib/src/test/cpp/livenessTracker_ut.cpp index 688961c772..c74d22f1fa 100644 --- a/ddprof-lib/src/test/cpp/livenessTracker_ut.cpp +++ b/ddprof-lib/src/test/cpp/livenessTracker_ut.cpp @@ -1093,6 +1093,19 @@ TEST_F(SelectLeakCandidatesTest, TopKlassesByGenerationCountEmptyTableReturnsZer // Reaches the private state onGC()'s container-usage gate depends on. class LivenessTrackerTestAccessor { public: + // Fold one surviving cohort through the same path as a real GC, excluding + // representative resolution because these tests have no live JVM. + static void foldClassCohort(LivenessTracker *t, u32 klass_id, jint tid, + u64 epoch) { + t->_table_lock.lock(); + t->_klass_count_scratch_size = 0; + for (int i = 0; i < 12; i++) { + t->accumulateKlassCount(klass_id, epoch, nullptr, tid); + } + t->foldKlassCountsLocked(nullptr, epoch, false); + t->_table_lock.unlock(); + } + // Exercise the production per-thread accumulator and fold without synthetic // tid histories. Each pair describes a one-cohort allocating thread. static void recordThreadCohorts( @@ -1201,6 +1214,40 @@ class LivenessTrackerTestAccessor { } }; +TEST_F(SelectLeakCandidatesTest, SyntheticClassHistorySurvivesRealGcFolds) { + LivenessTracker *tracker = LivenessTracker::instance(); + for (int epoch = 1; epoch <= 20; epoch++) { + int slot; + bool created; + tracker->klassPopulationRecordForTest( + 1, epoch * 10, epoch, &slot, &created, nullptr, nullptr, 0, + /*synthetic_sample=*/true); + tracker->tidTrendRecordForTest(1, 7, epoch * 3, epoch); + } + KlassCandidate out[5]; + ASSERT_EQ(1, tracker->selectLeakCandidates(out, 5)); + for (int epoch = 21; epoch <= 30; epoch++) { + LivenessTrackerTestAccessor::foldClassCohort(tracker, 1, 7, epoch); + } + EXPECT_EQ(1, tracker->selectLeakCandidates(out, 5)); +} + +TEST_F(SelectLeakCandidatesTest, OrdinaryClassHistoryStillDecaysAfterGrowthStops) { + LivenessTracker *tracker = LivenessTracker::instance(); + for (int epoch = 1; epoch <= 20; epoch++) { + int slot; + bool created; + tracker->klassPopulationRecordForTest(1, epoch * 10, epoch, &slot, &created); + tracker->tidTrendRecordForTest(1, 7, epoch * 3, epoch); + } + KlassCandidate out[5]; + ASSERT_EQ(1, tracker->selectLeakCandidates(out, 5)); + for (int epoch = 21; epoch <= 30; epoch++) { + LivenessTrackerTestAccessor::foldClassCohort(tracker, 1, 7, epoch); + } + EXPECT_EQ(0, tracker->selectLeakCandidates(out, 5)); +} + TEST_F(SelectLeakCandidatesTest, LateAllocatingThreadCanQualifyAfterScratchFills) { LivenessTracker *tracker = LivenessTracker::instance(); std::vector> cohorts; diff --git a/ddprof-lib/src/test/cpp/referenceChainsAnchorTests.inc b/ddprof-lib/src/test/cpp/referenceChainsAnchorTests.inc index d39be8edff..d2494b57bd 100644 --- a/ddprof-lib/src/test/cpp/referenceChainsAnchorTests.inc +++ b/ddprof-lib/src/test/cpp/referenceChainsAnchorTests.inc @@ -467,6 +467,121 @@ TEST_F(ReferenceChainsBfsTest, SweepPushFiresOnStaticEdgeOntoChainAttachedHolder tracker->stop(); } +TEST_F(ReferenceChainsBfsTest, StaticFieldCollectionRemainsIndexedAfterFirstLook) { + Arguments args; + ASSERT_FALSE(args.parse("referencechains=true:hops=64:budget=1000")); + ReferenceChainTracker *tracker = ReferenceChainTracker::instance(); + ASSERT_FALSE(tracker->start(args)); + int parent = addNode(); + int holder = addNode(); + int table = addNode(); + int payload = addNode(); + node_tags[parent] = tracker->nextTag(); + node_tags[holder] = tracker->nextTag(); + FrontierTable *frontier = tracker->frontierTable(); + ASSERT_TRUE(ReferenceChainsTestAccessor::insertFrontierEntry( + frontier, node_tags[parent], 0, 0, FrontierEntryState::EXPANDED, + JVMTI_HEAP_REFERENCE_STACK_LOCAL)); + // The holder was first discovered through another object, before the + // one-time static-field sweep reaches it. It has never been a root. + ASSERT_TRUE(ReferenceChainsTestAccessor::insertFrontierEntry( + frontier, node_tags[holder], node_tags[parent], 1, + FrontierEntryState::EXPANDED, 0)); + ReferenceChainPassContext ctx{}; + ctx.tracker = tracker; + ctx.frontier = frontier; + ctx.hop_cap = 64; + ctx.budget = 1000; + const jlong holder_class_tag = -101; + jlong owner_class_tag = -102; + for (int repeat = 0; repeat < 2; repeat++) { + ReferenceChainsTestAccessor::reportHeapReferenceForTest( + JVMTI_HEAP_REFERENCE_STATIC_FIELD, holder_class_tag, + &node_tags[holder], &owner_class_tag, &ctx); + } + EXPECT_TRUE(ReferenceChainsTestAccessor::collectStaticFieldAnchorsForRotationForTest(4).empty()); + std::vector drained; + ASSERT_EQ(1, ReferenceChainsTestAccessor::drainStaticAnchorFifoForTest(16, drained)); + int edges = 0; + ReferenceChainsTestAccessor::walkStaticAnchorFifoForTest( + &mock_jvmti, &mock_jni, {drained[0].tag}, 1000, &edges, nullptr); + ASSERT_EQ(0u, ReferenceChainsTestAccessor::staticAnchorFifoSizeForTest()); + + // Classification is outside heap callbacks. Neither an unknown shape nor + // a known non-collection may enter the persistent container tier. + ReferenceChainsTestAccessor::primeClassShapeForTest(holder_class_tag, false); + EXPECT_TRUE(ReferenceChainsTestAccessor::collectStaticFieldAnchorsForRotationForTest(4).empty()); + ReferenceChainsTestAccessor::primeClassShapeForTest(holder_class_tag, true); + auto selected = ReferenceChainsTestAccessor::collectStaticFieldAnchorsForRotationForTest(4); + ASSERT_EQ(std::vector{node_tags[holder]}, selected); + ReferenceChainsTestAccessor::walkStaticFieldAnchorsForTest( + &mock_jvmti, &mock_jni, selected, 1000, &edges); + + // Mutate after both the FIFO and fresh-lane first looks. No static-field + // edge is delivered again, so discovery requires a persistent revisit. + const jlong leak_tag = ReferenceChainsTestAccessor::leakTagBase(); + node_tags[payload] = leak_tag; + script = { + {JVMTI_HEAP_REFERENCE_FIELD, holder, table, -1}, + {JVMTI_HEAP_REFERENCE_ARRAY_ELEMENT, table, payload, -1}, + }; + selected = ReferenceChainsTestAccessor::collectStaticFieldAnchorsForRotationForTest(4); + ASSERT_EQ(std::vector{node_tags[holder]}, selected); + ReferenceChainsTestAccessor::walkStaticFieldAnchorsForTest( + &mock_jvmti, &mock_jni, selected, 1000, &edges); + ASSERT_GT(tags_ever_assigned[payload], 0); + EXPECT_NE(leak_tag, node_tags[payload]); + EXPECT_EQ(leak_tag, ReferenceChainsTestAccessor::frontierLeakTag(node_tags[payload])); + FrontierEntry holder_entry{}; + ASSERT_TRUE(frontier->lookup(node_tags[holder], &holder_entry)); + EXPECT_EQ(node_tags[parent], holder_entry.parent_tag); + EXPECT_EQ(1u, holder_entry.depth); + tracker->stop(); +} + +TEST_F(ReferenceChainsBfsTest, GlobalRootsIndexNewAndPreviouslyDiscoveredCollections) { + Arguments args; + ASSERT_FALSE(args.parse("referencechains=true:hops=64:budget=1000")); + ReferenceChainTracker *tracker = ReferenceChainTracker::instance(); + ASSERT_FALSE(tracker->start(args)); + int parent = addNode(); + int new_holder = addNode(); + int old_holder = addNode(); + node_tags[parent] = tracker->nextTag(); + node_tags[old_holder] = tracker->nextTag(); + FrontierTable *frontier = tracker->frontierTable(); + ASSERT_TRUE(ReferenceChainsTestAccessor::insertFrontierEntry( + frontier, node_tags[parent], 0, 0, FrontierEntryState::EXPANDED, + JVMTI_HEAP_REFERENCE_STACK_LOCAL)); + ASSERT_TRUE(ReferenceChainsTestAccessor::insertFrontierEntry( + frontier, node_tags[old_holder], node_tags[parent], 1, + FrontierEntryState::EXPANDED, 0)); + const jlong class_tag = -101; + ReferenceChainsTestAccessor::primeClassShapeForTest(class_tag, true); + ReferenceChainPassContext ctx{}; + ctx.tracker = tracker; + ctx.frontier = frontier; + ctx.hop_cap = 64; + ctx.budget = 1000; + for (int repeat = 0; repeat < 2; repeat++) { + for (int holder : {new_holder, old_holder}) { + ReferenceChainsTestAccessor::reportHeapRootForTest( + JVMTI_HEAP_ROOT_JNI_GLOBAL, class_tag, &node_tags[holder], &ctx); + } + } + auto selected = ReferenceChainsTestAccessor::collectStaticFieldAnchorsForRotationForTest(4); + ASSERT_EQ(2u, selected.size()); + EXPECT_NE(selected.end(), std::find(selected.begin(), selected.end(), node_tags[new_holder])); + EXPECT_NE(selected.end(), std::find(selected.begin(), selected.end(), node_tags[old_holder])); + // Both the new root and the chain-attached collection must remain indexed + // after their fresh-lane turn, without changing the old holder's parent. + EXPECT_EQ(2u, ReferenceChainsTestAccessor::collectStaticFieldAnchorsForRotationForTest(4).size()); + FrontierEntry old_entry{}; + ASSERT_TRUE(frontier->lookup(node_tags[old_holder], &old_entry)); + EXPECT_EQ(node_tags[parent], old_entry.parent_tag); + tracker->stop(); +} + // At-risk FIFO mechanics: a chain-attached holder drained from the at-risk FIFO is descend-walked and // intercepts a leak chunk 3 hops below it - the repair for the population the root-attached // collector demonstrably cannot select (the negative control below). diff --git a/ddprof-lib/src/test/cpp/referenceChainsTestAccessors.h b/ddprof-lib/src/test/cpp/referenceChainsTestAccessors.h index b159e250bf..101ca76aa0 100644 --- a/ddprof-lib/src/test/cpp/referenceChainsTestAccessors.h +++ b/ddprof-lib/src/test/cpp/referenceChainsTestAccessors.h @@ -807,6 +807,14 @@ class ReferenceChainsTestAccessor { context); } + // Deliver an observed edge through the production heap callback. + static jint reportHeapReferenceForTest(jvmtiHeapReferenceKind kind, + jlong class_tag, jlong *tag, + jlong *referrer_tag, void *context) { + return ReferenceChainTracker::heapReferenceCallback( + kind, nullptr, class_tag, 0, 0, tag, referrer_tag, -1, context); + } + // Simulates a tag left in _priority_expand by a prior pass's truncated expandFrontier() // batch without a full expandFrontier()/JVMTI round-trip. static void pushPriorityExpand(jlong tag) { diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/referencechains/LeakingCacheScenario.java b/ddprof-test/src/test/java/com/datadoghq/profiler/referencechains/LeakingCacheScenario.java index c2404f563c..cfa7dbc2c9 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/referencechains/LeakingCacheScenario.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/referencechains/LeakingCacheScenario.java @@ -294,16 +294,24 @@ private static ReferenceChainAssertions.ChainMatch findMatch(Path scratchDumpPat } /** - * Realistic-leak fixture - identical shape to {@code ReferenceChainTrackingTest.CachedPayload}. - * The 32 {@code long} fields exist purely so a given number of megabytes needs far fewer entries - * to reach {@code ObjectSampler}'s real 256KiB sampling floor - deliberately plain fields, not - * a nested array (a same-instance companion array would be a second, separate heap allocation - * the size-weighted sampler would compete for, taking attention away from CachedPayload). + * Inline padding supplies sampled instances for the detector's eight-survivor per-thread + * threshold while preserving the one-shot allocator threads and number of retained objects. + * With only 32 longs, a failing release run retained at most six sampled instances per worker: + * its one-age cohorts could never qualify, regardless of traversal time. About 1KiB per payload + * provides more sampling opportunities within the same rounds at the 256KiB sampling interval. + * Plain fields keep that allocation volume on CachedPayload itself; a companion array would + * compete for samples without contributing any CachedPayload survivors. */ static final class CachedPayload { final String key; long p0, p1, p2, p3, p4, p5, p6, p7, p8, p9, p10, p11, p12, p13, p14, p15; long p16, p17, p18, p19, p20, p21, p22, p23, p24, p25, p26, p27, p28, p29, p30, p31; + long p32, p33, p34, p35, p36, p37, p38, p39, p40, p41, p42, p43, p44, p45, p46, p47; + long p48, p49, p50, p51, p52, p53, p54, p55, p56, p57, p58, p59, p60, p61, p62, p63; + long p64, p65, p66, p67, p68, p69, p70, p71, p72, p73, p74, p75, p76, p77, p78, p79; + long p80, p81, p82, p83, p84, p85, p86, p87, p88, p89, p90, p91, p92, p93, p94, p95; + long p96, p97, p98, p99, p100, p101, p102, p103, p104, p105, p106, p107, p108, p109, p110, p111; + long p112, p113, p114, p115, p116, p117, p118, p119, p120, p121, p122, p123, p124, p125, p126, p127; CachedPayload(String key) { this.key = key; From ba932822235c51a3854b541704d9795fbe1324d5 Mon Sep 17 00:00:00 2001 From: Paul Fournillon Date: Fri, 23 Oct 2026 21:31:33 +0200 Subject: [PATCH 3/3] Isolate seeded class histories to debug test hooks --- ddprof-lib/src/main/cpp/javaApi.cpp | 9 ++-- ddprof-lib/src/main/cpp/livenessTracker.cpp | 12 +++-- ddprof-lib/src/main/cpp/livenessTracker.h | 47 ++++++++++++++----- .../src/test/cpp/livenessTracker_ut.cpp | 34 +++++++++++--- 4 files changed, 73 insertions(+), 29 deletions(-) diff --git a/ddprof-lib/src/main/cpp/javaApi.cpp b/ddprof-lib/src/main/cpp/javaApi.cpp index 79de11b68b..ae642ba69a 100644 --- a/ddprof-lib/src/main/cpp/javaApi.cpp +++ b/ddprof-lib/src/main/cpp/javaApi.cpp @@ -1125,14 +1125,11 @@ extern "C" DLLEXPORT void JNICALL Java_com_datadoghq_profiler_JavaProfiler_seedKlassPopulationSample0( JNIEnv *env, jclass unused, jint klassId, jint count, jlong epoch) { ProfiledThread::initCurrentThreadSignalSafe(); - int slot; - bool created; jweak evicted[KlassPopulationEntry::MAX_REPRESENTATIVES_PER_KLASS]; int evicted_count = 0; - LivenessTracker::instance()->klassPopulationRecordForTest( - (u32)klassId, (u32)count, (u64)epoch, &slot, &created, evicted, - &evicted_count, KlassPopulationEntry::MAX_REPRESENTATIVES_PER_KLASS, - /*synthetic_sample=*/true); + LivenessTracker::instance()->seedKlassPopulationSampleForTest( + (u32)klassId, (u32)count, (u64)epoch, evicted, + &evicted_count, KlassPopulationEntry::MAX_REPRESENTATIVES_PER_KLASS); for (int i = 0; i < evicted_count; i++) { if (evicted[i] != nullptr) { env->DeleteWeakGlobalRef(evicted[i]); diff --git a/ddprof-lib/src/main/cpp/livenessTracker.cpp b/ddprof-lib/src/main/cpp/livenessTracker.cpp index 12db40e5fa..b2bf73b948 100644 --- a/ddprof-lib/src/main/cpp/livenessTracker.cpp +++ b/ddprof-lib/src/main/cpp/livenessTracker.cpp @@ -965,8 +965,7 @@ void LivenessTracker::accumulateKlassCount(u32 klass_id, jlong age, jweak LivenessTracker::recordKlassPopulationSampleLocked( u32 klass_id, u32 count, u64 epoch, int *out_slot, bool *out_created, - jweak *out_evicted, int *out_evicted_count, int max_evicted, - bool synthetic_sample) { + jweak *out_evicted, int *out_evicted_count, int max_evicted) { // Linear scan is fine: MAX_KLASS_POPULATION_ENTRIES is small enough that a // full scan is cheap, the same shape NativeSocketSampler's fd LRU // (NativeSocketSampler's fd cache) and this class's own cleanup_table() @@ -1015,7 +1014,9 @@ jweak LivenessTracker::recordKlassPopulationSampleLocked( _klass_population[slot].ring_head = 0; _klass_population[slot].ring_fill = 0; _klass_population[slot].consecutive_positive = 0; - _klass_population[slot].synthetic = false; +#ifdef DEBUG + _klass_population[slot].test_seeded_history = false; +#endif _klass_population[slot].cached_slope = 0.0; // A reused (evicted) slot's previous class's per-tid trends must not // leak onto the new one, same as the fields above. @@ -1028,7 +1029,8 @@ jweak LivenessTracker::recordKlassPopulationSampleLocked( } KlassPopulationEntry &entry = _klass_population[slot]; - if (entry.synthetic && !synthetic_sample) { +#ifdef DEBUG + if (entry.test_seeded_history) { // Real GC folds must not replace a JNI test fixture's seeded ramp with // the sampled cohort's generation count. Keep its LRU epoch current. entry.last_updated_epoch = epoch; @@ -1036,7 +1038,7 @@ jweak LivenessTracker::recordKlassPopulationSampleLocked( *out_created = created; return evicted_ref; } - entry.synthetic = entry.synthetic || synthetic_sample; +#endif entry.count_ring[entry.ring_head] = count; entry.ring_head = (u8)((entry.ring_head + 1) % KLASS_POPULATION_RING_SIZE); if (entry.ring_fill < KLASS_POPULATION_RING_SIZE) { diff --git a/ddprof-lib/src/main/cpp/livenessTracker.h b/ddprof-lib/src/main/cpp/livenessTracker.h index 24b45f0e89..00a182672d 100644 --- a/ddprof-lib/src/main/cpp/livenessTracker.h +++ b/ddprof-lib/src/main/cpp/livenessTracker.h @@ -107,9 +107,11 @@ typedef struct KlassPopulationEntry { // without this counter it gets reported as a leak candidate almost as // often as a real leak does. u8 consecutive_positive; - // Explicit JNI test seeds own their class trend, as synthetic TidTrend - // histories do. Ordinary allocation-derived histories leave this false. - bool synthetic; +#ifdef DEBUG + // Explicit JNI test seeds own their class trend. Real GC folds preserve it + // until the test resets the population table or the entry is evicted. + bool test_seeded_history; +#endif // Slope (regression value at the ring's newest sample minus the value at // its oldest sample - see ringWindowStats, livenessTracker.cpp) as of the // last push, computed and cached by hasQualifyingGrowth() alongside @@ -701,8 +703,7 @@ class alignas(alignof(SpinLock)) LivenessTracker { int *out_slot, bool *out_created, jweak *out_evicted = nullptr, int *out_evicted_count = nullptr, - int max_evicted = 0, - bool synthetic_sample = false); + int max_evicted = 0); // Drains _klass_count_scratch into _klass_population for the epoch that // just finished, minting a fresh representative jweak (from each entry's @@ -1264,27 +1265,49 @@ class alignas(alignof(SpinLock)) LivenessTracker { _klass_population_size--; } - // Only explicit JNI fixture seeds set synthetic_sample: real GC folds - // preserve their ramp. The default keeps ordinary algorithm-test histories - // subject to production updates and decay. + // Ordinary algorithm-test histories remain subject to GC updates and decay. jweak klassPopulationRecordForTest(u32 klass_id, u32 count, u64 epoch, int *out_slot, bool *out_created, jweak *out_evicted = nullptr, int *out_evicted_count = nullptr, - int max_evicted = 0, - bool synthetic_sample = false) { + int max_evicted = 0) { _table_lock.lock(); klass_id = resolveTestKlassAliasLocked(klass_id); jweak evicted = recordKlassPopulationSampleLocked(klass_id, count, epoch, out_slot, out_created, out_evicted, out_evicted_count, - max_evicted, - synthetic_sample); + max_evicted); _table_lock.unlock(); return evicted; } +#ifdef DEBUG + /** + * Seeds the JNI traversal fixture's class history and protects it from real + * GC updates. Clear protection while recording another explicit seed so it + * updates the same ring and qualification counters as ordinary samples. + */ + void seedKlassPopulationSampleForTest( + u32 klass_id, u32 count, u64 epoch, jweak *out_evicted = nullptr, + int *out_evicted_count = nullptr, int max_evicted = 0) { + _table_lock.lock(); + klass_id = resolveTestKlassAliasLocked(klass_id); + for (int i = 0; i < _klass_population_size; i++) { + if (_klass_population[i].klass_id == klass_id) { + _klass_population[i].test_seeded_history = false; + break; + } + } + int slot; + bool created; + recordKlassPopulationSampleLocked(klass_id, count, epoch, &slot, &created, + out_evicted, out_evicted_count, max_evicted); + _klass_population[slot].test_seeded_history = true; + _table_lock.unlock(); + } +#endif + // Seeds one per-tid trend sample for tests/scenarios: pushes `count` as // tid's epoch sample on klass_id's KlassPopulationEntry, marking the // trend SYNTHETIC (exempt from the real fold's absent-tid decay and slot diff --git a/ddprof-lib/src/test/cpp/livenessTracker_ut.cpp b/ddprof-lib/src/test/cpp/livenessTracker_ut.cpp index c74d22f1fa..a87b9545fe 100644 --- a/ddprof-lib/src/test/cpp/livenessTracker_ut.cpp +++ b/ddprof-lib/src/test/cpp/livenessTracker_ut.cpp @@ -1214,14 +1214,11 @@ class LivenessTrackerTestAccessor { } }; -TEST_F(SelectLeakCandidatesTest, SyntheticClassHistorySurvivesRealGcFolds) { +#ifdef DEBUG +TEST_F(SelectLeakCandidatesTest, SeededClassHistorySurvivesRealGcFolds) { LivenessTracker *tracker = LivenessTracker::instance(); for (int epoch = 1; epoch <= 20; epoch++) { - int slot; - bool created; - tracker->klassPopulationRecordForTest( - 1, epoch * 10, epoch, &slot, &created, nullptr, nullptr, 0, - /*synthetic_sample=*/true); + tracker->seedKlassPopulationSampleForTest(1, epoch * 10, epoch); tracker->tidTrendRecordForTest(1, 7, epoch * 3, epoch); } KlassCandidate out[5]; @@ -1230,8 +1227,33 @@ TEST_F(SelectLeakCandidatesTest, SyntheticClassHistorySurvivesRealGcFolds) { LivenessTrackerTestAccessor::foldClassCohort(tracker, 1, 7, epoch); } EXPECT_EQ(1, tracker->selectLeakCandidates(out, 5)); + // A fold containing only another class must also preserve the seed when + // the zero-sample pass visits classes absent from the GC scratch table. + LivenessTrackerTestAccessor::foldClassCohort(tracker, 2, 8, 31); + EXPECT_EQ(1, tracker->selectLeakCandidates(out, 5)); + KlassPopulationEntry entry; + ASSERT_TRUE(tracker->klassPopulationLookupForTest(1, &entry)); + EXPECT_EQ(31u, entry.last_updated_epoch); } +TEST_F(SelectLeakCandidatesTest, SeededClassHistoryAcceptsExplicitUpdatesAfterGc) { + LivenessTracker *tracker = LivenessTracker::instance(); + for (int epoch = 1; epoch <= 20; epoch++) { + tracker->seedKlassPopulationSampleForTest(1, epoch * 10, epoch); + tracker->tidTrendRecordForTest(1, 7, epoch * 3, epoch); + } + KlassCandidate out[5]; + ASSERT_EQ(1, tracker->selectLeakCandidates(out, 5)); + LivenessTrackerTestAccessor::foldClassCohort(tracker, 1, 7, 21); + // Protection excludes GC updates, but must not freeze subsequent explicit + // fixture input or bypass the normal qualification calculation. + for (int epoch = 22; epoch <= 51; epoch++) { + tracker->seedKlassPopulationSampleForTest(1, 0, epoch); + } + EXPECT_EQ(0, tracker->selectLeakCandidates(out, 5)); +} +#endif + TEST_F(SelectLeakCandidatesTest, OrdinaryClassHistoryStillDecaysAfterGrowthStops) { LivenessTracker *tracker = LivenessTracker::instance(); for (int epoch = 1; epoch <= 20; epoch++) {