fix(jfr): skip emitting empty constant pools#603
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
This PR updates the JFR checkpoint constant-pool (cpool) emission logic to avoid writing empty (0-element) constant-pool sections, which strict JFR readers reject. It does so by making each variable-pool writer return whether it actually emitted a pool section, writing a placeholder pool-count byte in writeCpool, and then back-patching the real pool count in finishChunk() using pwrite() after the buffer has been flushed.
Changes:
- Make variable pool writers (
writeThreads,writeStackTraces,writeMethods,writeClasses,writePackages, andwriteConstantPoolSectionoverloads) return0/1depending on whether a pool section was emitted. - Emit a 1-byte placeholder for the constant pool count in
writeCpool, compute the actual count as pools are emitted, and return it. - Back-patch the real constant pool count in
finishChunk()flush-safe viapwrite(), mirroring the existing cpool size patch behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ddprof-lib/src/main/cpp/flightRecorder.h | Updates writer method signatures and documents new “skip empty pool + return emitted count” semantics. |
| ddprof-lib/src/main/cpp/flightRecorder.cpp | Implements skipping empty pool sections, dynamic pool counting with placeholder + pwrite() back-patch, and reorders writeLogLevels among always-present pools. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Reliability & Chaos Results❌ 24 failure(s) detected Pipeline: https://gitlab.ddbuild.io/DataDog/java-profiler/-/pipelines/119645822 ❌ chaos: profiler gmalloc aarch64 21 0 3 temXchaos❌ chaos: profiler gmalloc aarch64 25 0 3 temXchaos❌ chaos: profiler gmalloc amd64 21 0 3 temXchaos❌ chaos: profiler gmalloc amd64 25 0 3 temXchaos❌ chaos: profiler jemalloc aarch64 21 0 3 temXchaos❌ chaos: profiler jemalloc aarch64 25 0 3 temXchaos❌ chaos: profiler jemalloc amd64 21 0 3 temXchaos❌ chaos: profiler jemalloc amd64 25 0 3 temXchaos❌ chaos: profiler tcmalloc aarch64 21 0 3 temXchaos❌ chaos: profiler tcmalloc aarch64 25 0 3 temXchaos❌ chaos: profiler tcmalloc amd64 21 0 3 temXchaos❌ chaos: profiler tcmalloc amd64 25 0 3 temXchaos❌ chaos: profiler tracer gmalloc aarch64 21 0 3 temXchaos❌ chaos: profiler tracer gmalloc aarch64 25 0 3 temXchaos❌ chaos: profiler tracer gmalloc amd64 21 0 3 temXchaos❌ chaos: profiler tracer gmalloc amd64 25 0 3 temXchaos❌ chaos: profiler tracer jemalloc aarch64 21 0 3 temXchaos❌ chaos: profiler tracer jemalloc aarch64 25 0 3 temXchaos❌ chaos: profiler tracer jemalloc amd64 21 0 3 temXchaos❌ chaos: profiler tracer jemalloc amd64 25 0 3 temXchaos❌ chaos: profiler tracer tcmalloc aarch64 21 0 3 temXchaos❌ chaos: profiler tracer tcmalloc aarch64 25 0 3 temXchaos❌ chaos: profiler tracer tcmalloc amd64 21 0 3 temXchaos❌ chaos: profiler tracer tcmalloc amd64 25 0 3 temXchaos |
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
CI Test ResultsRun: #27785068042 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 30 | Failed: 0 | Cancelled: 2 Updated: 2026-06-18 22:50:50 UTC |
|
@copilot resolve the merge conflicts in this pull request |
Merged |
rkennke
left a comment
rkennke
left a comment
There was a problem hiding this comment.
One minor thing, up to you to fix or not.
What might be useful (maybe as a follow-up?) is a regression test. A gtest that produces a chunk with empty variable pools and asserts (a) the back-patched count equals the number of emitted sections and (b) no empty section is present would be sufficient.
| _thread_ids[i][old_index].clear(); | ||
| } | ||
|
|
||
| if (threads.empty()) { |
There was a problem hiding this comment.
This is effectively dead code, right? Because we do threads.insert(_tid) just a few lines up. This also means we have 5 non-empty cpools, not 4.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| String idStr = el.attrs.get("id"); | ||
| if (idStr != null) { | ||
| ClassDef cd = new ClassDef(); | ||
| cd.id = Long.parseLong(idStr); |
| if ("field".equals(child.name)) { | ||
| FieldDef fd = new FieldDef(); | ||
| fd.name = child.attrs.get("name"); | ||
| fd.typeId = Long.parseLong(child.attrs.get("class")); |
| fd.typeId = Long.parseLong(child.attrs.get("class")); | ||
| fd.constantPool = "true".equals(child.attrs.get("constantPool")); | ||
| String dim = child.attrs.get("dimension"); | ||
| fd.dimension = dim != null ? Integer.parseInt(dim) : 0; |
Strict JFR readers (OpenJDK jfr, JMC, Datadog backend) reject a checkpoint
declaring a constant pool with zero elements ("Pool ... must contain at
least one element"). The writers always emitted every pool with a
hard-coded count of 12, so a chunk produced before any string label /
context attribute / package existed was unreadable.
Each variable pool writer now skips emission and returns 0 when empty.
writeCpool writes a 1-byte placeholder pool-count and sums the emitted
pools; finishChunk back-patches the real count via pwrite after flush,
mirroring the existing cpool size patch.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…de in writeThreads writeThreads always emits (_tid inserted unconditionally), so the threads.empty() guard was dead code and the pool count baseline was wrong (4 instead of 5). Move writeThreads to the constant-pool group and start pool_count at 5. Add EmptyConstantPoolTest: parses raw JFR binary and asserts no pool section has zero elements and pool count >= 5 in both early and active-profiling dumps. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jbachorik
force-pushed
the
jb/jfr-skip-empty-pools
branch
from
5cb40ce to
70e3534
Compare
What does this PR do?:
Stops the JFR checkpoint from emitting empty (0-element) constant-pool sections. Each variable pool writer (
writeThreads,writeStackTraces,writeMethods,writeClasses,writePackages, and the threewriteConstantPoolSectionoverloads) now skips emission and returns 0 when its pool is empty.writeCpoolwrites a 1-byte placeholder pool-count and sums the actually-emitted pools;finishChunkback-patches the real count viapwriteafter flush, mirroring the existing cpool size patch.writeLogLevelsis moved up alongside the other always-present constant pools.Motivation:
OpenJDK
jfrrejects a checkpoint that declares a constant pool with zero elements:The previous code hard-coded the pool count to
12and always emitted every pool, so a chunk produced before any string label / context attribute / package existed contained an empty pool and was unreadable by these tools.Additional Notes:
Stacked on #602; rebase to
mainafter it merges. The diff here is only the empty-pool skipping + flush-safe count back-patch (Set B); the method-pool id allocator lives in #602.The count cannot be precomputed: the Method/Class/Package/Symbol pools are only fully populated as a side effect of
writeStackTraces/writeMethods, and empty variable pools are skipped. Hence the placeholder + back-patch.How to test the change?:
Validated with OpenJDK
jfr printon recordings produced by theMethodIdReuseTestdump driver.Before (build without this change):
After (this change):
Local results (debug build, macOS):
:ddprof-lib:buildDebug -Pskip-gtest→ BUILD SUCCESSFUL:ddprof-lib:gtestDebug_methodMapId_ut→[ PASSED ] 5 tests(no regression)jfr print --events 'datadog.*'on a produced recording → exit 0 (was exit 1 /must contain at least one elementbefore)For Datadog employees:
If this PR touches code that signs or publishes builds or packages, or handles
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.This PR doesn't touch any of that.
JIRA: PROF-15130
🤖 Generated with Claude Code