diff --git a/ddprof-lib/src/main/cpp/flightRecorder.cpp b/ddprof-lib/src/main/cpp/flightRecorder.cpp index 3a2f633d61..29156844cf 100644 --- a/ddprof-lib/src/main/cpp/flightRecorder.cpp +++ b/ddprof-lib/src/main/cpp/flightRecorder.cpp @@ -354,7 +354,7 @@ void Lookup::fillJavaMethodInfo(MethodInfo *mi, jmethodID method, } else { Counters::increment(JMETHODID_SKIPPED); class_name_id = _classes->lookupDuringDump("", 0, Profiler::maxClassMapSize()); - method_name_id = _symbols.lookup("jvmtiError"); + method_name_id = _symbols.lookup(""); method_sig_id = _symbols.lookup("()L;"); } diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/CTimerSamplerTest.java b/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/CTimerSamplerTest.java index 6fe6fe3073..123b015834 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/CTimerSamplerTest.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/CTimerSamplerTest.java @@ -23,7 +23,6 @@ import java.util.Set; import java.util.concurrent.ExecutionException; -import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assumptions.assumeFalse; import static org.junit.jupiter.api.Assumptions.assumeTrue; @@ -55,11 +54,8 @@ public void test(@CStack String cstack) throws ExecutionException, InterruptedEx verifyCStackSettings(); // Streamed rather than materialized: cpu=100us over this workload can produce tens of - // thousands of samples, and every check here is per-event with no need to retain them. - long sampleCount = streamEvents("datadog.ExecutionSample", sample -> { - String stackTrace = sample.getStackTraceString(); - assertFalse(stackTrace.contains("jvmtiError")); - }); + // thousands of samples; streamEvents counts them without retaining them in memory. + long sampleCount = streamEvents("datadog.ExecutionSample", sample -> { }); assertTrue(sampleCount > 0, "datadog.ExecutionSample was empty"); } diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/RemoteSymbolicationTest.java b/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/RemoteSymbolicationTest.java index 89e07023bb..de6d18841a 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/RemoteSymbolicationTest.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/RemoteSymbolicationTest.java @@ -18,9 +18,7 @@ import com.datadoghq.profiler.JfrEvents; import com.datadoghq.profiler.JfrFrame; -import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.junit.jupiter.api.Assertions.fail; /** * Integration test for remote symbolication feature. @@ -110,11 +108,7 @@ public void testRemoteSymbolicationEnabled(@CStack String cstack) throws Excepti // Iterate through frames to check for test library frames for (JfrFrame frame : sample.getStackTrace().frames()) { - // Check for jvmtiError in method name String methodName = frame.methodName(); - if (methodName != null && methodName.contains("jvmtiError")) { - fail("Found jvmtiError in frame method name: " + methodName); - } // Get class name (contains build-id for remote symbolication frames) String className = frame.className(); diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/SmokeCpuTest.java b/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/SmokeCpuTest.java index 74a0ddf2b9..bd894e07e5 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/SmokeCpuTest.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/cpu/SmokeCpuTest.java @@ -17,7 +17,6 @@ import java.util.concurrent.ExecutionException; -import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; import com.datadoghq.profiler.Platform; @@ -44,7 +43,6 @@ public void testComputations(@CStack String cstack) throws Exception { // on mac the usage of itimer to drive the sampling provides very unreliable outputs for (JfrEvent sample : events) { String stackTrace = sample.getStackTraceString(); - assertFalse(stackTrace.contains("jvmtiError")); if ("vmx".equals(stackTrace)) { // extra checks to make sure we see the mixed stacktraces assertTrue(stackTrace.contains("JavaCalls::call_virtual()"), @@ -71,7 +69,6 @@ public void testIOBound(@CStack String cstack) throws Exception { // on mac the usage of itimer to drive the sampling provides very unreliable outputs for (JfrEvent sample : events) { String stackTrace = sample.getStackTraceString(); - assertFalse(stackTrace.contains("jvmtiError")); if ("vmx".equals(stackTrace)) { // extra checks to make sure we see the mixed stacktraces assertTrue(stackTrace.contains("JavaCalls::call_virtual()"), diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/memleak/JMethodIDInvalidationStressTest.java b/ddprof-test/src/test/java/com/datadoghq/profiler/memleak/JMethodIDInvalidationStressTest.java index b098b46070..443fbd635b 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/memleak/JMethodIDInvalidationStressTest.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/memleak/JMethodIDInvalidationStressTest.java @@ -15,6 +15,8 @@ */ package com.datadoghq.profiler.memleak; +import com.datadoghq.profiler.JfrEvent; +import com.datadoghq.profiler.JfrFrame; import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; @@ -33,6 +35,7 @@ import java.util.Map; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.atomic.AtomicReference; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -188,6 +191,22 @@ public void testProfilerSurvivesConcurrentClassUnloadDuringDump() throws Excepti + "churn, not just that the JVM didn't crash; if this keeps getting skipped, the " + "churn isn't racing unload against resolveMethod/fillJavaMethodInfo tightly " + "enough (consider more churn threads or a longer window)."); + + // The jmethodid_skipped_count counter is incremented in the exact branch of + // Lookup::fillJavaMethodInfo (the JVMTI-resolution-failure 'else') that serializes a + // stale-jmethodID frame as ''. line_number_table_unreadable is a separate, + // mutually-exclusive branch where the jmethodID *did* resolve to a real method/class name + // and only the line-number-table copy failed -- it never produces ''. So the + // recording assertion below is only meaningful when skippedDelta > 0; a run that only + // triggered the line-table race would otherwise fail spuriously. Gate it accordingly. + Assumptions.assumeTrue(skippedDelta > 0, + "Churn window hit only the line-number-table race (line_number_table_unreadable delta=" + + unreadableLineTableDelta + ", jmethodid_skipped_count delta=" + skippedDelta + + ") -- the '' label assertion is only meaningful when the" + + " JVMTI-resolution-failure branch ran; skipping to avoid a spurious failure."); + // Assert that the recording produced by that branch uses the '' label and + // never the legacy 'jvmtiError' one -- this fails if the label is reverted to 'jvmtiError'. + assertUnloadedFrameLabel(dumpFile); } finally { running.set(false); for (Thread t : churnThreads) { @@ -217,6 +236,47 @@ public void testProfilerSurvivesConcurrentClassUnloadDuringDump() throws Excepti } } + /** + * Asserts that the JFR recording produced by the churn window serializes stale-jmethodID + * frames as {@code ""} and never as the legacy {@code "jvmtiError"} label. + * This is the regression guard for the flightRecorder.cpp remap: reverting the label to + * {@code "jvmtiError"} makes this assertion fail. Only stack-trace-bearing event types + * are inspected; events without a {@code stackTrace} field are skipped. + */ + private void assertUnloadedFrameLabel(Path recording) throws Exception { + AtomicBoolean foundUnloaded = new AtomicBoolean(); + AtomicBoolean foundLegacy = new AtomicBoolean(); + AtomicReference legacySample = new AtomicReference<>(); + for (String eventType : new String[]{"datadog.ExecutionSample", "datadog.AllocationSample"}) { + streamEvents(recording, eventType, event -> { + if (!event.has(STACK_TRACE)) { + return; + } + for (JfrFrame frame : event.getStackTrace().frames()) { + String name = frame.methodName(); + if (name == null) { + continue; + } + if (name.equals("")) { + foundUnloaded.set(true); + } else if (name.equals("jvmtiError")) { + foundLegacy.set(true); + if (legacySample.get() == null) { + legacySample.set(event.getStackTraceString()); + } + } + } + }); + } + assertTrue(foundUnloaded.get(), + "Expected at least one frame serialized as '' in " + recording + + " (jmethodid_skipped_count fired, so the stale-jmethodID branch ran), " + + "but none was found -- the remap to '' may have been reverted."); + assertTrue(!foundLegacy.get(), + "Found a frame serialized as the legacy 'jvmtiError' label in " + recording + + "; expected ''. First offending sample: " + legacySample.get()); + } + private void churnLoop(AtomicBoolean running) { while (running.get()) { try { diff --git a/ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/SmokeWallTest.java b/ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/SmokeWallTest.java index 91a717efea..d8cda8b4b3 100644 --- a/ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/SmokeWallTest.java +++ b/ddprof-test/src/test/java/com/datadoghq/profiler/wallclock/SmokeWallTest.java @@ -12,12 +12,10 @@ import com.datadoghq.profiler.junit.RetryTest; import org.junit.jupiter.api.TestTemplate; import org.junit.jupiter.params.provider.ValueSource; -import com.datadoghq.profiler.JfrEvent; import com.datadoghq.profiler.JfrEvents; import java.util.concurrent.ExecutionException; -import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assumptions.assumeFalse; public class SmokeWallTest extends CStackAwareAbstractProfilerTest { @@ -44,11 +42,6 @@ public void test(@CStack String cstack) throws ExecutionException, InterruptedEx verifyCStackSettings(); JfrEvents events = verifyEvents("datadog.MethodSample"); - - for (JfrEvent sample : events) { - String stackTrace = sample.getStackTraceString(); - assertFalse(stackTrace.contains("jvmtiError")); - } } @Override