From 8e4185c2bb9eb254fedadca004337209eb5ea308 Mon Sep 17 00:00:00 2001 From: mgronlun Date: Mon, 24 Mar 2025 12:38:26 +0100 Subject: [PATCH 1/6] 8352696 --- src/hotspot/share/opto/library_call.cpp | 11 +- .../jfr/jvm/TestJvmCommitIntrinsicAndEA.java | 117 ++++++++++++++++++ 2 files changed, 124 insertions(+), 4 deletions(-) create mode 100644 test/jdk/jdk/jfr/jvm/TestJvmCommitIntrinsicAndEA.java diff --git a/src/hotspot/share/opto/library_call.cpp b/src/hotspot/share/opto/library_call.cpp index d5e33e7f9ed96..3462bd78bbc8b 100644 --- a/src/hotspot/share/opto/library_call.cpp +++ b/src/hotspot/share/opto/library_call.cpp @@ -3154,7 +3154,8 @@ bool LibraryCallKit::inline_native_jvm_commit() { set_control(is_notified); // Reset notified state. - Node* notified_reset_memory = store_to_memory(control(), notified_offset, _gvn.intcon(0), T_BOOLEAN, MemNode::unordered); + store_to_memory(control(), notified_offset, _gvn.intcon(0), T_BOOLEAN, MemNode::unordered); + Node* notified_reset_memory = reset_memory(); // Iff notified, the return address of the commit method is the current position of the backing java buffer. This is used to reset the event writer. Node* current_pos_X = _gvn.transform(new LoadXNode(control(), input_memory_state, java_buffer_pos_offset, TypeRawPtr::NOTNULL, TypeX_X, MemNode::unordered)); @@ -3172,13 +3173,15 @@ bool LibraryCallKit::inline_native_jvm_commit() { Node* next_pos_X = _gvn.transform(ConvL2X(arg)); // Store the next_position to the underlying jfr java buffer. - Node* commit_memory; #ifdef _LP64 - commit_memory = store_to_memory(control(), java_buffer_pos_offset, next_pos_X, T_LONG, MemNode::release); + store_to_memory(control(), java_buffer_pos_offset, next_pos_X, T_LONG, MemNode::release); #else - commit_memory = store_to_memory(control(), java_buffer_pos_offset, next_pos_X, T_INT, MemNode::release); + store_to_memory(control(), java_buffer_pos_offset, next_pos_X, T_INT, MemNode::release); #endif + Node* commit_memory = reset_memory(); + set_all_memory(commit_memory); + // Now load the flags from off the java buffer and decide if the buffer is a lease. If so, it needs to be returned post-commit. Node* java_buffer_flags_offset = _gvn.transform(new AddPNode(top(), java_buffer, _gvn.transform(MakeConX(in_bytes(JFR_BUFFER_FLAGS_OFFSET))))); Node* flags = make_load(control(), java_buffer_flags_offset, TypeInt::UBYTE, T_BYTE, MemNode::unordered); diff --git a/test/jdk/jdk/jfr/jvm/TestJvmCommitIntrinsicAndEA.java b/test/jdk/jdk/jfr/jvm/TestJvmCommitIntrinsicAndEA.java new file mode 100644 index 0000000000000..6a875817f2b28 --- /dev/null +++ b/test/jdk/jdk/jfr/jvm/TestJvmCommitIntrinsicAndEA.java @@ -0,0 +1,117 @@ +/* + * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +package jdk.jfr.jvm; + +import java.util.List; +import java.util.Map; +import java.util.concurrent.Executor; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.RejectedExecutionException; +import java.util.concurrent.ThreadFactory; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.locks.LockSupport; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import jdk.jfr.EventType; +import jdk.jfr.Recording; +import jdk.jfr.consumer.RecordedEvent; +import jdk.test.lib.Asserts; +import jdk.test.lib.jfr.EventNames; +import jdk.test.lib.jfr.Events; + +/** + * + * A successful execution is when the JTREG test executes successfully, + * i.e., with no exceptions or JVM asserts. + * + * In JVM debug builds, C2 will assert as part of EscapeAnalysis and dump an hs_err.log: + * + * # + * # A fatal error has been detected by the Java Runtime Environment: + * # + * # Internal Error (..\open\src\hotspot\share\opto\escape.cpp:4788) + * # assert(false) failed: EA: missing memory path + * + * Current thread JavaThread "C2 CompilerThread2" + * Current CompileTask: + * C2:27036 1966 ! 4 java.lang.VirtualThread::run (173 bytes) + * + * ConnectionGraph::split_unique_types+0x2409 (escape.cpp:4788) + * ConnectionGraph::compute_escape+0x11e6 (escape.cpp:397) + * ConnectionGraph::do_analysis+0xf2 (escape.cpp:118) + * Compile::Optimize+0x85d (compile.cpp:2381) + * ... + * + * The error is because C2's Escape Analysis does not recognize a pattern + * where one input of memory Phi node is MergeMem node, and another is RAW store. + * This pattern is created by the jdk.jfr.internal.JVM.commit() intrinsic, + * which is inlined because of inlining the JFR event jdk.VirtualThreadStart. + * + * As a result, EA complains about a strange memory graph. + */ + +/** + * @test + * @bug 8352696 + * @requires vm.flagless + * @requires vm.hasJFR & vm.debug + * @library /test/lib /test/jdk + * @run main/othervm jdk.jfr.jvm.TestJvmCommitIntrinsicAndEA + */ +public final class TestJvmCommitIntrinsicAndEA { + + public static void main(String[] args) throws Throwable { + try (Recording recording = new Recording()) { + recording.enable(EventNames.VirtualThreadStart).withoutStackTrace(); + recording.enable(EventNames.VirtualThreadEnd).withoutStackTrace(); + recording.start(); + // execute 10_000 tasks, each in their own virtual thread + ThreadFactory factory = Thread.ofVirtual().factory(); + try (var executor = Executors.newThreadPerTaskExecutor(factory)) { + for (int i = 0; i < 10_000; i++) { + executor.submit(() -> { }); + } + } finally { + recording.stop(); + } + + Map events = sumEvents(recording); + System.err.println(events); + + int startCount = events.getOrDefault(EventNames.VirtualThreadStart, 0); + int endCount = events.getOrDefault(EventNames.VirtualThreadEnd, 0); + Asserts.assertEquals(10_000, startCount, "Expected 10000, got " + startCount); + Asserts.assertEquals(10_000, endCount, "Expected 10000, got " + endCount); + } + } + + private static Map sumEvents(Recording recording) throws Exception { + List events = Events.fromRecording(recording); + return events.stream() + .map(RecordedEvent::getEventType) + .collect(Collectors.groupingBy(EventType::getName, + Collectors.summingInt(x -> 1))); + } +} From cd7f30b6634bfaacbb7d7aa23afc9107784fc463 Mon Sep 17 00:00:00 2001 From: mgronlun Date: Mon, 24 Mar 2025 13:22:13 +0100 Subject: [PATCH 2/6] indentation --- test/jdk/jdk/jfr/jvm/TestJvmCommitIntrinsicAndEA.java | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/test/jdk/jdk/jfr/jvm/TestJvmCommitIntrinsicAndEA.java b/test/jdk/jdk/jfr/jvm/TestJvmCommitIntrinsicAndEA.java index 6a875817f2b28..8fb145c0e4d2a 100644 --- a/test/jdk/jdk/jfr/jvm/TestJvmCommitIntrinsicAndEA.java +++ b/test/jdk/jdk/jfr/jvm/TestJvmCommitIntrinsicAndEA.java @@ -82,8 +82,8 @@ */ public final class TestJvmCommitIntrinsicAndEA { - public static void main(String[] args) throws Throwable { - try (Recording recording = new Recording()) { + public static void main(String[] args) throws Throwable { + try (Recording recording = new Recording()) { recording.enable(EventNames.VirtualThreadStart).withoutStackTrace(); recording.enable(EventNames.VirtualThreadEnd).withoutStackTrace(); recording.start(); @@ -109,9 +109,7 @@ public static void main(String[] args) throws Throwable { private static Map sumEvents(Recording recording) throws Exception { List events = Events.fromRecording(recording); - return events.stream() - .map(RecordedEvent::getEventType) - .collect(Collectors.groupingBy(EventType::getName, - Collectors.summingInt(x -> 1))); + return events.stream().map(RecordedEvent::getEventType) + .collect(Collectors.groupingBy(EventType::getName, Collectors.summingInt(x -> 1))); } } From e072153b492a07ba65ab5ea49d6cf508468ed5f2 Mon Sep 17 00:00:00 2001 From: mgronlun Date: Mon, 24 Mar 2025 13:24:40 +0100 Subject: [PATCH 3/6] merged requires --- test/jdk/jdk/jfr/jvm/TestJvmCommitIntrinsicAndEA.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/jdk/jdk/jfr/jvm/TestJvmCommitIntrinsicAndEA.java b/test/jdk/jdk/jfr/jvm/TestJvmCommitIntrinsicAndEA.java index 8fb145c0e4d2a..de03d65cf1894 100644 --- a/test/jdk/jdk/jfr/jvm/TestJvmCommitIntrinsicAndEA.java +++ b/test/jdk/jdk/jfr/jvm/TestJvmCommitIntrinsicAndEA.java @@ -75,8 +75,7 @@ /** * @test * @bug 8352696 - * @requires vm.flagless - * @requires vm.hasJFR & vm.debug + * @requires vm.flagless & vm.hasJFR & vm.debug * @library /test/lib /test/jdk * @run main/othervm jdk.jfr.jvm.TestJvmCommitIntrinsicAndEA */ From 0ba8abfbbe1d2252a4976e21a6c701075238a6c0 Mon Sep 17 00:00:00 2001 From: mgronlun Date: Mon, 24 Mar 2025 13:37:20 +0100 Subject: [PATCH 4/6] Xbatch --- test/jdk/jdk/jfr/jvm/TestJvmCommitIntrinsicAndEA.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/jdk/jdk/jfr/jvm/TestJvmCommitIntrinsicAndEA.java b/test/jdk/jdk/jfr/jvm/TestJvmCommitIntrinsicAndEA.java index de03d65cf1894..a69c7e21a628f 100644 --- a/test/jdk/jdk/jfr/jvm/TestJvmCommitIntrinsicAndEA.java +++ b/test/jdk/jdk/jfr/jvm/TestJvmCommitIntrinsicAndEA.java @@ -77,7 +77,7 @@ * @bug 8352696 * @requires vm.flagless & vm.hasJFR & vm.debug * @library /test/lib /test/jdk - * @run main/othervm jdk.jfr.jvm.TestJvmCommitIntrinsicAndEA + * @run main/othervm -Xbatch jdk.jfr.jvm.TestJvmCommitIntrinsicAndEA */ public final class TestJvmCommitIntrinsicAndEA { From c63fb60891bce8f787df63a31f19bc0d8a89d210 Mon Sep 17 00:00:00 2001 From: mgronlun Date: Mon, 24 Mar 2025 13:43:13 +0100 Subject: [PATCH 5/6] fold --- src/hotspot/share/opto/library_call.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/hotspot/share/opto/library_call.cpp b/src/hotspot/share/opto/library_call.cpp index 3462bd78bbc8b..9e98c9883f76a 100644 --- a/src/hotspot/share/opto/library_call.cpp +++ b/src/hotspot/share/opto/library_call.cpp @@ -3173,11 +3173,7 @@ bool LibraryCallKit::inline_native_jvm_commit() { Node* next_pos_X = _gvn.transform(ConvL2X(arg)); // Store the next_position to the underlying jfr java buffer. -#ifdef _LP64 - store_to_memory(control(), java_buffer_pos_offset, next_pos_X, T_LONG, MemNode::release); -#else - store_to_memory(control(), java_buffer_pos_offset, next_pos_X, T_INT, MemNode::release); -#endif + store_to_memory(control(), java_buffer_pos_offset, next_pos_X, LP64_ONLY(T_LONG) NOT_LP64(T_INT), MemNode::release); Node* commit_memory = reset_memory(); set_all_memory(commit_memory); From cae054c9c1a4a21c5a3032dbbcc401194dc7d53b Mon Sep 17 00:00:00 2001 From: mgronlun Date: Mon, 24 Mar 2025 20:25:54 +0100 Subject: [PATCH 6/6] simplified test --- .../jfr/jvm/TestJvmCommitIntrinsicAndEA.java | 47 +++++-------------- 1 file changed, 13 insertions(+), 34 deletions(-) diff --git a/test/jdk/jdk/jfr/jvm/TestJvmCommitIntrinsicAndEA.java b/test/jdk/jdk/jfr/jvm/TestJvmCommitIntrinsicAndEA.java index a69c7e21a628f..1983d18230a73 100644 --- a/test/jdk/jdk/jfr/jvm/TestJvmCommitIntrinsicAndEA.java +++ b/test/jdk/jdk/jfr/jvm/TestJvmCommitIntrinsicAndEA.java @@ -23,23 +23,13 @@ package jdk.jfr.jvm; -import java.util.List; -import java.util.Map; import java.util.concurrent.Executor; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; -import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.ThreadFactory; -import java.util.concurrent.atomic.AtomicBoolean; -import java.util.concurrent.locks.LockSupport; -import java.util.stream.Collectors; -import java.util.stream.Stream; -import jdk.jfr.EventType; -import jdk.jfr.Recording; -import jdk.jfr.consumer.RecordedEvent; -import jdk.test.lib.Asserts; +import java.util.concurrent.CountDownLatch; +import jdk.jfr.consumer.RecordingStream; import jdk.test.lib.jfr.EventNames; -import jdk.test.lib.jfr.Events; /** * @@ -80,35 +70,24 @@ * @run main/othervm -Xbatch jdk.jfr.jvm.TestJvmCommitIntrinsicAndEA */ public final class TestJvmCommitIntrinsicAndEA { + private static final int NUM_TASKS = 10_000; public static void main(String[] args) throws Throwable { - try (Recording recording = new Recording()) { - recording.enable(EventNames.VirtualThreadStart).withoutStackTrace(); - recording.enable(EventNames.VirtualThreadEnd).withoutStackTrace(); - recording.start(); - // execute 10_000 tasks, each in their own virtual thread + CountDownLatch latch = new CountDownLatch(NUM_TASKS); + + try (RecordingStream rs = new RecordingStream()) { + rs.enable(EventNames.VirtualThreadStart).withoutStackTrace(); + rs.enable(EventNames.VirtualThreadEnd).withoutStackTrace(); + rs.onEvent(EventNames.VirtualThreadEnd, e -> latch.countDown()); + rs.startAsync(); + // Execute NUM_TASKS, each in their own virtual thread. ThreadFactory factory = Thread.ofVirtual().factory(); try (var executor = Executors.newThreadPerTaskExecutor(factory)) { - for (int i = 0; i < 10_000; i++) { + for (int i = 0; i < NUM_TASKS; i++) { executor.submit(() -> { }); } - } finally { - recording.stop(); } - - Map events = sumEvents(recording); - System.err.println(events); - - int startCount = events.getOrDefault(EventNames.VirtualThreadStart, 0); - int endCount = events.getOrDefault(EventNames.VirtualThreadEnd, 0); - Asserts.assertEquals(10_000, startCount, "Expected 10000, got " + startCount); - Asserts.assertEquals(10_000, endCount, "Expected 10000, got " + endCount); + latch.await(); } } - - private static Map sumEvents(Recording recording) throws Exception { - List events = Events.fromRecording(recording); - return events.stream().map(RecordedEvent::getEventType) - .collect(Collectors.groupingBy(EventType::getName, Collectors.summingInt(x -> 1))); - } }