JFR event types and emission for reference chains - #796
Conversation
Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||
CI Test ResultsRun: #35741567290 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-09-22 14:59:11 UTC |
|
❌ 39 passed, 1 failed out of 40 configurations Test Matrix
Failure Detailsmusl-x64-hotspot-jdk21Tracer+profiler: Links
|
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. |
ab3b0eb to
4534a7f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab3b0ebbe1
ℹ️ 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.
Normal liveness events can write an unknown leak tag. A long reference chain can also exceed the fixed JFR buffer and damage memory.
🤖 Datadog Autotest · Commit ab3b0eb · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
4534a7f to
71320e1
Compare
The null-pointer store is the deliberate never-returning crash, but clang scan-build reports a null dereference. Under __clang_analyzer__, replace it with __builtin_trap() - the analyzer understands the trap as a halt and explores no false path past it; the real build keeps the store.
Introduces the ReferenceChainEvent/ReferenceChainAbandonedEvent payloads (event.h), their JFR metadata (jfrMetadata.*), and the FlightRecorder emission paths that serialize chain events into JFR recording buffers, including the constant-pool handling for per-hop edge labels. Emission is pull-style: profiler.cpp snapshots events and hands them to FlightRecorder; this layer does not depend on the tracker itself.
71320e1 to
80173ad
Compare
Review findings on the JFR plumbing layer: - MAX_REFERENCE_CHAIN_EVENT_HOPS was a fixed 4096, permitting a ~438 KB worst-case event (near-limit edge labels) against a ~61 KB recording buffer - the reservation flushed first but the margin underflowed, so the write ran past the buffer (debug assert, release corruption). The cap is now derived from RECORDING_BUFFER_LIMIT minus the event's fixed fields, divided by the per-hop worst case, so a full-cap event always fits. - Truncation dropped ALL edge labels: the label count was gated on _edges.size() == emitted_size, which only holds for untruncated chains. Labels align with the chain's leaf-first element order, so truncation now emits the first emitted_size labels and loses only the root-side ones. - ObjectLivenessEvent::leak_tag is default-initialized to 0 so any construction path that forgets to set it serializes a defined untagged value (flush_table() overwrites it from the entry, which track() zeroes at insert). Moves the JFR round-trip and arguments parsing unit tests into this layer (they test exactly this code), rewrites the round-trip test to construct events directly instead of through the tracker, and adds byte-level boundary tests: oversize-chain truncation with label preservation, the size-prefix invariant, and the default leak tag.
d51dfb8 to
8b7de9c
Compare
rkennke
left a comment
There was a problem hiding this comment.
Nice change. I did a manual review, see below some findings.
Uncommitted plan documents, rotted .cpp:NNN line references, and a nonexistent j9WallClock.cpp path replaced with symbol references that stay valid as the code moves.
rkennke
left a comment
There was a problem hiding this comment.
🗿 🤖 🔴
Sphinx Review found 1 critical/high severity finding(s) that must be addressed.
8b7de9c to
e3b3f66
Compare
- ReferenceChainEvent carries one vector of ReferenceChainHop (klass id + retention-edge label) instead of two parallel vectors - Compress the sub-option floor/ceiling rationale and the provisional default constant comments to one concise statement each - Drop design-doc and Jira references from code comments; revert the unrelated LineNumberTable comment rewrite
❌ ErrorsYour PR has failed checks. Please review the issues below and take necessary action before merging. 🚦 2 Pipeline jobs failed
Useful? React with 👍 / 👎 This comment will be updated automatically if new data arrives.🔗 Commit SHA: d23c6b6 | Docs | View more details | Give us feedback! |
kaahos
left a comment
There was a problem hiding this comment.
Nice work, thanks! I've suggested 1 potential optimization and left 1 minor comment but nothing blocking for me.
@kaahos Thanks! I applied the suggestions - they were all valuable! |
Are you sure you pushed those fixes? I don't see them. |
rkennke
left a comment
There was a problem hiding this comment.
Looks good. You might have forgotten to push some fixes (see other comment).
|
@rkennke Yep. The signature window timed out and the push was just abandoned. Repushed. Thanks for pointing that out! |
The event/argument comments named collector classes, methods and files that do not exist at this layer of the stack; describe the contracts without those forward references instead.
500a7ea to
d23c6b6
Compare
* Model crashNow's intentional crash as a trap under scan-build The null-pointer store is the deliberate never-returning crash, but clang scan-build reports a null dereference. Under __clang_analyzer__, replace it with __builtin_trap() - the analyzer understands the trap as a halt and explores no false path past it; the real build keeps the store. * Add JFR event types for reference chains Introduces the ReferenceChainEvent/ReferenceChainAbandonedEvent payloads (event.h), their JFR metadata (jfrMetadata.*), and the FlightRecorder emission paths that serialize chain events into JFR recording buffers, including the constant-pool handling for per-hop edge labels. Emission is pull-style: profiler.cpp snapshots events and hands them to FlightRecorder; this layer does not depend on the tracker itself. * Keep chain events inside the recording buffer; keep truncated labels Review findings on the JFR plumbing layer: - MAX_REFERENCE_CHAIN_EVENT_HOPS was a fixed 4096, permitting a ~438 KB worst-case event (near-limit edge labels) against a ~61 KB recording buffer - the reservation flushed first but the margin underflowed, so the write ran past the buffer (debug assert, release corruption). The cap is now derived from RECORDING_BUFFER_LIMIT minus the event's fixed fields, divided by the per-hop worst case, so a full-cap event always fits. - Truncation dropped ALL edge labels: the label count was gated on _edges.size() == emitted_size, which only holds for untruncated chains. Labels align with the chain's leaf-first element order, so truncation now emits the first emitted_size labels and loses only the root-side ones. - ObjectLivenessEvent::leak_tag is default-initialized to 0 so any construction path that forgets to set it serializes a defined untagged value (flush_table() overwrites it from the entry, which track() zeroes at insert). Moves the JFR round-trip and arguments parsing unit tests into this layer (they test exactly this code), rewrites the round-trip test to construct events directly instead of through the tracker, and adds byte-level boundary tests: oversize-chain truncation with label preservation, the size-prefix invariant, and the default leak tag. * Drop transient and stale line-number references from comments Uncommitted plan documents, rotted .cpp:NNN line references, and a nonexistent j9WallClock.cpp path replaced with symbol references that stay valid as the code moves. * Address review: merge chain hops, trim comments, drop doc/jira refs - ReferenceChainEvent carries one vector of ReferenceChainHop (klass id + retention-edge label) instead of two parallel vectors - Compress the sub-option floor/ceiling rationale and the provisional default constant comments to one concise statement each - Drop design-doc and Jira references from code comments; revert the unrelated LineNumberTable comment rewrite * Make comments layer-local: no references to later stack layers The event/argument comments named collector classes, methods and files that do not exist at this layer of the stack; describe the contracts without those forward references instead. * Drop a later-layer class name from the arguments test comment * Clamp reference-chain edge labels to the reserved cap when writing * Reuse reservation constants and drop edge_labels stack array in recordReferenceChain
What does this PR do?:
Adds the JFR plumbing for reference-chain events: the
ReferenceChainEvent/ReferenceChainAbandonedEventpayloads (event.h), their dynamic JFR metadata and constant-pool handling (jfrMetadata.*), the FlightRecorder emission paths that serialize chain events and per-hop edge labels into recording buffers (flightRecorder.*), and thereferencechainsArgumentsblock (arguments.*) that configures the feature.Motivation:
Part 2 of the stacked series for reference chains (PROF-15341). Emission is pull-style — the later tracker layer snapshots events and hands them to FlightRecorder — so this layer is independently reviewable and compiles/links without the tracker.
Additional Notes:
Stacked series for PROF-15341. Merge order: #804 (scan-build crashNow fix) -> this PR -> #797 -> #798 -> #799 -> #800 -> #801 -> #802. #795 (UBSan ELF fix) is already merged. Until #804 lands, this PR's diff additionally shows the small faultInjection.cpp change from #804.
How to test the change?:
The JFR round-trip unit tests for chain events land in the gtest PR later in this stack;
buildDebugcompiles and links this layer standalone.For Datadog employees: