Repository navigation
Conversation
|
@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
CI Test ResultsRun: #41111734688 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-10-30 19:23:21 UTC |
aa527d2 to
2e0bf07
Compare
|
Note for CI triage: |
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
Reliability & Chaos Results✅ All reliability & chaos checks passed Pipeline: https://gitlab.ddbuild.io/DataDog/java-profiler/-/pipelines/148870218 |
2974e98 to
a5dc1d8
Compare
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.
522ee15 to
9f0aeca
Compare
rkennke
left a comment
rkennke
left a comment
There was a problem hiding this comment.
Reviewed the gating change and the new urgency/admission logic. The main concerns:
- Overhead regression for liveheap-only users: the leak-signal flag isn't tied to reference chains being enabled.
- The 5-minute burn-in also turns off the OOM ramp/emergency admission, not only the urgent latch.
updateUrgentAdmission()baseline handling: it never advances, and it isn't seeded from the emergency band.
The rest are nits. I also checked removing the urgent candidate-less bypass: since pollWatchedTargets() only builds chains for selectLeakCandidates() results, I agree the old bypass couldn't produce chains on its own, so no objection there.
| // table itself persists across recordings. | ||
| _gc_generations.store(args._gc_generations, std::memory_order_relaxed); | ||
| // See _leak_signal_enabled's own comment for why either source suffices. | ||
| _leak_signal_enabled.store(args._gc_generations || args._record_liveness, |
There was a problem hiding this comment.
_leak_signal_enabled now follows generations || liveness regardless of whether reference chains are enabled. A plain liveheap session (memory=N:l, no referencechains) therefore starts paying for:
- per-klass population tracking in
cleanup_table()(resolveKlassId()JNI round-trips +foldKlassCountsLocked()under_table_lock), - a heap-floor sample on every GC in
onGC()(resolvePostGcHeapUsage()plus anOS::getContainerMemoryUsage()cgroup read when the container limit is resolved), maybeForceCleanup()forced sweeps every 30s.
Before this PR those were gated on _gc_generations, so liveheap-only users didn't pay. The only consumers are ReferenceChainTracker and the test seams, so this looks like it should also require reference chains to be enabled (e.g. fold args._reference_chains into the flag, or have the tracker set it when it starts).
There was a problem hiding this comment.
Good catch, and embarrassing that three review rounds did not surface it. Fixed in d8e594993: the flag is now generations || (record_liveness && reference_chains), so plain memory=N:l keeps its old cheap profile — track() still feeds the liveness events, but the klass folding, heap-floor ring and forced sweeps stay off. The truth-table tests now pin exactly this: liveheap-only disarms the gate and its consumers, liveheap + referencechains=true arms it.
| // modular; production stamps are always <= now. | ||
| u64 activated_ns = _leak_signal_activated_ns.load(std::memory_order_acquire); | ||
| u64 since_ns = OS::nanotime() - activated_ns; | ||
| if (activated_ns == 0 || since_ns < OOM_PROJECTION_BURN_IN_NS) { |
There was a problem hiding this comment.
Question on the burn-in tradeoff: since _leak_signal_activated_ns is stamped on first activation, for the first 5 minutes secondsToOOM() returns -1. That disables everything keyed on it, not just the urgent latch: the threadLoop() cadence/pause-target ramp, the budget boost, updateUrgentAdmission() (including the emergency band, whose comment says it admits "without waiting"), and the no-progress-abandonment suppression.
So attaching to a JVM that is already leaking, or a process that OOMs 3–4 minutes after start, gets none of the end-of-life capture. I get why warmup growth needs suppressing for the urgent latch. Could the burn-in apply only to isUrgent() and leave the ramp running? Alternatively, it could be released early once the projection is backed by a real candidate.
There was a problem hiding this comment.
Burn-in stays as is. The two exposure cases do not justify a change: we do not attach mid-life today, and if we add it, a 5-minute blind window on an already-leaking heap is an acceptable tradeoff. Early release on candidates fails either way — a warming cache produces the same monotone per-klass survivor growth a leak does, so it re-opens warmup-driven searching. A process OOMing within ~4 minutes of start is not salvageable by widening the search window; that case belongs to a different mechanism — heap dump on OOME plus post-hoc extraction of the interesting parts — which is separate work.
| // that means "always true" here. | ||
| bool ReferenceChainTracker::updateUrgentAdmission(double seconds_to_oom, bool urgent) { | ||
| if (!urgent) { | ||
| // Left set after an episode so the next episode re-snapshots against that |
There was a problem hiding this comment.
This comment contradicts the next line: it says the baseline is "Left set after an episode", but the code clears it. I think it should say something like "Cleared when the episode ends so the next episode re-snapshots against its own entry state."
There was a problem hiding this comment.
Yeah, that comment says the opposite of what the code does. Rewrote it to what's actually happening: cleared when the episode ends so the next episode re-snapshots against its own entry state (d8e594993).
| // last chapter can before the process dies, without waiting for growth | ||
| // evidence. OOM_URGENT_THRESHOLD_S is isUrgent()'s own latch threshold, so | ||
| // the emergency band and the hysteretic urgent episode cover the same range. | ||
| if (seconds_to_oom < OOM_URGENT_THRESHOLD_S) { |
There was a problem hiding this comment.
The emergency-band early return never touches _admission_growth_baseline. If an episode's first projection is already < 300s (likely, since the burn-in can hide the projection until it is already short), baseline is still 0 here. The first time the projection rises above 300s (e.g. 295s → 305s while still in the 1800s ramp band), the next branch treats it as a "first observation": it snapshots and returns false. 100% admission then switches off until a newer GC sample exceeds that snapshot, even though the heap is still growing. Consider seeding the baseline here too (when it's 0 and a sample exists).
There was a problem hiding this comment.
Nice trace. Fixed — the emergency band now seeds the baseline when it's still 0 and a sample exists, so the ramp-band transition compares against the entry level instead of re-snapshotting (d8e594993). Covered by a new test (EmergencyBandEntrySeedsGrowthBaseline) in both directions: growing past the seed admits, flat doesn't.
| _admission_growth_baseline = newest_used; | ||
| return false; | ||
| } | ||
| return newest_used > _admission_growth_baseline; |
There was a problem hiding this comment.
The baseline is captured once per episode and never moves, so this compares against the episode's entry level, not against recent growth. Example: baseline 1.0 GB at band entry, a spike to 1.3 GB, then the heap settles flat at 1.1 GB. The projection window still contains the earlier rise, so it can stay inside the 1800s band for several GCs, and 1.1 GB > 1.0 GB keeps setUrgentTracking(true) the whole time. The comment above says admission happens "never for a heap that has stopped growing", but here it does. Comparing against the previous sample (or advancing the baseline to each newly observed sample) would match the stated intent.
There was a problem hiding this comment.
Also right. The baseline is now a high-water mark — every admitted sample raises it. So the 1.0 → 1.3 → 1.1 example drops the boost at the first settled sample and only re-admits when growth passes the previous peak. New test PlateauAboveEntryBaselineLosesBoost (d8e594993).
| // the no-op cheap. Read-only; this accessor never toggles the flag. | ||
| // Read-only observation point for the explicit generations option (see | ||
| // _gc_generations's own comment); no production code keys on it. | ||
| bool gcGenerationsEnabled() const { |
There was a problem hiding this comment.
Nit: after this PR, gcGenerationsEnabled() (and effectively _gc_generations, apart from a TEST_LOG) is no longer read by production code. Keeping a public accessor for the wrong gate invites future code to key on it, which is the bug this PR fixes. Consider removing it and having the tests assert on leakSignalEnabled(), or moving it under DEBUG || UNIT_TEST.
A few comments in the PR also describe history rather than current behaviour; these go stale:
hasLeakSignal(): "…when this branch used to return true"setGcGenerationsForTest: "Kept for existing callers…"AggressiveLeakReferenceChainTestjavadoc: "That bypass and its one-shot urgency entitlement were removed…"
There was a problem hiding this comment.
Went with guarding it under DEBUG || UNIT_TEST instead of removing — the truth tables assert both gates independently, and I'd rather keep that pinned. History comments fixed too (d8e594993).
| ) | ||
| } | ||
|
|
||
| // Test .inc/.h files resolve via quoted includes relative to the test |
There was a problem hiding this comment.
Nit (AGENTS.md: "If modifying a file that does not contain Datadog copyright, add one"): this file has no Datadog copyright header. The same applies to ddprof-lib/src/test/cpp/referenceChainsOomTests.inc and referenceChainsLifecycleTests.inc, which this PR also modifies.
There was a problem hiding this comment.
Added, same for the two .inc files (d8e594993).
- Derive the leak signal from generations || (liveheap && referencechains): the collector is the signal's only consumer, so a plain liveheap session keeps its cheaper profile (no per-epoch klass folding, heap-floor ring or forced sweeps). - Seed the urgent-admission growth baseline at emergency-band entry so the first ramp-band observation compares against the episode entry level instead of re-snapshotting and dropping the boost mid-growth. - Advance the baseline per admitted sample (high-water mark): a plateau above the entry level loses the boost until growth resumes. - Guard gcGenerationsEnabled() as test-only; rewrite comments that narrated history or an internal observation instead of current behavior. - Add copyright headers to the files this PR modifies that lacked one.
- StopClearsAdmissionGrowthBaseline no longer races the BFS thread: the test pins bfsThreadRunning() == false before driving the updateUrgentAdmission accessor directly. - threadLoop() computes secondsToOOM() once per iteration and threads the value through shouldRunPass/canAffordNewSearch/hasLeakSignal/ isUrgent; one-arg wrappers read once and forward. - New publishUrgencyState() test drives the real admission gate and asserts _urgent_tracking follows the gated decision. - AggressiveLeak's candidate-free check retries once before failing, tolerating a transient candidate in the shared test JVM. - setLeakSignalActivatedNsForTest0 negative-argument guard now has a test; the stamp is restored in finally. - GtestTaskBuilder declares PathSensitivity.RELATIVE on the test include file tree as a named task input. - Ownership comments on _admission_growth_baseline/_oom_ramp_active amended to name the non-BFS writers.
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.