Replace JMC's JFR loader with a lightweight, streaming jafar-backed API - #719
Merged
Conversation
…ents API JMC's loader can hang forever if an internal parallel chunk-parsing worker dies from OOM (root cause of BoundMethodHandleProfilerTest CI hangs); jafar's synchronous UntypedJafarParser surfaces failures instead. JfrEvents.load() collapses jafar's resolved values in place and supports an early-exit predicate so verifyStackTraces() stops parsing once every pattern is matched, instead of materializing the whole event set. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
BoundMethodHandleProfilerTest, NativeLibrariesTest and NativememSampledProfilerTest OOM'd the 512MB test heap because they collected every matching event into a list just to run per-event checks/counts or a presence check. Adds JfrEvents.forEach/reduce and a stopWhen-aborting presence check, and switches these three tests to them, so at most one event is resolved in memory at a time. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
CI Test ResultsRun: #31101754038 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-08-06 12:48:16 UTC |
Merging origin/main pulled in this test written against the old JMC-based verifyEvents(), which no longer compiles now that it returns JfrEvents. Uses streamEvents() instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Benchmark Results (commit 476968e)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/129184073 Commit: ✅ Within expected boundariesNo significant runtime deltas (all within run-to-run noise) and no internal-counter outliers. Runtime details (per benchmark × JDK)
Internal counter details (ddprof)ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
|
JVM-internal slash-separated names broke tests comparing against getClass().getName(), failing CI across the full test matrix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
|
Bits Code status: ✅ Done Comment @DataDog to request changes |
Contributor
Benchmark Results (commit 1813c79)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/129338346 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10354 ms (21 iters) | ✅ 10275 ms (21 iters) | ≈ -0.8% (±10.5%) | — / — |
| akka-uct | 25 | ✅ 8787 ms (24 iters) | ✅ 8959 ms (24 iters) | ≈ +2% (±9.1%) | — / — |
| finagle-chirper | 21 | ✅ 5957 ms (33 iters) | ✅ 5924 ms (33 iters) | ≈ -0.6% (±25.2%) | |
| finagle-chirper | 25 | ✅ 5392 ms (36 iters) | ✅ 5397 ms (36 iters) | ≈ +0.1% (±23.8%) | |
| fj-kmeans | 21 | ✅ 2816 ms (66 iters) | ✅ 2811 ms (66 iters) | ≈ -0.2% (±2.7%) | — / — |
| fj-kmeans | 25 | ✅ 2768 ms (68 iters) | ✅ 2831 ms (66 iters) | ≈ +2.3% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2120 ms (88 iters) | ✅ 2123 ms (88 iters) | ≈ +0.1% (±2.7%) | — / — |
| future-genetic | 25 | ✅ 1896 ms (98 iters) | ✅ 1955 ms (96 iters) | 🔴 +3.1% | — / — |
| naive-bayes | 21 | ✅ 1270 ms (134 iters) | ✅ 1291 ms (133 iters) | ≈ +1.7% (±32.8%) | — / — |
| naive-bayes | 25 | ✅ 1018 ms (168 iters) | ✅ 1001 ms (171 iters) | ≈ -1.7% (±31.2%) | — / — |
| reactors | 21 | ✅ 16420 ms (15 iters) | ✅ 16111 ms (15 iters) | ≈ -1.9% (±7.9%) | — / — |
| reactors | 25 | ✅ 18699 ms (15 iters) | ✅ 18937 ms (15 iters) | ≈ +1.3% (±4%) | — / — |
Internal counter details (ddprof)
ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
| Benchmark | JDK | Dropped rec | Dropped jvmti | Dropped trace | Skipped WC | AGCT fail | Unwind fail |
|---|---|---|---|---|---|---|---|
| akka-uct | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 2070 / 1989 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 3 / ✅ | 2171 / 2223 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 3 | 8122 / 8221 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 7 / 6 | 1260 / 1260 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 1285 / 1265 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 4 / ✅ | 3013 / 3057 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 2940 / 2889 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 4 / 3 | 3480 / 3538 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 4 / 4 | 3506 / 3498 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 2 | 1649 / 1651 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 1951 / 1935 | ✅ / ✅ | ✅ / ✅ |
…g them
AllocationProfilerTest, CTimerSamplerTest, MegamorphicCallTest, GCGenerationsTest,
and MemleakProfilerTest each materialized every matching JFR event (deep-resolved,
full stack traces) into memory, which OOMs the test heap under heavy sampling load.
Also fixes AllocationProfilerTest comparing array class names against
Class.getCanonicalName() ("int[]") instead of getClassName()'s Class.getName()-style
format ("[I"), which silently zeroed out recorded allocation sizes.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Benchmark Results (commit 4a1a732)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/129359134 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10289 ms (21 iters) | ✅ 10222 ms (21 iters) | ≈ -0.7% (±10%) | — / — |
| akka-uct | 25 | ✅ 8790 ms (24 iters) | ✅ 8895 ms (24 iters) | ≈ +1.2% (±9.7%) | — / — |
| finagle-chirper | 21 | ✅ 5974 ms (33 iters) | ✅ 6014 ms (33 iters) | ≈ +0.7% (±25.3%) | |
| fj-kmeans | 21 | ✅ 2827 ms (66 iters) | ✅ 2827 ms (66 iters) | ≈ 0% (±2.5%) | — / — |
| fj-kmeans | 25 | ✅ 2777 ms (67 iters) | ✅ 2827 ms (66 iters) | ≈ +1.8% (±2.7%) | — / — |
| future-genetic | 21 | ✅ 2051 ms (91 iters) | ✅ 2134 ms (87 iters) | 🔴 +4% | — / — |
| future-genetic | 25 | ✅ 1951 ms (95 iters) | ✅ 1897 ms (97 iters) | 🟢 -2.8% | — / — |
| naive-bayes | 21 | ✅ 1305 ms (131 iters) | ✅ 1261 ms (136 iters) | ≈ -3.4% (±32%) | — / — |
| naive-bayes | 25 | ✅ 1016 ms (168 iters) | ✅ 1006 ms (169 iters) | ≈ -1% (±31.1%) | — / — |
| reactors | 21 | ✅ 16098 ms (15 iters) | ✅ 16291 ms (15 iters) | ≈ +1.2% (±7.7%) | — / — |
| reactors | 25 | ✅ 18510 ms (15 iters) | ✅ 18456 ms (15 iters) | ≈ -0.3% (±5.6%) | — / — |
Internal counter details (ddprof)
ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
| Benchmark | JDK | Dropped rec | Dropped jvmti | Dropped trace | Skipped WC | AGCT fail | Unwind fail |
|---|---|---|---|---|---|---|---|
| akka-uct | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 1992 / 1899 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 2274 / 2380 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 6 / 4 | 8805 / 8745 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 8626 / 8103 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 1275 / 1277 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 1273 / 1275 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 3067 / 2940 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 2952 / 2841 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 6 / 6 | 3510 / 3507 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 4 | 3489 / 3459 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 1707 / 1762 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 1865 / 1839 | ✅ / ✅ | ✅ / ✅ |
…vadoc Use verifyEventPresent instead of streamEvents+assert for presence-only checks, drop dead allocation-comparison code left over from the materialization fix, and correct a verifyEvents javadoc overclaim about stack traces. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jbachorik
marked this pull request as ready for review
August 6, 2026 10:45
![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: 77ba5ae6f0
ℹ️ 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".
Contributor
Benchmark Results (commit 77ba5ae)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/129366368 Commit: ✅ Within expected boundariesNo significant runtime deltas (all within run-to-run noise) and no internal-counter outliers. Runtime details (per benchmark × JDK)
Internal counter details (ddprof)ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
|
…sing headers/javadoc JfrEvents.load/forEach/reduce read events from jafar's per-chunk parser callbacks, which run concurrently across chunks for multi-chunk recordings; synchronize the shared list/counter/accumulator mutations to avoid lost updates and nondeterministic verification. getStackTraceString appended a trailing newline after the last frame, breaking endsWith-style checks. Also adds missing copyright headers and public-method javadoc across the touched test files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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?:
Replaces JMC's
FlightRecordingLoader(used across theddprof-testsuite to parse and assert on JFR recordings) with a smallJfrEvent/JfrEventsAPI backed byio.btrace:jafar-parser.JfrEventsoffers a materializingload()for callers that need the full collection, plus streamingforEach/reduceand an early-exit presence check for callers that only need per-event checks, counts, or presence — these resolve at most one event in memory at a time instead of collecting every match into a list.Motivation:
JMC's loader parses chunks on an internal thread pool via an
ExecutorCompletionService; if a worker dies from an uncaughtOutOfMemoryErrormid-parse, the main thread'stake()blocks forever waiting for a result that never arrives. jafar'sUntypedJafarParser.run()is synchronous and single-threaded from the caller's perspective, so a worker failure surfaces as a propagated exception instead of a silent hang.The streaming variants exist because some tests only need per-event checks/counts or a plain presence check against a high-volume event type — materializing the full collection for that wastes memory for no benefit.
Additional Notes:
None.
How to test the change?:
Existing
ddprof-testsuite exercises this end-to-end (every test that asserts on JFR recording contents goes throughJfrEvents).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.Unsure? Have a question? Request a review!