diff --git a/ddprof-lib/src/main/cpp/flightRecorder.cpp b/ddprof-lib/src/main/cpp/flightRecorder.cpp index 308993ec5..f368f1deb 100644 --- a/ddprof-lib/src/main/cpp/flightRecorder.cpp +++ b/ddprof-lib/src/main/cpp/flightRecorder.cpp @@ -761,7 +761,8 @@ off_t Recording::finishChunk(bool end_recording, bool do_cleanup) { } off_t cpool_offset = lseek(_fd, 0, SEEK_CUR); - writeCpool(_buf); + int count_offset_in_cpool = 0; + int pool_count = writeCpool(_buf, &count_offset_in_cpool); flush(_buf); off_t cpool_end = lseek(_fd, 0, SEEK_CUR); @@ -771,6 +772,13 @@ off_t Recording::finishChunk(bool end_recording, bool do_cleanup) { ssize_t result = pwrite(_fd, _buf->data(), 5, cpool_offset); (void)result; + // Patch the constant pool count placeholder (written as a 1-byte put8 in + // writeCpool). Done flush-safe via pwrite to the FILE offset, mirroring the + // size patch above: _buf has been flushed/reset, so _buf->data() is scratch. + _buf->put8(0, (char)pool_count); + result = pwrite(_fd, _buf->data(), 1, cpool_offset + count_offset_in_cpool); + (void)result; + off_t chunk_end = lseek(_fd, 0, SEEK_CUR); // // Workaround for JDK-8191415: compute actual TSC frequency, in case JFR is @@ -1359,15 +1367,24 @@ void Recording::writeNativeLibraries(Buffer *buf) { _recorded_lib_count = native_lib_count; } -void Recording::writeCpool(Buffer *buf) { +int Recording::writeCpool(Buffer *buf, int *count_offset_in_cpool) { + // Offset of the cpool start within the buffer. The header below is tiny and + // flush-free, so the placeholder offset captured relative to this start is a + // stable cpool-relative offset usable for a flush-safe back-patch by the + // caller (mirrors the cpool SIZE patch). + int cpool_start = buf->offset(); buf->skip(5); // size will be patched later buf->putVar64(T_CPOOL); buf->putVar64(_start_ticks); buf->put8(0); buf->put8(0); buf->put8(1); - // constant pool count - bump each time a new pool is added - buf->put8(12); + // Constant pool count. We cannot precompute it: the Method/Class/Package/Symbol + // pools are only fully populated as a side effect of writeStackTraces/writeMethods + // (fillJavaMethodInfo), and empty variable pools are skipped entirely. Write a + // 1-byte placeholder here and back-patch it flush-safe in the caller. + *count_offset_in_cpool = buf->offset() - cpool_start; + buf->put8(0); // Profiler::rotateDictsAndRun() rotates the three dictionaries before this // path runs, so classMap()->standby() returns an old-active snapshot stable @@ -1379,21 +1396,26 @@ void Recording::writeCpool(Buffer *buf) { // standby() captures the pre-rotation state which writeClasses extends. Lookup lookup(this, &_method_map, Profiler::instance()->classMap()); lookup.initClassCache(); + // CONSTANT pools: always non-empty, always emitted -> 5 sections. + // writeThreads always emits: it inserts _tid unconditionally before checking. writeFrameTypes(buf); writeThreadStates(buf); writeExecutionModes(buf); - writeThreads(buf); - writeStackTraces(buf, &lookup); - writeMethods(buf, &lookup); - writeClasses(buf, &lookup); - writePackages(buf, &lookup); - writeConstantPoolSection(buf, T_SYMBOL, &lookup._symbols); - writeConstantPoolSection(buf, T_STRING, - Profiler::instance()->stringLabelMap()->standby()); - writeConstantPoolSection(buf, T_ATTRIBUTE_VALUE, - Profiler::instance()->contextValueMap()->standby()); writeLogLevels(buf); + writeThreads(buf); + int pool_count = 5; + // VARIABLE pools: each returns 1 if emitted, 0 if empty (and thus skipped). + pool_count += writeStackTraces(buf, &lookup); + pool_count += writeMethods(buf, &lookup); + pool_count += writeClasses(buf, &lookup); + pool_count += writePackages(buf, &lookup); + pool_count += writeConstantPoolSection(buf, T_SYMBOL, &lookup._symbols); + pool_count += writeConstantPoolSection( + buf, T_STRING, Profiler::instance()->stringLabelMap()->standby()); + pool_count += writeConstantPoolSection( + buf, T_ATTRIBUTE_VALUE, Profiler::instance()->contextValueMap()->standby()); flushIfNeeded(buf); + return pool_count; } void Recording::writeFrameTypes(Buffer *buf) { @@ -1466,7 +1488,7 @@ void Recording::writeThreads(Buffer *buf) { // We flush from old_index (the previous active set) std::unordered_set threads; - threads.insert(_tid); + threads.insert(_tid); // always present: the recording thread itself for (int i = 0; i < CONCURRENCY_LEVEL; ++i) { // Collect thread IDs from the fixed-size table into the main set @@ -1512,14 +1534,22 @@ void Recording::writeThreads(Buffer *buf) { } } -void Recording::writeStackTraces(Buffer *buf, Lookup *lookup) { +int Recording::writeStackTraces(Buffer *buf, Lookup *lookup) { // Reset all referenced flags before processing for (MethodMap::iterator it = _method_map.begin(); it != _method_map.end(); ++it) { it->second._referenced = false; } + // Tracks how many traces were written so the empty pool can be skipped. + // Note: even with zero traces, the methods marking pass below must still run + // via processCallTraces, but no T_STACK_TRACE section is emitted in that case. + int trace_count = 0; // Use safe trace processing with guaranteed lifetime during callback execution - Profiler::instance()->processCallTraces([this, buf, lookup](const std::unordered_set& traces) { + Profiler::instance()->processCallTraces([this, buf, lookup, &trace_count](const std::unordered_set& traces) { + if (traces.empty()) { + return; + } + trace_count = (int)traces.size(); buf->putVar64(T_STACK_TRACE); buf->putVar64(traces.size()); for (std::unordered_set::const_iterator it = traces.begin(); @@ -1558,9 +1588,10 @@ void Recording::writeStackTraces(Buffer *buf, Lookup *lookup) { flushIfNeeded(buf); } }); // End of processCallTraces lambda + return trace_count > 0 ? 1 : 0; } -void Recording::writeMethods(Buffer *buf, Lookup *lookup) { +int Recording::writeMethods(Buffer *buf, Lookup *lookup) { MethodMap *method_map = lookup->_method_map; u32 marked_count = 0; @@ -1571,6 +1602,10 @@ void Recording::writeMethods(Buffer *buf, Lookup *lookup) { } } + if (marked_count == 0) { + return 0; + } + buf->putVar64(T_METHOD); buf->putVar64(marked_count); for (MethodMap::iterator it = method_map->begin(); it != method_map->end(); @@ -1587,9 +1622,10 @@ void Recording::writeMethods(Buffer *buf, Lookup *lookup) { flushIfNeeded(buf); } } + return 1; } -void Recording::writeClasses(Buffer *buf, Lookup *lookup) { +int Recording::writeClasses(Buffer *buf, Lookup *lookup) { DEBUG_ASSERT_NOT_IN_SIGNAL(); std::map classes; // standby() returns the dump buffer — the stable snapshot captured by @@ -1598,6 +1634,10 @@ void Recording::writeClasses(Buffer *buf, Lookup *lookup) { // cross-thread writers via waitForRefCountToClear() before returning. lookup->_classes->standby()->collect(classes); + if (classes.empty()) { + return 0; + } + buf->putVar64(T_CLASS); buf->putVar64(classes.size()); for (std::map::const_iterator it = classes.begin(); @@ -1610,12 +1650,17 @@ void Recording::writeClasses(Buffer *buf, Lookup *lookup) { buf->putVar64(0); // access flags flushIfNeeded(buf); } + return 1; } -void Recording::writePackages(Buffer *buf, Lookup *lookup) { +int Recording::writePackages(Buffer *buf, Lookup *lookup) { std::map packages; lookup->_packages.collect(packages); + if (packages.empty()) { + return 0; + } + buf->putVar32(T_PACKAGE); buf->putVar32(packages.size()); for (std::map::const_iterator it = packages.begin(); @@ -1624,10 +1669,14 @@ void Recording::writePackages(Buffer *buf, Lookup *lookup) { buf->putVar64(lookup->getSymbol(it->second)); flushIfNeeded(buf); } + return 1; } -void Recording::writeConstantPoolSection( +int Recording::writeConstantPoolSection( Buffer *buf, JfrType type, std::map &constants) { + if (constants.empty()) { + return 0; + } flushIfNeeded(buf); buf->putVar64(type); buf->putVar64(constants.size()); @@ -1639,20 +1688,21 @@ void Recording::writeConstantPoolSection( buf->putVar64(it->first); buf->putUtf8(it->second, length); } + return 1; } -void Recording::writeConstantPoolSection(Buffer *buf, JfrType type, - Dictionary *dictionary) { +int Recording::writeConstantPoolSection(Buffer *buf, JfrType type, + Dictionary *dictionary) { std::map constants; dictionary->collect(constants); - writeConstantPoolSection(buf, type, constants); + return writeConstantPoolSection(buf, type, constants); } -void Recording::writeConstantPoolSection(Buffer *buf, JfrType type, - StringDictionaryBuffer *buffer) { +int Recording::writeConstantPoolSection(Buffer *buf, JfrType type, + StringDictionaryBuffer *buffer) { std::map constants; buffer->collect(constants); - writeConstantPoolSection(buf, type, constants); + return writeConstantPoolSection(buf, type, constants); } void Recording::writeLogLevels(Buffer *buf) { diff --git a/ddprof-lib/src/main/cpp/flightRecorder.h b/ddprof-lib/src/main/cpp/flightRecorder.h index acb43da8c..63c8d8008 100644 --- a/ddprof-lib/src/main/cpp/flightRecorder.h +++ b/ddprof-lib/src/main/cpp/flightRecorder.h @@ -287,30 +287,37 @@ class Recording { void writeJvmInfo(Buffer *buf); void writeSystemProperties(Buffer *buf); void writeNativeLibraries(Buffer *buf); - void writeCpool(Buffer *buf); + // Writes the cpool checkpoint. Returns the number of pool sections actually + // emitted (empty variable pools are skipped) and reports the byte offset of + // the pool-count placeholder within the cpool via *count_offset_in_cpool, so + // the caller can back-patch it flush-safe alongside the cpool size field. + int writeCpool(Buffer *buf, int *count_offset_in_cpool); void writeFrameTypes(Buffer *buf); void writeThreadStates(Buffer *buf); void writeExecutionModes(Buffer *buf); + // writeThreads always emits: _tid is inserted unconditionally so the thread + // pool is never empty. The following variable-pool writers return 1 if a + // section was emitted, 0 if the pool was empty and skipped. void writeThreads(Buffer *buf); - void writeStackTraces(Buffer *buf, Lookup *lookup); + int writeStackTraces(Buffer *buf, Lookup *lookup); - void writeMethods(Buffer *buf, Lookup *lookup); + int writeMethods(Buffer *buf, Lookup *lookup); - void writeClasses(Buffer *buf, Lookup *lookup); + int writeClasses(Buffer *buf, Lookup *lookup); - void writePackages(Buffer *buf, Lookup *lookup); + int writePackages(Buffer *buf, Lookup *lookup); - void writeConstantPoolSection(Buffer *buf, JfrType type, - std::map &constants); + int writeConstantPoolSection(Buffer *buf, JfrType type, + std::map &constants); - void writeConstantPoolSection(Buffer *buf, JfrType type, - Dictionary *dictionary); - void writeConstantPoolSection(Buffer *buf, JfrType type, - StringDictionaryBuffer *buffer); + int writeConstantPoolSection(Buffer *buf, JfrType type, + Dictionary *dictionary); + int writeConstantPoolSection(Buffer *buf, JfrType type, + StringDictionaryBuffer *buffer); void writeLogLevels(Buffer *buf); diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/metadata/EmptyConstantPoolTest.java b/ddprof-test/src/test/java/com/datadoghq/profiler/metadata/EmptyConstantPoolTest.java new file mode 100644 index 000000000..3df18f0c2 --- /dev/null +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/metadata/EmptyConstantPoolTest.java @@ -0,0 +1,367 @@ +/* + * Copyright 2026 Datadog, Inc + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.datadoghq.profiler.metadata; + +import com.datadoghq.profiler.AbstractProfilerTest; +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Regression test for PROF-15130: the JFR checkpoint must not emit any constant-pool section with + * zero elements. OpenJDK {@code jfr print} rejects such a chunk with + * {@code "Pool X must contain at least one element"}. + * + *

The fix: each variable pool writer ({@code writeThreads}, {@code writeStackTraces}, etc.) + * skips emission when its pool is empty and returns 0; {@code writeCpool} sums the actually-emitted + * pool count and back-patches it into the checkpoint header after flushing. + * + *

Two scenarios are tested: + *

    + *
  1. An early dump (before any profiling samples are collected) — maximises the number of + * empty variable pools (stacktraces, methods, classes, etc.).
    2. + *
  2. A dump after a brief profiling window — exercises the back-patched count with a mix of + * emitted and skipped variable pools.
    3. + *
+ */ +public class EmptyConstantPoolTest extends AbstractProfilerTest { + + @Test + public void noEmptyPoolsInEarlyDump() throws Exception { + // dump immediately, before any samples can be collected + Path rec = Files.createTempFile("empty-cpool-early-", ".jfr"); + try { + dump(rec); + stopProfiler(); + assertNoEmptyPoolsAndCountConsistent(rec); + } finally { + Files.deleteIfExists(rec); + } + } + + @Test + public void noEmptyPoolsAfterBriefProfiling() throws Exception { + waitForProfilerReady(2000); + // a short busy-loop so at least some stacks are collected + long acc = 0; + long deadline = System.currentTimeMillis() + 300; + while (System.currentTimeMillis() < deadline) { + acc ^= deadline; + } + // consume acc to prevent DCE + if (acc == Long.MIN_VALUE) throw new IllegalStateException(); + + Path rec = Files.createTempFile("empty-cpool-active-", ".jfr"); + try { + dump(rec); + stopProfiler(); + assertNoEmptyPoolsAndCountConsistent(rec); + } finally { + Files.deleteIfExists(rec); + } + } + + @Override + protected String getProfilerCommand() { + return "cpu=1ms,wall=~1ms"; + } + + // ── oracle ───────────────────────────────────────────────────────────────── + + /** + * Walks every checkpoint in every chunk of the JFR file and asserts: + *
    + *
  1. Every declared pool section has element count {@code > 0}.
    2. + *
  2. The declared pool count in the checkpoint header equals the number of + * sections we can actually walk (i.e. the back-patch is consistent).
    3. + *
+ */ + static void assertNoEmptyPoolsAndCountConsistent(Path file) throws IOException { + byte[] all = Files.readAllBytes(file); + long pos = 0; + int chunksChecked = 0; + while (pos + 8 <= all.length) { + if (!(all[(int) pos] == 'F' && all[(int) pos + 1] == 'L' + && all[(int) pos + 2] == 'R' && all[(int) pos + 3] == 0)) { + break; + } + new ChunkChecker(all, (int) pos).assertPoolsValid(); + long chunkSize = beLong(all, (int) pos + 8); + assertTrue(chunkSize > 0, "chunk size must be positive"); + pos += chunkSize; + chunksChecked++; + } + assertTrue(chunksChecked > 0, "no JFR chunks found in recording"); + } + + // ── JFR chunk parser ─────────────────────────────────────────────────────── + + private static final class ChunkChecker { + private final byte[] f; + private final int base; + private final long chunkSize; + private final long cpOffset; + private final long metaOffset; + + // class id -> field layout (parsed from metadata) + private final Map classes = new HashMap<>(); + + ChunkChecker(byte[] f, int base) { + this.f = f; + this.base = base; + this.chunkSize = beLong(f, base + 8); + this.cpOffset = beLong(f, base + 16); + this.metaOffset = beLong(f, base + 24); + parseMetadata(); + } + + void assertPoolsValid() { + Set visited = new HashSet<>(); + long off = base + cpOffset; + while (off >= base && off < base + chunkSize && !visited.contains(off)) { + visited.add(off); + long[] p = {off}; + readVarLong(p); // size + readVarLong(p); // typeId (checkpoint event) + readVarLong(p); // start + readVarLong(p); // duration + long deltaRaw = readVarLong(p); + long delta = (deltaRaw >>> 1) ^ -(deltaRaw & 1); // zig-zag decode + p[0] += 1; // flush byte + long declaredPoolCount = readVarLong(p); + + boolean fullWalk = true; + for (long i = 0; i < declaredPoolCount; i++) { + long classId = readVarLong(p); + long count = readVarLong(p); + assertTrue(count > 0, + "constant pool section for classId=" + classId + + " has zero elements (empty pool was not skipped)"); + ClassDef cd = classes.get(classId); + if (cd == null) { + // Unknown layout — cannot safely skip entries; stop walking. + // We still checked count > 0 for this section. + fullWalk = false; + break; + } + for (long e = 0; e < count; e++) { + readVarLong(p); // entry id + for (FieldDef fd : cd.fields) { + skipField(p, fd); + } + } + } + // Only assert count consistency when we could walk all sections; + // an unknown class layout breaks the walk early but is not a test failure. + if (fullWalk) { + assertTrue(declaredPoolCount >= 5, + "pool count " + declaredPoolCount + + " is less than 5 (frameTypes + threadStates + " + + "executionModes + logLevels + threads are always non-empty)"); + } + + if (delta == 0) break; + off += delta; + } + } + + // ── metadata parsing ────────────────────────────────────────────────── + + private void parseMetadata() { + long[] p = {base + metaOffset}; + readVarLong(p); // size + readVarLong(p); // typeId + readVarLong(p); // start + readVarLong(p); // duration + readVarLong(p); // metadataId + long stringCount = readVarLong(p); + String[] pool = new String[(int) stringCount]; + for (int i = 0; i < stringCount; i++) { + pool[i] = readString(p); + } + collectClasses(readElement(p, pool)); + } + + private void collectClasses(Element el) { + if ("class".equals(el.name)) { + String idStr = el.attrs.get("id"); + if (idStr != null) { + ClassDef cd = new ClassDef(); + cd.id = Long.parseLong(idStr); + cd.name = el.attrs.getOrDefault("name", ""); + for (Element child : el.children) { + if ("field".equals(child.name)) { + FieldDef fd = new FieldDef(); + fd.name = child.attrs.get("name"); + 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; + cd.fields.add(fd); + } + } + classes.put(cd.id, cd); + } + } + for (Element child : el.children) { + collectClasses(child); + } + } + + // ── field-skipping ──────────────────────────────────────────────────── + + private void skipField(long[] p, FieldDef fd) { + if (fd.dimension == 1) { + long n = readVarLong(p); + for (long i = 0; i < n; i++) { + skipScalar(p, fd); + } + return; + } + skipScalar(p, fd); + } + + private void skipScalar(long[] p, FieldDef fd) { + if (fd.constantPool) { + readVarLong(p); + return; + } + ClassDef t = classes.get(fd.typeId); + String tn = t != null ? t.name : ""; + switch (tn) { + case "java.lang.String": readString(p); break; + case "boolean": + case "byte": p[0] += 1; break; + case "short": + case "char": + case "int": + case "long": readVarLong(p); break; + case "float": p[0] += 4; break; + case "double": p[0] += 8; break; + default: + if (t != null) { + for (FieldDef sub : t.fields) { + skipField(p, sub); + } + } + break; + } + } + + // ── low-level readers ───────────────────────────────────────────────── + + private Element readElement(long[] p, String[] pool) { + Element el = new Element(); + el.name = pool[(int) readVarLong(p)]; + long attrCount = readVarLong(p); + for (long i = 0; i < attrCount; i++) { + String k = pool[(int) readVarLong(p)]; + String v = pool[(int) readVarLong(p)]; + el.attrs.put(k, v); + } + long childCount = readVarLong(p); + for (long i = 0; i < childCount; i++) { + el.children.add(readElement(p, pool)); + } + return el; + } + + private long readVarLong(long[] p) { + long result = 0; + int shift = 0; + int i = (int) p[0]; + for (int b = 0; b < 8; b++) { + int by = f[i++] & 0xff; + result |= ((long) (by & 0x7f)) << shift; + if ((by & 0x80) == 0) { p[0] = i; return result; } + shift += 7; + } + result |= ((long) (f[i++] & 0xff)) << shift; + p[0] = i; + return result; + } + + private String readString(long[] p) { + int i = (int) p[0]; + int enc = f[i++] & 0xff; + p[0] = i; + switch (enc) { + case 0: return null; + case 1: return ""; + case 3: { + long len = readVarLong(p); + String s = new String(f, (int) p[0], (int) len, + java.nio.charset.StandardCharsets.UTF_8); + p[0] += len; + return s; + } + case 4: { + long len = readVarLong(p); + StringBuilder sb = new StringBuilder((int) len); + for (long c = 0; c < len; c++) sb.append((char) readVarLong(p)); + return sb.toString(); + } + case 5: { + long len = readVarLong(p); + String s = new String(f, (int) p[0], (int) len, + java.nio.charset.StandardCharsets.ISO_8859_1); + p[0] += len; + return s; + } + default: + throw new IllegalStateException("unknown JFR string encoding " + enc); + } + } + } + + // ── helpers ──────────────────────────────────────────────────────────────── + + private static long beLong(byte[] f, int off) { + long v = 0; + for (int i = 0; i < 8; i++) v = (v << 8) | (f[off + i] & 0xffL); + return v; + } + + private static final class ClassDef { + long id; + String name; + final List fields = new ArrayList<>(); + } + + private static final class FieldDef { + String name; + long typeId; + boolean constantPool; + int dimension; + } + + private static final class Element { + String name; + final Map attrs = new HashMap<>(); + final List children = new ArrayList<>(); + } +}