8342902: Deduplication of acquire calls in BindingSpecializer causes escape-analyisis failure

mcimadamore · mcimadamore · commit f1a9a8d25b2e · 2024-10-25T21:07:48.000Z
Reviewed-by: jvernee
diff --git a/src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java b/src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java
@@ -24,6 +24,7 @@
  */
 package jdk.internal.foreign.abi;
 
+import java.lang.classfile.Annotation;
 import java.lang.classfile.ClassFile;
 import java.lang.classfile.CodeBuilder;
 import java.lang.classfile.Label;
@@ -46,10 +47,13 @@
 import jdk.internal.foreign.abi.Binding.ShiftRight;
 import jdk.internal.foreign.abi.Binding.VMLoad;
 import jdk.internal.foreign.abi.Binding.VMStore;
+import jdk.internal.vm.annotation.ForceInline;
 import sun.security.action.GetBooleanAction;
+import sun.security.action.GetIntegerAction;
 import sun.security.action.GetPropertyAction;
 
 import java.io.IOException;
+import java.lang.classfile.attribute.RuntimeVisibleAnnotationsAttribute;
 import java.lang.constant.ClassDesc;
 import java.lang.constant.ConstantDesc;
 import java.lang.constant.DynamicConstantDesc;
@@ -77,6 +81,8 @@ public class BindingSpecializer {
         = GetPropertyAction.privilegedGetProperty("jdk.internal.foreign.abi.Specializer.DUMP_CLASSES_DIR");
     private static final boolean PERFORM_VERIFICATION
         = GetBooleanAction.privilegedGetProperty("jdk.internal.foreign.abi.Specializer.PERFORM_VERIFICATION");
+    private static final int SCOPE_DEDUP_DEPTH
+            = GetIntegerAction.privilegedGetProperty("jdk.internal.foreign.abi.Specializer.SCOPE_DEDUP_DEPTH", 2);
 
     // Bunch of helper constants
     private static final int CLASSFILE_VERSION = ClassFileFormatVersion.latest().major();
@@ -99,6 +105,7 @@ public class BindingSpecializer {
     private static final ClassDesc CD_ValueLayout_OfFloat = referenceClassDesc(ValueLayout.OfFloat.class);
     private static final ClassDesc CD_ValueLayout_OfDouble = referenceClassDesc(ValueLayout.OfDouble.class);
     private static final ClassDesc CD_AddressLayout = referenceClassDesc(AddressLayout.class);
+    private static final ClassDesc CD_ForceInline = referenceClassDesc(ForceInline.class);
 
     private static final MethodTypeDesc MTD_NEW_BOUNDED_ARENA = MethodTypeDesc.of(CD_Arena, CD_long);
     private static final MethodTypeDesc MTD_NEW_EMPTY_ARENA = MethodTypeDesc.of(CD_Arena);
@@ -196,8 +203,9 @@ private static byte[] specializeHelper(MethodType leafType, MethodType callerMet
             clb.withFlags(ACC_PUBLIC + ACC_FINAL + ACC_SUPER)
                .withSuperclass(CD_Object)
                .withVersion(CLASSFILE_VERSION, 0)
-               .withMethodBody(METHOD_NAME, methodTypeDesc(callerMethodType), ACC_PUBLIC | ACC_STATIC,
-                    cb -> new BindingSpecializer(cb, callerMethodType, callingSequence, abi, leafType).specialize());
+               .withMethod(METHOD_NAME, methodTypeDesc(callerMethodType), ACC_PUBLIC | ACC_STATIC,
+                    mb -> mb.with(RuntimeVisibleAnnotationsAttribute.of(Annotation.of(CD_ForceInline)))
+                            .withCode(cb -> new BindingSpecializer(cb, callerMethodType, callingSequence, abi, leafType).specialize()));
         });
 
         if (DUMP_CLASSES_DIR != null) {
@@ -502,11 +510,19 @@ private void emitAcquireScope() {
 
         // start with 1 scope to maybe acquire on the stack
         assert curScopeLocalIdx != -1;
-        boolean hasOtherScopes = curScopeLocalIdx != 0;
-        for (int i = 0; i < curScopeLocalIdx; i++) {
+        boolean hasLookup = false;
+
+        // Here we check if the current scope has not been already acquired.
+        // To do that, we generate many comparisons (one per cached scope).
+        // Note that we always skip comparisons against the very first cached scope
+        // (as that is the function address, which typically belongs to another scope).
+        // We also stop the comparisons at SCOPE_DEDUP_DEPTH, to keep a lid on the size
+        // of the generated code.
+        for (int i = 1; i < curScopeLocalIdx && i <= SCOPE_DEDUP_DEPTH; i++) {
             cb.dup() // dup for comparison
               .aload(scopeSlots[i])
               .if_acmpeq(skipAcquire);
+            hasLookup = true;
         }
 
         // 1 scope to acquire on the stack
@@ -516,10 +532,10 @@ private void emitAcquireScope() {
         cb.invokevirtual(CD_MemorySessionImpl, "acquire0", MTD_ACQUIRE0) // call acquire on the other
           .astore(nextScopeLocal); // store off one to release later
 
-        if (hasOtherScopes) { // avoid ASM generating a bunch of nops for the dead code
+        if (hasLookup) { // avoid ASM generating a bunch of nops for the dead code
             cb.goto_(end)
-              .labelBinding(skipAcquire)
-              .pop(); // drop scope
+                    .labelBinding(skipAcquire)
+                    .pop(); // drop scope
         }
 
         cb.labelBinding(end);
diff --git a/test/micro/org/openjdk/bench/java/lang/foreign/CallByRefHighArity.java b/test/micro/org/openjdk/bench/java/lang/foreign/CallByRefHighArity.java
@@ -0,0 +1,247 @@
+/*
+ * Copyright (c) 2024, 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 org.openjdk.bench.java.lang.foreign;
+
+import org.openjdk.jmh.annotations.*;
+
+import java.lang.foreign.*;
+import java.lang.invoke.*;
+import java.util.concurrent.TimeUnit;
+import java.util.function.Supplier;
+
+@BenchmarkMode(Mode.AverageTime)
+@Warmup(iterations = 5, time = 500, timeUnit = TimeUnit.MILLISECONDS)
+@Measurement(iterations = 10, time = 500, timeUnit = TimeUnit.MILLISECONDS)
+@State(org.openjdk.jmh.annotations.Scope.Thread)
+@OutputTimeUnit(TimeUnit.NANOSECONDS)
+@Fork(value = 3, jvmArgsAppend = { "--enable-native-access=ALL-UNNAMED", "-Djava.library.path=micro/native" })
+public class CallByRefHighArity {
+
+    static {
+        System.loadLibrary("CallByRefHighArity");
+    }
+
+    @Param
+    SegmentKind kind;
+
+    public enum SegmentKind {
+        CONFINED,
+        SHARED,
+        GLOBAL,
+        HEAP
+    }
+
+    Supplier<MemorySegment> segmentSupplier;
+    Arena arena;
+
+    @Setup
+    public void setup() {
+        if (kind == SegmentKind.CONFINED) {
+            arena = Arena.ofConfined();
+            MemorySegment segment = arena.allocateFrom(ValueLayout.JAVA_INT, 0);
+            segmentSupplier = () -> segment;
+        } else if (kind == SegmentKind.SHARED) {
+            arena = Arena.ofShared();
+            MemorySegment segment = arena.allocateFrom(ValueLayout.JAVA_INT, 0);
+            segmentSupplier = () -> segment;
+        } else if (kind == SegmentKind.HEAP) {
+            byte[] array = new byte[8];
+            MemorySegment segment = MemorySegment.ofArray(array);
+            segmentSupplier = () -> segment;
+        } else { // global
+            segmentSupplier = () -> MemorySegment.ofAddress(0);
+        }
+    }
+
+    @TearDown
+    public void tearDown() {
+        if (arena != null) {
+            arena.close();
+        }
+    }
+
+    // A shared library that exports the functions below
+    private static final SymbolLookup LOOKUP = SymbolLookup.loaderLookup();
+
+    // void noop_params0() {}
+    private static final MethodHandle MH_NOOP_PARAMS0 = Linker.nativeLinker()
+            .downcallHandle(FunctionDescriptor.ofVoid(), Linker.Option.critical(true))
+            .bindTo(LOOKUP.find("noop_params0").orElseThrow());
+
+    // void noop_params1(void *param0) {}
+    private static final MethodHandle MH_NOOP_PARAMS1 = Linker.nativeLinker()
+            .downcallHandle(FunctionDescriptor.ofVoid(
+                    ValueLayout.ADDRESS
+            ), Linker.Option.critical(true))
+            .bindTo(LOOKUP.find("noop_params1").orElseThrow());
+
+    // void noop_params2(void *param0, void *param1) {}
+    private static final MethodHandle MH_NOOP_PARAMS2 = Linker.nativeLinker()
+            .downcallHandle(FunctionDescriptor.ofVoid(
+                    ValueLayout.ADDRESS,
+                    ValueLayout.ADDRESS
+            ), Linker.Option.critical(true))
+            .bindTo(LOOKUP.find("noop_params2").orElseThrow());
+
+    // void noop_params3(void *param0, void *param1, void *param2) {}
+    private static final MethodHandle MH_NOOP_PARAMS3 = Linker.nativeLinker()
+            .downcallHandle(FunctionDescriptor.ofVoid(
+                    ValueLayout.ADDRESS,
+                    ValueLayout.ADDRESS,
+                    ValueLayout.ADDRESS
+            ), Linker.Option.critical(true))
+            .bindTo(LOOKUP.find("noop_params3").orElseThrow());
+
+    // void noop_params4(void *param0, void *param1, void *param2, void *param3) {}
+    private static final MethodHandle MH_NOOP_PARAMS4 = Linker.nativeLinker()
+            .downcallHandle(FunctionDescriptor.ofVoid(
+                    ValueLayout.ADDRESS,
+                    ValueLayout.ADDRESS,
+                    ValueLayout.ADDRESS,
+                    ValueLayout.ADDRESS
+            ), Linker.Option.critical(true))
+            .bindTo(LOOKUP.find("noop_params4").orElseThrow());
+
+    // void noop_params5(int param0, int param1, void *param2, void *param3, void *param4) {}
+    private static final MethodHandle MH_NOOP_PARAMS5 = Linker.nativeLinker()
+            .downcallHandle(FunctionDescriptor.ofVoid(
+                    ValueLayout.ADDRESS,
+                    ValueLayout.ADDRESS,
+                    ValueLayout.ADDRESS,
+                    ValueLayout.ADDRESS,
+                    ValueLayout.ADDRESS
+            ), Linker.Option.critical(true))
+            .bindTo(LOOKUP.find("noop_params5").orElseThrow());
+
+    // void noop_params10(void *param0, void *param1, void *param2, void *param3, void *param4,
+    //                    void *param5, void *param6, void *param7, void *param8, void *param9) {}
+    private static final MethodHandle MH_NOOP_PARAMS10 = Linker.nativeLinker()
+            .downcallHandle(FunctionDescriptor.ofVoid(
+                    ValueLayout.ADDRESS,
+                    ValueLayout.ADDRESS,
+                    ValueLayout.ADDRESS,
+                    ValueLayout.ADDRESS,
+                    ValueLayout.ADDRESS,
+                    ValueLayout.ADDRESS,
+                    ValueLayout.ADDRESS,
+                    ValueLayout.ADDRESS,
+                    ValueLayout.ADDRESS,
+                    ValueLayout.ADDRESS
+            ), Linker.Option.critical(true))
+            .bindTo(LOOKUP.find("noop_params10").orElseThrow());
+
+    @Benchmark
+    public void noop_params0() {
+        try {
+            MH_NOOP_PARAMS0.invokeExact();
+        } catch (Throwable t) {
+            throw new AssertionError(t);
+        }
+    }
+
+    @Benchmark
+    public void noop_params1() {
+        try {
+            MH_NOOP_PARAMS1.invokeExact(
+                    segmentSupplier.get()
+            );
+        } catch (Throwable t) {
+            throw new AssertionError(t);
+        }
+    }
+
+    @Benchmark
+    public void noop_params2() {
+        try {
+            MH_NOOP_PARAMS2.invokeExact(
+                    segmentSupplier.get(),
+                    segmentSupplier.get()
+            );
+        } catch (Throwable t) {
+            throw new AssertionError(t);
+        }
+    }
+
+    @Benchmark
+    public void noop_params3() {
+        try {
+            MH_NOOP_PARAMS3.invokeExact(
+                    segmentSupplier.get(),
+                    segmentSupplier.get(),
+                    segmentSupplier.get()
+            );
+        } catch (Throwable t) {
+            throw new AssertionError(t);
+        }
+    }
+
+    @Benchmark
+    public void noop_params4() {
+        try {
+            MH_NOOP_PARAMS4.invokeExact(
+                    segmentSupplier.get(),
+                    segmentSupplier.get(),
+                    segmentSupplier.get(),
+                    segmentSupplier.get()
+            );
+        } catch (Throwable t) {
+            throw new AssertionError(t);
+        }
+    }
+
+    @Benchmark
+    public void noop_params5() {
+        try {
+            MH_NOOP_PARAMS5.invokeExact(
+                    segmentSupplier.get(),
+                    segmentSupplier.get(),
+                    segmentSupplier.get(),
+                    segmentSupplier.get(),
+                    segmentSupplier.get()
+            );
+        } catch (Throwable t) {
+            throw new AssertionError(t);
+        }
+    }
+
+    @Benchmark
+    public void noop_params10() {
+        try {
+            MH_NOOP_PARAMS10.invokeExact(
+                    segmentSupplier.get(),
+                    segmentSupplier.get(),
+                    segmentSupplier.get(),
+                    segmentSupplier.get(),
+                    segmentSupplier.get(),
+                    segmentSupplier.get(),
+                    segmentSupplier.get(),
+                    segmentSupplier.get(),
+                    segmentSupplier.get(),
+                    segmentSupplier.get()
+            );
+        } catch (Throwable t) {
+            throw new AssertionError(t);
+        }
+    }
+}
diff --git a/test/micro/org/openjdk/bench/java/lang/foreign/libCallByRefHighArity.c b/test/micro/org/openjdk/bench/java/lang/foreign/libCallByRefHighArity.c
@@ -0,0 +1,33 @@
+/*
+ * Copyright (c) 2024, 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.
+ */
+
+#include "export.h"
+
+EXPORT void noop_params0() {}
+EXPORT void noop_params1(void *param0) {}
+EXPORT void noop_params2(void *param0, void *param1) {}
+EXPORT void noop_params3(void *param0, void *param1, void *param2) {}
+EXPORT void noop_params4(void *param0, void *param1, void *param2, void *param3) {}
+EXPORT void noop_params5(void *param0, void *param1, void *param2, void *param3, void *param4) {}
+EXPORT void noop_params10(void *param0, void *param1, void *param2, void *param3, void *param4,
+                          void *param5, void *param6, void *param7, void *param8, void *param9) {}
