[GR-28460] Improve InlineBeforeAnalysis.

PullRequest: graal/9469

Author: Christian Wimmer

substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/phases/InlineBeforeAnalysis.java

@@ -128,9 +128,26 @@ class InlineBeforeAnalysisMethodScope extends PEMethodScope {
             super(targetGraph, caller, callerLoopScope, encodedGraph, method, invokeData, inliningDepth, arguments);
 
             if (caller == null) {
+                /*
+                 * The root method that we are decoding, i.e., inlining into. No policy, because the
+                 * whole method must of course be decoded.
+                 */
+                policyScope = null;
+            } else if (caller.caller == null) {
+                /*
+                 * The first level of method inlining, i.e., the top scope from the inlining policy
+                 * point of view.
+                 */
                 policyScope = policy.createTopScope();
+                if (graph.getDebug().isLogEnabled()) {
+                    graph.getDebug().logv(repeat("  ", inliningDepth) + "createTopScope for " + method.format("%H.%n(%p)") + ": " + policyScope);
+                }
             } else {
+                /* Nested inlining. */
                 policyScope = policy.openCalleeScope((cast(caller)).policyScope);
+                if (graph.getDebug().isLogEnabled()) {
+                    graph.getDebug().logv(repeat("  ", inliningDepth) + "openCalleeScope for " + method.format("%H.%n(%p)") + ": " + policyScope);
+                }
             }
         }
     }
@@ -146,6 +163,10 @@ class InlineBeforeAnalysisMethodScope extends PEMethodScope {
                         new ConcurrentHashMap<>(), new ConcurrentHashMap<>(), true);
         this.bb = bb;
         this.policy = policy;
+
+        if (graph.getDebug().isLogEnabled()) {
+            graph.getDebug().logv("InlineBeforeAnalysis: decoding " + graph.method().format("%H.%n(%p)"));
+        }
     }
 
     @Override
@@ -192,8 +213,16 @@ protected void handleNonInlinedInvoke(MethodScope methodScope, LoopScope loopSco
 
     private void maybeAbortInlining(MethodScope ms, Node node) {
         InlineBeforeAnalysisMethodScope methodScope = cast(ms);
-        if (!methodScope.inliningAborted && methodScope.isInlinedMethod() && !policy.processNode(methodScope.policyScope, node)) {
-            methodScope.inliningAborted = true;
+        if (!methodScope.inliningAborted && methodScope.isInlinedMethod()) {
+            if (graph.getDebug().isLogEnabled()) {
+                graph.getDebug().logv(repeat("  ", methodScope.inliningDepth) + "  node " + node + ": " + methodScope.policyScope);
+            }
+            if (!policy.processNode(methodScope.policyScope, node)) {
+                if (graph.getDebug().isLogEnabled()) {
+                    graph.getDebug().logv(repeat("  ", methodScope.inliningDepth) + "    abort!");
+                }
+                methodScope.inliningAborted = true;
+            }
         }
     }
 
@@ -221,8 +250,12 @@ protected void finishInlining(MethodScope is) {
         InvokeData invokeData = inlineScope.invokeData;
 
         if (inlineScope.inliningAborted) {
-            policy.abortCalleeScope(callerScope.policyScope, inlineScope.policyScope);
-
+            if (graph.getDebug().isLogEnabled()) {
+                graph.getDebug().logv(repeat("  ", callerScope.inliningDepth) + "  aborted " + invokeData.callTarget.targetMethod().format("%H.%n(%p)") + ": " + inlineScope.policyScope);
+            }
+            if (callerScope.policyScope != null) {
+                policy.abortCalleeScope(callerScope.policyScope, inlineScope.policyScope);
+            }
             killControlFlowNodes(inlineScope, invokeData.invokePredecessor.next());
             assert invokeData.invokePredecessor.next() == null : "Successor must have been a fixed node created in the aborted scope, which is deleted now";
             invokeData.invokePredecessor.setNext(invokeData.invoke.asNode());
@@ -239,12 +272,26 @@ protected void finishInlining(MethodScope is) {
             return;
         }
 
-        policy.commitCalleeScope(callerScope.policyScope, inlineScope.policyScope);
+        if (graph.getDebug().isLogEnabled()) {
+            graph.getDebug().logv(repeat("  ", callerScope.inliningDepth) + "  committed " + invokeData.callTarget.targetMethod().format("%H.%n(%p)") + ": " + inlineScope.policyScope);
+        }
+        if (callerScope.policyScope != null) {
+            policy.commitCalleeScope(callerScope.policyScope, inlineScope.policyScope);
+        }
         ((AnalysisMethod) invokeData.callTarget.targetMethod()).registerAsInlined();
 
         super.finishInlining(inlineScope);
     }
 
+    /* String.repeat is only available in JDK 11 and later, so need to do our own. */
+    private static String repeat(String s, int count) {
+        StringBuilder result = new StringBuilder();
+        for (int i = 0; i < count; i++) {
+            result.append(s);
+        }
+        return result.toString();
+    }
+
     /**
      * Kill fixed nodes of structured control flow. Not as generic, but faster, than
      * {@link GraphUtil#killCFG}.

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/hub/ClassForNameSupport.java

@@ -58,11 +58,6 @@ public static Class<?> forNameOrNull(String className, ClassLoader classLoader)
         // Note: for non-predefined classes, we (currently) don't need to check the provided loader
         return result;
     }
-
-    /** Whether a call to {@link Class#forName} for the given class can be folded to a constant. */
-    public static boolean canBeFolded(Class<?> clazz) {
-        return !PredefinedClassesSupport.isPredefined(clazz);
-    }
 }
 
 @AutomaticFeature

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/phases/InlineBeforeAnalysisPolicyImpl.java

@@ -25,10 +25,12 @@
 package com.oracle.svm.hosted.phases;
 
 import org.graalvm.compiler.graph.Node;
-import org.graalvm.compiler.nodeinfo.NodeSize;
+import org.graalvm.compiler.nodes.AbstractBeginNode;
+import org.graalvm.compiler.nodes.AbstractEndNode;
 import org.graalvm.compiler.nodes.CallTargetNode;
 import org.graalvm.compiler.nodes.ConstantNode;
 import org.graalvm.compiler.nodes.FrameState;
+import org.graalvm.compiler.nodes.FullInfopointNode;
 import org.graalvm.compiler.nodes.Invoke;
 import org.graalvm.compiler.nodes.LogicConstantNode;
 import org.graalvm.compiler.nodes.ParameterNode;
@@ -39,6 +41,7 @@
 import org.graalvm.compiler.nodes.graphbuilderconf.GraphBuilderContext;
 import org.graalvm.compiler.nodes.java.AbstractNewObjectNode;
 import org.graalvm.compiler.nodes.java.NewArrayNode;
+import org.graalvm.compiler.nodes.spi.ValueProxy;
 import org.graalvm.compiler.options.Option;
 import org.graalvm.util.GuardedAnnotationAccess;
 
@@ -60,6 +63,12 @@
  * method above the limit. On the other hand, the inlining depth is generous because we do do not
  * need to limit it. Note that more experimentation is necessary to come up with the optimal
  * configuration.
+ * 
+ * Important: the implementation details of this class are publicly observable API. Since
+ * {@link java.lang.reflect.Method} constants can be produced by inlining lookup methods with
+ * constant arguments, reducing inlining can break customer code. This means we can never reduce the
+ * amount of inlining in a future version without breaking compatibility. This also means that we
+ * must be conservative and only inline what is necessary for known use cases.
  */
 public final class InlineBeforeAnalysisPolicyImpl extends InlineBeforeAnalysisPolicy<InlineBeforeAnalysisPolicyImpl.CountersScope> {
 
@@ -87,6 +96,11 @@ static final class CountersScope implements InlineBeforeAnalysisPolicy.Scope {
         CountersScope(CountersScope accumulated) {
             this.accumulated = accumulated;
         }
+
+        @Override
+        public String toString() {
+            return numNodes + "/" + numInvokes + " (" + accumulated.numNodes + "/" + accumulated.numInvokes + ")";
+        }
     }
 
     @Override
@@ -152,8 +166,13 @@ protected boolean processNode(CountersScope scope, Node node) {
             /* Infrastructure nodes that are not even visible to the policy. */
             throw VMError.shouldNotReachHere("Node must not be visible to policy: " + node.getClass().getTypeName());
         }
-        if (node.getNodeClass().size() == NodeSize.SIZE_0 || node instanceof FrameState) {
-            /* Infrastructure nodes that are never counted. */
+        if (node instanceof FullInfopointNode || node instanceof ValueProxy || node instanceof FrameState || node instanceof AbstractBeginNode || node instanceof AbstractEndNode) {
+            /*
+             * Infrastructure nodes that are never counted. We could look at the NodeSize annotation
+             * of a node, but that is a bit unreliable. For example, FrameState and
+             * ExceptionObjectNode have size != 0 but we do not want to count them; CallTargetNode
+             * has size 0 but we need to count it.
+             */
             return true;
         }
 

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/snippets/ReflectionPlugins.java

@@ -63,7 +63,7 @@
 import com.oracle.svm.core.SubstrateOptions;
 import com.oracle.svm.core.TypeResult;
 import com.oracle.svm.core.annotate.Delete;
-import com.oracle.svm.core.hub.ClassForNameSupport;
+import com.oracle.svm.core.hub.PredefinedClassesSupport;
 import com.oracle.svm.core.option.HostedOptionKey;
 import com.oracle.svm.core.util.VMError;
 import com.oracle.svm.hosted.ExceptionSynthesizer;
@@ -204,7 +204,6 @@ public boolean apply(GraphBuilderContext b, ResolvedJavaMethod targetMethod, Rec
 
     private void registerClassPlugins(InvocationPlugins plugins) {
         registerFoldInvocationPlugins(plugins, Class.class,
-                        "getClassLoader",
                         "isInterface", "isPrimitive",
                         "getField", "getMethod", "getConstructor",
                         "getDeclaredField", "getDeclaredMethod", "getDeclaredConstructor");
@@ -228,6 +227,12 @@ public boolean apply(GraphBuilderContext b, ResolvedJavaMethod targetMethod, Rec
                 return processClassForName(b, targetMethod, nameNode, initializeNode);
             }
         });
+        r.register1("getClassLoader", Receiver.class, new InvocationPlugin() {
+            @Override
+            public boolean apply(GraphBuilderContext b, ResolvedJavaMethod targetMethod, Receiver receiver) {
+                return processClassGetClassLoader(b, targetMethod, receiver);
+            }
+        });
     }
 
     private static final Constructor<MethodHandles.Lookup> LOOKUP_CONSTRUCTOR = ReflectionUtil.lookupConstructor(MethodHandles.Lookup.class, Class.class);
@@ -275,7 +280,7 @@ private boolean processClassForName(GraphBuilderContext b, ResolvedJavaMethod ta
             return throwException(b, targetMethod, targetParameters, e.getClass(), e.getMessage());
         }
         Class<?> clazz = typeResult.get();
-        if (!ClassForNameSupport.canBeFolded(clazz)) {
+        if (PredefinedClassesSupport.isPredefined(clazz)) {
             return false;
         }
 
@@ -290,6 +295,24 @@ private boolean processClassForName(GraphBuilderContext b, ResolvedJavaMethod ta
         return true;
     }
 
+    /**
+     * For {@link PredefinedClassesSupport predefined classes}, the class loader is not known yet at
+     * image build time. So we must not constant fold Class.getClassLoader for such classes. But for
+     * "normal" classes, it is important to fold it because it unlocks further constant folding of,
+     * e.g., Class.forName calls.
+     */
+    private boolean processClassGetClassLoader(GraphBuilderContext b, ResolvedJavaMethod targetMethod, Receiver receiver) {
+        Object classValue = unbox(b, receiver.get(false), JavaKind.Object);
+        if (!(classValue instanceof Class)) {
+            return false;
+        }
+        Class<?> clazz = (Class<?>) classValue;
+        if (PredefinedClassesSupport.isPredefined(clazz)) {
+            return false;
+        }
+        return pushConstant(b, targetMethod, () -> clazz.getName(), JavaKind.Object, clazz.getClassLoader()) != null;
+    }
+
     /**
      * Helper to register all declared methods by name only, to avoid listing all the complete
      * parameter types. It also simplifies handling of different JDK versions, because methods not