Repository navigation
Conversation
Collaborator
Author
|
@DataDog review |
![datadog-official[bot]](https://avatars.githubusercontent.com/in/1358509?s=60&v=4){kind=small}
There was a problem hiding this comment.
Existing Java integration scenarios still require candidate-free searches. The new admission gate prevents those searches, invalidating their pass-count and abandonment expectations.
🤖 Bits Code Review · Commit 07ab531
Contributor
CI Test ResultsRun: #41061208905 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-10-30 14:13:27 UTC |
Contributor
jbachorik
force-pushed
the
investigate-reference-chains
branch
2 times, most recently
from
October 29, 2026 16:09
aa527d2 to
2e0bf07
Compare
Collaborator
Author
|
Note for CI triage: |
jbachorik
marked this pull request as ready for review
October 30, 2026 10:00
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51348a4fc8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
More details
The candidate gate prevents urgency from starting empty searches while preserving liveheap-only population signals. Static review found the burn-in lifecycle and recording-boundary resets consistent with the intended warmup protection.
🤖 Bits Code Review · Commit 51348a4
Contributor
Reliability & Chaos Results✅ All reliability & chaos checks passed Pipeline: https://gitlab.ddbuild.io/DataDog/java-profiler/-/pipelines/148870218 |
1 of 3 tasks
jbachorik
force-pushed
the
investigate-reference-chains
branch
from
October 30, 2026 13:03
2974e98 to
a5dc1d8
Compare
2 of 3 tasks
The collector ran whole-heap searches in healthy processes through two authorization paths that bypassed the candidate requirement: - hasLeakSignal() returned true unconditionally when generations tracking was off, so a liveheap-only session passed the gate with zero leak candidates; the search cannot exit early (the no-progress abandonment is suppressed while urgent) and ran to the auto-tuned TTL. - The urgent-OOM latch entitled one candidate-less search per episode, and a warmup heap-floor ramp projected imminent OOM from a seconds-long observation window. hasLeakSignal() now keys on an effective flag (generations OR liveheap tracking) and requires selectLeakCandidates() > 0; urgency only sharpens the OOM ramp while a candidate-driven search runs. secondsToOOM() returns no projection until the leak signal has been active for a burn-in period, so transient warmup growth cannot latch urgency. The per-search urgent-entitlement flag was removed (behaviorally dead).
AggressiveLeakReferenceChainTest asserted that the BFS thread runs passes from the urgent-OOM projection alone with zero per-klass candidates - the behavior the gate fix removed. It now asserts the opposite: with the leak signal armed and the projection deep in the urgent band, the gate stays closed (live-thread idle window plus a race-free direct shouldRunPass() evaluation - the removed one-shot entitlement was what made the direct call order-dependent). ReferenceChainTrackingTest.shouldReportAbandonedSearchOnTinyFrontierCap qualifies its candidate only through the debug-only population seams; in a release build no candidate exists and the search now (correctly) stays idle, so the method assumes a debug build. The profiler-command comment still described the removed hasLeakSignal() short-circuit; updated.
The OOM ramp's 100% tracking admission followed the raw projection-band comparison, but the smoothed 30-sample regression keeps the projection inside the band for several recovery GCs after the heap has physically stopped growing. Outside the emergency band (<5min) the boost now requires the newest heap-floor sample to exceed the value observed at episode entry; a recovered spike never satisfies that. Emergency-band admission stays immediate, and the ramp's budget/cadence effects are unchanged. Also corrects two stale claims about the urgency latch: the ramp calculation never consults it (its only consumer is no-progress abandonment suppression), and the budget boost follows the raw band, not the latch.
stop() and resetSearchStateForTest() now clear _admission_growth_baseline alongside the urgency latch: a stale baseline would let the next episode's first urgent observation skip updateUrgentAdmission()'s entry re-snapshot and admit against the previous recording's heap level. hasLeakSignal() increments reference_chain_urgent_no_candidate when urgency is latched but no klass explains the growth, so the candidate-only gate's refused evaluations are visible in diagnostics instead of silent. The leak-signal test mutators (setLeakSignalSourcesForTest, setLeakSignalActivatedNsForTest) move behind DEBUG/UNIT_TEST with JNI seams (backdateLeakSignalActivationForTest0 and companions) so no release consumer can flip the gate or backdate the burn-in. Stale comments on OOM_URGENT_THRESHOLD_S, gcGenerationsEnabled() and secondsToOOM() corrected to match the candidate-only gate.
- StopClearsAdmissionGrowthBaseline pins the stop() baseline reset. - AggressiveLeak: the negative search-gate test now asserts the projection is actually engaged (backdated burn-in stamp via the new seam), so its assertions cannot pass vacuously while secondsToOOM() is inside burn-in; cleanup goes through the shared restoreHeapFloorDefaults() helper. - ReferenceChainTrackingTest: the abandonment retry loop is capped at 3 dumps with a passesRun probe and paced last dump, bounding the O(recording-size) dump I/O; the dead if-guard behind the debug-build assumption is removed; a growth-gate equality scenario pins > vs >=.
The latch neither starts searches nor feeds the ramp calculation; its only consumer is no-progress-abandonment suppression. The admission boost releases on the raw band comparison, not the latch hysteresis. The candidate-only gate's refused urgent evaluations are now counted.
- setLeakSignalActivatedNsForTest0 rejects negative arguments instead of casting them to a near-UINT64_MAX burn-in stamp that would read as burn-in-never-passed. - evaluateGateAfterSearchReset samples the per-klass candidate table before the search reset: a pass decided on an organic candidate can straddle the reset (the pass counter increments at pass end), and the candidate stays selectable from the not-reset population table, so the pre-reset read keeps the candidateSeen bracket honest. - The abandonment test's warmup dump path is deleted inside the try/finally guard, closing the leak window between createTempFile and guard entry.
- The stop()-side reset of the urgency/baseline fields now states the full claim: production writes go through the BFS thread only, and the join in stopThread() is what makes the control-thread plain stores safe. - The leak-signal test-seam block now records why UNIT_TEST is required beyond DEBUG (gtestRelease compiles with -DNDEBUG and no -DDEBUG) and which consumer class needs each arm.
Each fact now has one home: the generations/liveheap equivalence lives on _leak_signal_enabled, the SPSC acquire pairing on newestHeapFloorSample()'s definition, the burn-in rationale on OOM_PROJECTION_BURN_IN_NS; the other sites point instead of restating.
The reference-chain subsystem contract changed: collection stays idle without a generations or liveheap leak-signal source. Reflect that in the AGENTS.md subsystem row and the doc/README.md index entries.
Indexes only name the subsystem; activation semantics belong to the linked pages.
The per-test compile task wired only the test .cpp into sources, while the .inc test partitions and test-local headers resolve via quoted includes relative to the test directory and appear in no include path. An .inc-only edit therefore reported UP-TO-DATE and silently ran a stale binary with the old tests. Register them as task inputs (not sources: they must not be compiled standalone) and drop the touch-the-.cpp workaround comment.
Comments must state the mechanism, not how it was observed. Drop the environment, counter values and recording-size anecdotes; the factual claim each comment makes is unchanged.
The cache-leak and gc-root tests flake in-order because the search's root walk runs once per search (no re-visit of walked regions), the fixtures' HashMap/ArrayList resize churn invalidates the walked path mid-test, and the BFS thread's own backoff/cooldown scheduling leaves too few passes to retry discovery. Fix, test-only: - Pre-size the fixture cache (1<<17) so the walked table/Node path never dies to resize churn. - Drive bounded passes via the runReferenceChainPass0() debug seams in both success-path tests' retry loops (release stubs are no-ops), so discovery depends on cumulative frontier progress, not BFS cadence. - Widen both grace loops to 10 attempts; add failure diagnostics (leak-signal candidates, BFS pass count, representative frontier state, observed chain[0]s) plus the observedChainHeads() helper. - Correct stale comments (dump() drain semantics, nonexistent writeReferenceChain symbol, MIN_EFFECTIVE_BUDGET floor). Verified: 6/6 + 7/7 forced genuine in-order sweeps (cleanTestSlowDebug), isolated pass; the unmodified baseline reproduces the original flake.
Review follow-ups (sphinx, 4 accepted LOW findings): - Extract ReferenceChainAssertions.referenceChainFailureDiagnostics(): one canonical failure fingerprint shared by both discovery tests, closing the gc-root test's missing observedChain0s segment. - Move the debug-build gate inside driveReferenceChainPasses(); call sites no longer repeat it. - observedChainHeads() now reports <empty-chain> / <non-array-chain:...> sentinels instead of silently skipping malformed events; javadoc matches. - Pin the fingerprint format in ReferenceChainTestSeamsTest.
jbachorik
force-pushed
the
investigate-reference-chains
branch
from
October 30, 2026 15:13
522ee15 to
9f0aeca
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?:
Fixes the live-heap reference-chain collector running whole-heap searches in healthy processes. Two authorization paths bypassed the leak-candidate requirement:
hasLeakSignal()returned true unconditionally when generations tracking was off. A liveheap-only session then passed the gate with zero leak candidates, andpollWatchedTargets()fed the search loop the same way. The search cannot exit early — completion requires an untruncated pass and the 30-pass no-progress abandonment is suppressed while urgent — so it ran to the auto-tuned TTL. Staging evidence: frontier table grown to its cap, 0ReferenceChainevents, 0 abandonment events.secondsToOOM()needs only 10 GC samples and has no JVM-age requirement), arming the path from a seconds-long observation window.The fix:
hasLeakSignal()keys on an effective flag — generations OR liveheap tracking drives the same population-trend mechanisms — and requiresselectLeakCandidates() > 0. With neither on, the collector stays idle: no pass, restart, or search.secondsToOOM()returns no projection until the leak signal has been active for a burn-in period (OOM_PROJECTION_BURN_IN_NS, ~300 s), so transient warmup growth cannot latch urgency regardless of how frequently GCs compress the observation window.Motivation:
Staging recordings of
prof-analyzer(PROF-16284) showed the collector churning whole-heap BFS passes with zero leak candidates in healthy processes. Each pass pays a global handshake plus EA-materialization frame deopts (IterateOverReachableObjects→EscapeBarrier→deoptimize_frame), so an uncandidate search is pure recurring overhead on exactly the C2-heavy workloads where it stings.Additional Notes:
/sphinx review --effort=deep): 11 findings, all 8 fix-selections implemented and verified before this PR (liveheap-onlyinitialize()test cluster; observation-window guard replaced with the age-based burn-in; scratch exclusions). 3 informational/dismissed, 3 folded by adjudication.reference_chain_urgent_no_candidatecounter making the candidate-only gate's refused urgent evaluations visible, a positive control against the AggressiveLeak test's burn-in-vacuous assertions, and a cap on the abandonment test's dump retries. Range validation of the gate's equality boundary included.ReferenceChainTrackingTest.shouldReconstructReferrerChainThroughUnboundedCacheLeak(the CI note in the previous version of this description) is now fixed in this PR (last two commits). Root cause, from level-2 traces of forced genuine runs: the search's root walk runs once per search and later passes never re-visit walked regions; the fixture'sHashMapresizes ~14x while growing, so the walked table/Node path dies and the walked region is never re-admitted; and the BFS thread's own backoff/restart scheduling leaves too few passes to retry discovery. The fix is test-only: the fixture cache is pre-sized past its final load (no resize, walked path survives), both success-path tests drive bounded passes through the debug seams so discovery depends on cumulative frontier progress instead of BFS cadence, and both failure messages carry a canonical diagnostics fingerprint (ReferenceChainAssertions.referenceChainFailureDiagnostics). Verified with forced in-order sweeps (cleanTestSlowDebug; repeated plain invocations are Gradle UP-TO-DATE no-ops): 6/6 + 7/7 passes, isolated pass, and a reproduction on the pre-fix baseline. A dedicated sphinx review of the fix (7 findings, 0 critical/high) sealed 4 fixes and 3 dismissals.LeakTagCorrelationReferenceChainTest(shouldRotateFrontierLeakHoldersUnderPodLikeTightWindows,shouldCorrelateLeakChainTargetTagWithLiveObjectLeakTag) is separately flaky in-order. Both failures reproduce with this PR's test changes stashed or absent (Rotatealso fails in isolation), and the class runs before any of the fix's classes in the shared test JVM — pre-existing, out of scope here, not covered by PROF-16383's fix.How to test the change?:
C++ unit tests:
:ddprof-lib:gtestDebug— 1039 OK / 0 failed, including the new gate tests:LivenessTrackerSessionTest.LeakSignalRequiresGenerationsOrLiveness/LeakSignalDerivedByInitialize/LeakSignalSourcesSeamMatchesDerivation— effective-flag derivation, including liveheap-only sessions.SearchRestartTest.NoSearchWithoutLeakSignalSource/UrgentLatchWithoutCandidateDoesNotStartSearch— collector idles without a signal; urgency never starts a search.OomUrgencyTest.WarmupShortWindowDoesNotLatchUrgencyandSecondsToOOMTestburn-in cases — warmup growth cannot latch urgency.OomUrgencyTest.AdmissionBoostRequiresGrowthPastEntryBaseline/RecoveredSpikeStaysUnboostedWhileProjectionLags/EmergencyBandAdmitsWithoutGrowthEvidence/BandExitResetsGrowthBaseline— the admission growth gate, including the smoothed-projection-lags-recovery counterexample.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.