Repository navigation
Reference chains: recycled leak-tag identity and Java integration tests - #800
Merged
Merged
Conversation
2 tasks done
Contributor
Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This comment has been minimized.
This comment has been minimized.
Contributor
CI Test ResultsRun: #39299639079 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-10-19 18:27:17 UTC |
jbachorik
force-pushed
the
jb/rc-6-java-tests
branch
from
September 17, 2026 19:08
1916216 to
9ede839
Compare
Contributor
jbachorik
added this pull request to stack #803
September 17, 2026 20:02
jbachorik
force-pushed
the
jb/rc-6-java-tests
branch
from
September 17, 2026 20:05
9ede839 to
9fc9b0a
Compare
jbachorik
force-pushed
the
jb/rc-6-java-tests
branch
from
September 17, 2026 20:19
9fc9b0a to
1fad8a3
Compare
jbachorik
force-pushed
the
jb/rc-6-java-tests
branch
from
September 17, 2026 20:20
1fad8a3 to
4386c2b
Compare
2 tasks done
jbachorik
force-pushed
the
jb/rc-6-java-tests
branch
from
September 17, 2026 21:01
4386c2b to
d64a518
Compare
jbachorik
force-pushed
the
jb/rc-6-java-tests
branch
from
September 17, 2026 21:33
d64a518 to
4cd8612
Compare
jbachorik
force-pushed
the
jb/rc-6-java-tests
branch
2 times, most recently
from
September 18, 2026 05:52
19e586c to
aedf5bf
Compare
jbachorik
force-pushed
the
jb/rc-6-java-tests
branch
from
September 18, 2026 06:04
aedf5bf to
cec993f
Compare
jbachorik
force-pushed
the
jb/rc-6-java-tests
branch
from
September 18, 2026 07:06
cec993f to
1b8b709
Compare
…ne_lock frontierStateForTest() read _frontier with no lock while sibling seams take _engine_lock; resolvedChainTargetTagForTest() sampled _search_start_ns before taking _resolved_chains_lock. Take _engine_lock -> _resolved_chains_lock in both, cover the states, sentinels and lock ordering with gtests, and document the retired EDGE state and the -DDEBUG seam set.
…-opens on JDK 8 The scenario's native seams compile into debug/asan/tsan, so enable the seam path on all three (asserted via a stdout marker) instead of config==debug only. --add-opens is rejected by JDK 8's launcher, so add it only on JDK 9+.
Tail now watches a fresh chunk rotated on an 8-attempt maturation window, re-pins the representative each attempt, and raises the safepoint pain budget (painbudget=100) so a loaded box cannot freeze the search engine for the whole tail window.
Trim comments across the branch to non-obvious intents and whys: remove ticket/plan references, development-history wording, duplicated rationale blocks, restatements of adjacent code, and dangling half-written fragments; fix wrong cross-references. No code changes.
- Serialize leakTagIncarnation into HeapLiveObject and ReferenceChain so consumers join on (tag, stamp) across chunks; tags alone are recycled - Erase chains whose leak-tag owner died during drain and reject them in cacheResolvedChain: a stamp mismatch can never become valid again - Revert test seams to package-private; forward through ReferenceChainTestHooks in ddprof-test; restore the not-read-only warning on shouldRunPassForTest0 - Count drained chains per leaf class (drainReferenceChainEventCountForLeaf0) - Accept debug and *san builds for test-hook gating (asan/tsan nightlies) - Fix cache-leak test flake: persistent allocator thread, matched-scale seeds - Verify owner identity in leak-tag correlation assertions - Process-test hardening: bounded waits, synchronized log tail, child timeout budget, destroy on interrupt; copyright header
Both the referenceChainJfrRoundtrip_ut gtest and the ddprof-test ReferenceChainJfrParserTest get the shared .jfr path from DDPROF_REFERENCE_CHAIN_ROUNDTRIP_JFR; the producer deletes the file before running so the consumer never parses a stale recording.
The shared path let a multi-configuration invocation delete or overwrite another configuration's recording; both producer and consumer now derive the configuration from the task name. Also fail the build when no gtest*_referenceChainJfrRoundtrip_ut task matches, instead of the parser test silently degrading to a skip.
jbachorik
force-pushed
the
jb/rc-6-java-tests
branch
from
October 19, 2026 10:53
afb4457 to
981568f
Compare
Collaborator
Author
|
@DataDog review |
There was a problem hiding this comment.
The new allocation helper can discard completion signals when its worker finishes before the caller starts waiting, causing intermittent 30-second integration-test failures. A buffered completion queue preserves both task results and shutdown acknowledgements.
🤖 Bits Code Review · Commit 981568f
A SynchronousQueue drops the worker's bare offer() when the batch finishes between the handoff rendezvous and the caller entering poll(), turning a completed batch (or its rethrown failure) into a 30-second poll timeout. A one-slot ArrayBlockingQueue retains it; the protocol is single-in-flight so one slot suffices.
bumpLeakTagIncarnationLocked() read _leak_tag_incarnation with a plain read while lock-free acquire readers race it; use an atomic load for the read half of the release-store bump. Add a heapFloorResetForTest() seam (empty head/fill, storeRelease like the producer) with JNI wiring, so the AggressiveLeak tests can clear the seeded ring instead of leaving ~991MB samples for later tests in the shared JVM. Correct getTid()'s javadoc: getTid0() is compiled unconditionally (outside javaApi.cpp's #ifdef DEBUG seam block) and works in release builds too.
Expose cacheResolvedChain()'s verdict through the test accessor and assert dead-owner chains are refused (also after the tag is recycled for a new owner with the dead owner's stamp). Add FrontierChainFromSupersededSearchIsRejected: a frontier chain cached by an earlier search must not answer the seam after a restart bumps _search_start_ns, and nothing answers with no active search. Add GenerationChangeClearsCachedKlassIdsWholeTable: every row below _table_size clears its cached class id on a class-map generation change and a canary row at _table_size stays untouched, pinning the loop bound.
PersistentAllocator.run() clears the completion slot before the handoff so a stale outcome from a timed-out batch can never be attributed to the next task (the worker is parked at handoff.take() at that point). The AggressiveLeak tests reset the heap-floor ring in finally before re-enabling recording. ThreadLocalLeakScenario: keepBest keeps an in-pool chain over a later out-of-pool one, and an unresolved -1 incarnation fails loud or is a hard mismatch instead of degrading to tag-only correlation. LeakTagCorrelationScenario: keepBest ranks scans by the same evidence score scan() uses (owner-verified 2, in-pool 1), so later degraded scans cannot displace the best correlation.
The roundtrip gtest writes its seeded values (class names, edge labels, target tag, depth, incarnation) to a .expected sidecar next to the JFR and the Java parser test asserts against it, removing the duplicated hard-coded expectations across the two modules. Add ReferenceChainAssertions.requiredTestConfig(): child-JVM launchers fail loud when ddprof_test.config is unset instead of forwarding the literal null that the child would classify as a release build. ExternalProcessReferenceChainTest derives its expected seam path from isDebugInstrumentedBuild(), the same single encoding the child uses.
heapFloorResetForTest() published two independent storeReleases, so a concurrent reader could observe a torn old-fill/new-head window, and no store ordering closes that (seeing the first store's new value while the second is still old is possible either way). The documented precondition - heap-floor recording disabled - is what excludes every reader, so enforce it in DEBUG builds (where the flag exists): reset while recording is enabled fails an assert instead of racing silently. Expose heapFloorRisingForTest0() (JNI + hooks) and add shouldEmptyHeapFloorRingOnReset(): rising-then-empty through the real heapFloorRising() read, killing a no-op'd reset that previously survived the whole suite unobserved.
completion.clear() before the handoff was a timing-only guard: after a run() that timed out, the worker is not parked at handoff.take() yet, so the abandoned task's outcome could land after the clear and be attributed to the next task. Stamp each run with a monotonic generation carried through the Job/Outcome pair and discard stale generations at poll time; the stale entry is always offered before the current one's verdict because the worker offers it before parking at take(), through which alone the new Job is accepted.
Both leak-correlation scenarios silently degraded a missing leakTag (or, in LeakTagCorrelation, leakTagIncarnation) accessor to -1, yielding an empty correlation with no diagnostic - the failure mode the fail-loud guards were added to prevent. Both scenarios now throw when either identity accessor is absent. Consolidate LeakTagCorrelationScenario's owner-verified/in-pool/out-of-pool ranking into evidenceRank() (used by evidenceScore(), the within-scan candidate selection, and cross-referenced by the marker cascade) so the three sites cannot drift apart. Document that ThreadLocalLeakScenario's keepBest has nothing to merge from a losing scan (no transient/noise evidence fields in that scenario).
start() rebuilds the leak-tag pool by comparing every _leak_tag_in_use[i] against the table's owners; the singleton is static-storage and zero-initialized regardless, but a non-statically constructed tracker would read indeterminate bools there. Value-init the array in the constructor's init list, matching _leak_tag_incarnation.
The worker's completion.offer() could silently drop THIS task's own outcome: with one slot, a stale entry left by an abandoned (timed-out) run() can still occupy the queue when the current task finishes, and the dropped outcome turns a completed task into the caller's 30s poll timeout. Use put() instead - the caller's generation loop drains the stale entry, unblocking the worker; the same applies to close()'s exit acknowledgement, which close()'s drain unblocks. keepBest(): drop the dead other != null check (both scans are non-null after the early returns). The roundtrip sidecar readers now name the offending key and value instead of surfacing a bare NumberFormatException (or "null" for a missing key).
Code bodies referenced java.util.Properties,
org.openjdk.jmc.common.{item,unit} types, java.lang.reflect.Method and
java.lang.management.ManagementFactory inline; import them and use the
simple names instead. Javadoc prose and message strings naming JDK
internals (chain path descriptions) are untouched.
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?:
Java integration-test layer for reference chains, plus a production identity fix for recycled leak tags discovered by hardening tests.
Production: recycled leak-tag identity
Problem. Leak tags are recycled once their object dies. A cached chain matched by tag alone can belong to a previous owner of that tag, so the
ReferenceChain.targetTag → HeapLiveObject.leakTagjoin attributes a dead object's chain to the tag's new owner.Fix.
Related fix. Adopting a class-map generation (the
start()rebuild and the population-reset seam) invalidates the tracking table's cached class IDs. IDs from the wiped dictionary namespace would alias unrelated classes.Test seams (DEBUG builds only)
All serialized on
_engine_lock, so they never observe the BFS thread mid-mutation:The heap-floor ring (the aggregate heap-usage history behind the urgent-OOM gate) has its own lock-free seeding seams outside the engine lock: seed samples, fake max heap, suppress real-GC recording, and reset the ring between tests. The reset refuses to run while recording is enabled — the two index stores are not atomic as a pair, so the recording-disabled precondition is the only thing that excludes a concurrent reader.
Test layer
All assertions go through the public profiler API:
The scenario infrastructure fails loud on absent identity fields (a missing
leakTag/leakTagIncarnationaccessor throws instead of degrading the correlation to tag-only matching) and attributes persistent-allocator outcomes per run via a generation token, so an abandoned batch's result can never be attributed to the next one.Motivation:
The engine and JFR layers landed in earlier PRs with unit tests only. The recycled-tag identity problem surfaced while writing the correlation tests: a pass built on numeric tag equality can match another object's chain. That is why the fix is production code, not test scaffolding.
Additional Notes:
painbudget=100). The production default (5% of wall-clock) can starve the search engine on a loaded host for longer than the scenario's observation window, which made the test a load lottery.NativeThreadTest,DynamicNativeThread,ThreadEntryDetectionTest,VtableReceiverFrameTest) are unrelated and reproduce on a clean tree.How to test the change?:
ReferenceChainJfrParserTestassumes the roundtrip gtest's recording; run:ddprof-lib:gtestDebug_referenceChainJfrRoundtrip_utfirst (or any fulltestDebug, which orders the gtests ahead of the Java tests), otherwise the test is skipped with an assumption.For Datadog employees: