Merge pull request #8172 from headius/optimized_method_callee

Optimized __method__ and __callee__
jruby · Mar 29, 2024 · 4216efb · 4216efb
2 parents 5a08af3 + 8d263bb
commit 4216efb
Show file tree

Hide file tree

Showing 20 changed files with 337 additions and 11 deletions.
diff --git a/bench/bench_frame_name_methods.rb b/bench/bench_frame_name_methods.rb
@@ -0,0 +1,43 @@
+require 'benchmark/ips'
+
+def foo_method
+  __method__
+end
+
+def foo_callee
+  __callee__
+end
+
+alias bar_method foo_method
+alias bar_callee foo_callee
+
+Benchmark.ips do |bm|
+  bm.report("__method__ same") do |i|
+    while i > 0
+      i-=1
+      foo_method;foo_method;foo_method;foo_method;foo_method;foo_method;foo_method;foo_method;foo_method;foo_method
+    end
+  end
+
+  bm.report("__method__ different") do |i|
+    while i > 0
+      i-=1
+      bar_method;bar_method;bar_method;bar_method;bar_method;bar_method;bar_method;bar_method;bar_method;bar_method
+    end
+  end
+
+
+  bm.report("__callee__ same") do |i|
+    while i > 0
+      i-=1
+      foo_callee;foo_callee;foo_callee;foo_callee;foo_callee;foo_callee;foo_callee;foo_callee;foo_callee;foo_callee
+    end
+  end
+
+  bm.report("__callee__ different") do |i|
+    while i > 0
+      i-=1
+      bar_callee;bar_callee;bar_callee;bar_callee;bar_callee;bar_callee;bar_callee;bar_callee;bar_callee;bar_callee
+    end
+  end
+end
diff --git a/core/src/main/java/org/jruby/RubySymbol.java b/core/src/main/java/org/jruby/RubySymbol.java
@@ -71,6 +71,9 @@
 import org.jruby.util.TypeConverter;
 
 import java.lang.ref.WeakReference;
+import java.util.AbstractMap;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.locks.ReentrantLock;
 
 import static org.jruby.util.RubyStringBuilder.str;
@@ -346,6 +349,14 @@ public static RubySymbol newHardSymbol(Ruby runtime, String name) {
         return runtime.getSymbolTable().getSymbol(name, true);
     }
 
+    public static RubySymbol newMethodSymbolFromCompound(Ruby runtime, String compoundName) {
+        return runtime.getSymbolTable().getMethodSymbolFromCompound(compoundName);
+    }
+
+    public static RubySymbol newCalleeSymbolFromCompound(Ruby runtime, String compoundName) {
+        return runtime.getSymbolTable().getCalleeSymbolFromCompound(compoundName);
+    }
+
     /**
      * Return the symbol in the symbol table if it exists, null otherwise.
      * This method will not create the symbol if it does not exist.
@@ -979,6 +990,7 @@ public static final class SymbolTable {
 
         private final ReentrantLock tableLock = new ReentrantLock();
         private volatile SymbolEntry[] symbolTable;
+        private Map<String, CompoundSymbol> compoundSymbolTable = new ConcurrentHashMap<>();
         private int size;
         private int threshold;
         private final float loadFactor;
@@ -1093,6 +1105,46 @@ public RubySymbol getSymbol(ByteList bytes, ObjBooleanConsumer<RubySymbol> handl
             return symbol;
         }
 
+        /**
+         * Get the method name symbol from a compound name.
+         *
+         * @see #getCompoundSymbol(String)
+         * @param compoundName the compound name
+         * @return the method component of the compound name, as a symbol
+         */
+        public RubySymbol getMethodSymbolFromCompound(String compoundName) {
+            return getCompoundSymbol(compoundName).method;
+        }
+
+
+        /**
+         * Get the callee name symbol from a compound name.
+         *
+         * @see #getCompoundSymbol(String)
+         * @param compoundName the compound name
+         * @return the callee component of the compound name, as a symbol
+         */
+        public RubySymbol getCalleeSymbolFromCompound(String compoundName) {
+            return getCompoundSymbol(compoundName).callee;
+        }
+
+        /**
+         * Get a pair of symbols associated with the given compound method name, used by aliases to pass both the callee
+         * name and the original method name on the stack. This avoids re-parsing the compoundName and constructing new
+         * strings every time __method__ or __callee__ are used in an aliased call.
+         *
+         * @param compoundName the compound name used for a combination of alias and method
+         * @return a Map.Entry representing the __method__ and __callee__ symbols for that compound name as key and value
+         */
+        private CompoundSymbol getCompoundSymbol(String compoundName) {
+            return compoundSymbolTable.computeIfAbsent(compoundName, (cname) ->
+                    new CompoundSymbol(
+                            getSymbol(Helpers.getSuperNameFromCompositeName(cname), true),
+                            getSymbol(Helpers.getCalleeNameFromCompositeName(cname), true)));
+        }
+
+        private record CompoundSymbol(RubySymbol method, RubySymbol callee){}
+
         private RubySymbol findSymbol(ByteList bytes, int hash, boolean hard) {
             RubySymbol symbol = null;
 

diff --git a/core/src/main/java/org/jruby/internal/runtime/methods/InterpretedIRBodyMethod.java b/core/src/main/java/org/jruby/internal/runtime/methods/InterpretedIRBodyMethod.java
@@ -71,7 +71,7 @@ private IRubyObject INTERPRET_CLASS(InterpreterContext ic, ThreadContext context
     private IRubyObject interpretWithBacktrace(InterpreterContext ic, ThreadContext context, IRubyObject self, String name, Block block) {
         try {
             ThreadContext.pushBacktrace(context, name, ic.getFileName(), ic.getLine());
-            return ic.getEngine().interpret(context, null, self, ic, getImplementationClass().getMethodLocation(), name, block);
+            return ic.getEngine().interpret(context, null, self, ic, getImplementationClass().getMethodLocation(), null, block);
         } finally {
             ThreadContext.popBacktrace(context);
         }

diff --git a/core/src/main/java/org/jruby/ir/IRVisitor.java b/core/src/main/java/org/jruby/ir/IRVisitor.java
@@ -65,6 +65,7 @@ private void error(Object object) {
     public void EQQInstr(EQQInstr eqqinstr) { error(eqqinstr); }
     public void ExceptionRegionEndMarkerInstr(ExceptionRegionEndMarkerInstr exceptionregionendmarkerinstr) { error(exceptionregionendmarkerinstr); }
     public void ExceptionRegionStartMarkerInstr(ExceptionRegionStartMarkerInstr exceptionregionstartmarkerinstr) { error(exceptionregionstartmarkerinstr); }
+    public void FrameNameCallInstr(FrameNameCallInstr framenamecallinstr) { error(framenamecallinstr); }
     public void GetClassVarContainerModuleInstr(GetClassVarContainerModuleInstr getclassvarcontainermoduleinstr) { error(getclassvarcontainermoduleinstr); }
     public void GetClassVariableInstr(GetClassVariableInstr getclassvariableinstr) { error(getclassvariableinstr); }
     public void GetFieldInstr(GetFieldInstr getfieldinstr) { error(getfieldinstr); }

diff --git a/core/src/main/java/org/jruby/ir/Operation.java b/core/src/main/java/org/jruby/ir/Operation.java
@@ -84,6 +84,7 @@ public enum Operation {
     CALL_0O(OpFlags.f_has_side_effect | OpFlags.f_is_call | OpFlags.f_can_raise_exception),
     NORESULT_CALL_1O(OpFlags.f_has_side_effect | OpFlags.f_is_call | OpFlags.f_can_raise_exception),
     BLOCK_GIVEN_CALL(OpFlags.f_has_side_effect | OpFlags.f_is_call | OpFlags.f_can_raise_exception),
+    FRAME_NAME_CALL(OpFlags.f_has_side_effect | OpFlags.f_is_call | OpFlags.f_can_raise_exception),
 
     /** Ruby operators: should all these be calls? Implementing instrs don't inherit from CallBase.java */
     EQQ(OpFlags.f_has_side_effect | OpFlags.f_can_raise_exception), // a === call used in when

diff --git a/core/src/main/java/org/jruby/ir/builder/IRBuilderAST.java b/core/src/main/java/org/jruby/ir/builder/IRBuilderAST.java
@@ -2486,6 +2486,19 @@ public Operand buildVAlias(VAliasNode valiasNode) {
     }
 
     public Operand buildVCall(Variable result, VCallNode node) {
+        if (result == null) result = temp();
+
+        RubySymbol name = methodName = node.getName();
+
+        // special case methods with frame handling
+        String callName = name.idString();
+        switch (callName) {
+            case "__method__":
+            case "__callee__":
+                addInstr(new FrameNameCallInstr(result, callName));
+                return result;
+        }
+
         return _call(result, VARIABLE, buildSelf(), node.getName());
     }
 

diff --git a/core/src/main/java/org/jruby/ir/instructions/DefineClassInstr.java b/core/src/main/java/org/jruby/ir/instructions/DefineClassInstr.java
@@ -86,7 +86,7 @@ private IRubyObject INTERPRET_CLASS(ThreadContext context, RubyModule clazz) {
 
         try {
             ThreadContext.pushBacktrace(context, id, ic.getFileName(), ic.getLine());
-            return ic.getEngine().interpret(context, null, clazz, ic, clazz.getMethodLocation(), id, Block.NULL_BLOCK);
+            return ic.getEngine().interpret(context, null, clazz, ic, clazz.getMethodLocation(), null, Block.NULL_BLOCK);
         } finally {
             body.cleanupAfterExecution();
             if (!hasExplicitCallProtocol) post(ic, context);

diff --git a/core/src/main/java/org/jruby/ir/instructions/DefineModuleInstr.java b/core/src/main/java/org/jruby/ir/instructions/DefineModuleInstr.java
@@ -77,7 +77,7 @@ private IRubyObject INTERPRET_MODULE(ThreadContext context, RubyModule clazz) {
 
         try {
             ThreadContext.pushBacktrace(context, id, ic.getFileName(), ic.getLine());
-            return ic.getEngine().interpret(context, null, clazz, ic, clazz.getMethodLocation(), id, Block.NULL_BLOCK);
+            return ic.getEngine().interpret(context, null, clazz, ic, clazz.getMethodLocation(), null, Block.NULL_BLOCK);
         } finally {
             body.cleanupAfterExecution();
             if (!hasExplicitCallProtocol) post(ic, context);

diff --git a/core/src/main/java/org/jruby/ir/instructions/FrameNameCallInstr.java b/core/src/main/java/org/jruby/ir/instructions/FrameNameCallInstr.java
@@ -0,0 +1,70 @@
+package org.jruby.ir.instructions;
+
+import org.jruby.RubySymbol;
+import org.jruby.ir.IRVisitor;
+import org.jruby.ir.Operation;
+import org.jruby.ir.operands.Variable;
+import org.jruby.ir.persistence.IRReaderDecoder;
+import org.jruby.ir.persistence.IRWriterEncoder;
+import org.jruby.ir.transformations.inlining.CloneInfo;
+import org.jruby.runtime.ThreadContext;
+import org.jruby.runtime.builtin.IRubyObject;
+import org.jruby.runtime.callsite.CacheEntry;
+import org.jruby.runtime.callsite.VariableCachingCallSite;
+
+import java.util.Objects;
+
+/**
+ * A call to __method__ or __callee__ which can be optimized to use the frame method name directly.
+ */
+public class FrameNameCallInstr extends NoOperandResultBaseInstr implements FixedArityInstr {
+    private final VariableCachingCallSite frameNameSite;
+    private final boolean callee;
+
+    public FrameNameCallInstr(Variable result, String methodName) {
+        super(Operation.FRAME_NAME_CALL, Objects.requireNonNull(result, "FrameNameCallInstr result is null"));
+
+        this.frameNameSite =
+                new VariableCachingCallSite(Objects.requireNonNull(methodName, "FrameNameCallInstr methodName is null"));
+
+        this.callee = methodName.equals("__callee__");
+    }
+
+    public String getMethodName() {
+        return frameNameSite.getMethodName();
+    }
+
+    @Override
+    public Instr clone(CloneInfo ii) {
+        return new FrameNameCallInstr(ii.getRenamedVariable(result), frameNameSite.getMethodName());
+    }
+
+    public static FrameNameCallInstr decode(IRReaderDecoder d) {
+        return new FrameNameCallInstr(d.decodeVariable(), d.decodeString());
+    }
+
+    @Override
+    public void encode(IRWriterEncoder e) {
+        super.encode(e);
+        e.encode(frameNameSite.getMethodName());
+    }
+
+    public IRubyObject getFrameName(ThreadContext context, IRubyObject self, String compositeName) {
+        CacheEntry frameNameEntry = frameNameSite.retrieveCache(self);
+
+        if (!frameNameEntry.method.getRealMethod().isBuiltin()) {
+            return frameNameSite.call(context, self, self);
+        }
+
+        if (compositeName == null) return context.nil;
+
+        return callee ?
+                RubySymbol.newCalleeSymbolFromCompound(context.runtime, compositeName):
+                RubySymbol.newMethodSymbolFromCompound(context.runtime, compositeName);
+    }
+
+    @Override
+    public void visit(IRVisitor visitor) {
+        visitor.FrameNameCallInstr(this);
+    }
+}
diff --git a/core/src/main/java/org/jruby/ir/interpreter/ExitableInterpreterEngine.java b/core/src/main/java/org/jruby/ir/interpreter/ExitableInterpreterEngine.java
@@ -107,7 +107,7 @@ public ExitableReturn interpret(ThreadContext context, Block block, IRubyObject
                         break;
                     case CALL_OP:
                         if (profile) Profiler.updateCallSite(instr, interpreterContext.getScope(), scopeVersion);
-                        processCall(context, instr, operation, currDynScope, currScope, temp, self);
+                        processCall(context, instr, operation, currDynScope, currScope, temp, self, name, block);
                         break;
                     case RET_OP:
                         processReturnOp(context, block, instr, operation, currDynScope, temp, self, currScope);

diff --git a/core/src/main/java/org/jruby/ir/interpreter/InterpreterEngine.java b/core/src/main/java/org/jruby/ir/interpreter/InterpreterEngine.java
@@ -11,6 +11,7 @@
 import org.jruby.ir.instructions.CheckArityInstr;
 import org.jruby.ir.instructions.CheckForLJEInstr;
 import org.jruby.ir.instructions.CopyInstr;
+import org.jruby.ir.instructions.FrameNameCallInstr;
 import org.jruby.ir.instructions.Instr;
 import org.jruby.ir.instructions.JumpInstr;
 import org.jruby.ir.instructions.LineNumberInstr;
@@ -150,7 +151,7 @@ public IRubyObject interpret(ThreadContext context, Block block, IRubyObject sel
                         break;
                     case CALL_OP:
                         if (profile) Profiler.updateCallSite(instr, interpreterContext.getScope(), scopeVersion);
-                        processCall(context, instr, operation, currDynScope, currScope, temp, self);
+                        processCall(context, instr, operation, currDynScope, currScope, temp, self, name, block);
                         break;
                     case RET_OP:
                         return processReturnOp(context, block, instr, operation, currDynScope, temp, self, currScope);
@@ -286,7 +287,7 @@ protected static void receiveArg(ThreadContext context, Instr i, Operation opera
         }
     }
 
-    protected static void processCall(ThreadContext context, Instr instr, Operation operation, DynamicScope currDynScope, StaticScope currScope, Object[] temp, IRubyObject self) {
+    protected static void processCall(ThreadContext context, Instr instr, Operation operation, DynamicScope currDynScope, StaticScope currScope, Object[] temp, IRubyObject self, String frameName, Block selfBlock) {
         Object result;
 
         switch(operation) {
@@ -358,6 +359,11 @@ protected static void processCall(ThreadContext context, Instr instr, Operation
             case NORESULT_CALL:
                 instr.interpret(context, currScope, currDynScope, self, temp);
                 break;
+            case FRAME_NAME_CALL:
+                setResult(temp, currDynScope, instr,
+                        ((FrameNameCallInstr) instr).getFrameName(
+                                context, self, selfBlock == null ? frameName : IRRuntimeHelpers.getFrameNameFromBlock(selfBlock)));
+                break;
             case CALL:
             default:
                 result = instr.interpret(context, currScope, currDynScope, self, temp);

diff --git a/core/src/main/java/org/jruby/ir/interpreter/StartupInterpreterEngine.java b/core/src/main/java/org/jruby/ir/interpreter/StartupInterpreterEngine.java
@@ -61,7 +61,7 @@ public IRubyObject interpret(ThreadContext context, Block block, IRubyObject sel
                         break;
                     case CALL_OP:
                         if (profile) Profiler.updateCallSite(instr, interpreterContext.getScope(), scopeVersion);
-                        processCall(context, instr, operation, currDynScope, currScope, temp, self);
+                        processCall(context, instr, operation, currDynScope, currScope, temp, self, name, block);
                         break;
                     case RET_OP:
                         return processReturnOp(context, block, instr, operation, currDynScope, temp, self, currScope);

diff --git a/core/src/main/java/org/jruby/ir/persistence/IRReaderStream.java b/core/src/main/java/org/jruby/ir/persistence/IRReaderStream.java
@@ -295,6 +295,7 @@ public Instr decodeInstr() {
             case EQQ: return EQQInstr.decode(this);
             case EXC_REGION_END: return new ExceptionRegionEndMarkerInstr();
             case EXC_REGION_START: return ExceptionRegionStartMarkerInstr.decode(this);
+            case FRAME_NAME_CALL: return FrameNameCallInstr.decode(this);
             case GET_CVAR: return GetClassVariableInstr.decode(this);
             case GET_ENCODING: return GetEncodingInstr.decode(this);
             case GET_ERROR_INFO: return GetErrorInfoInstr.decode(this);

diff --git a/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java b/core/src/main/java/org/jruby/ir/runtime/IRRuntimeHelpers.java
@@ -1964,7 +1964,7 @@ public static void defCompiledInstanceMethod(ThreadContext context, MethodHandle
     public static IRubyObject invokeModuleBody(ThreadContext context, DynamicMethod method) {
         RubyModule implClass = method.getImplementationClass();
 
-        return method.call(context, implClass, implClass, "", Block.NULL_BLOCK);
+        return method.call(context, implClass, implClass, null, Block.NULL_BLOCK);
     }
 
     @JIT
@@ -2662,4 +2662,10 @@ private static IRRuntimeHelpersSites sites(ThreadContext context) {
     public static int arrayLength(RubyArray array) {
         return array.getLength();
     }
+
+    @Interp @JIT
+    public static String getFrameNameFromBlock(Block block) {
+        // FIXME: binding.getMethod does not appear to be the right name in defined_method bodies... WHY?
+        return block.getBinding().getFrame().getName();
+    }
 }
diff --git a/core/src/main/java/org/jruby/ir/targets/IRBytecodeAdapter.java b/core/src/main/java/org/jruby/ir/targets/IRBytecodeAdapter.java
@@ -318,8 +318,7 @@ public void loadFrameName() {
         if (superNameOffset == -1) {
             // load from self block
             loadSelfBlock();
-            adapter.invokevirtual(p(Block.class), "getBinding", sig(Binding.class));
-            adapter.invokevirtual(p(Binding.class), "getMethod", sig(String.class));
+            adapter.invokestatic(p(IRRuntimeHelpers.class), "getFrameNameFromBlock", sig(String.class, Block.class));
         } else {
             adapter.aload(superNameOffset);
         }

diff --git a/core/src/main/java/org/jruby/ir/targets/InvocationCompiler.java b/core/src/main/java/org/jruby/ir/targets/InvocationCompiler.java
@@ -133,4 +133,9 @@ public interface InvocationCompiler {
      * Invoke block_given? or iterator? with awareness of any built-in methods.
      */
     void invokeBlockGiven(String methodName, String file);
+
+    /**
+     * Invoke __method__ or __callee__ with awareness of any built-in methods.
+     */
+    void invokeFrameName(String methodName, String file);
 }
diff --git a/core/src/main/java/org/jruby/ir/targets/JVMVisitor.java b/core/src/main/java/org/jruby/ir/targets/JVMVisitor.java
@@ -1650,6 +1650,12 @@ public void ExceptionRegionStartMarkerInstr(ExceptionRegionStartMarkerInstr exce
         throw new NotCompilableException("Marker instructions shouldn't reach compiler: " + exceptionregionstartmarkerinstr);
     }
 
+    @Override
+    public void FrameNameCallInstr(FrameNameCallInstr framenamecallinstr) {
+        jvmMethod().getInvocationCompiler().invokeFrameName(framenamecallinstr.getMethodName(), file);
+        handleCallResult(jvmMethod(), framenamecallinstr.getResult());
+    }
+
     @Override
     public void GetClassVarContainerModuleInstr(GetClassVarContainerModuleInstr getclassvarcontainermoduleinstr) {
         jvmMethod().loadContext();