[Skylark] Move method invocation logic to MethodDescriptor.

This serves 2 purposes:
- better encapsulation and domain model.
- opens the door to optimizations like using `MethodHandle` instead of `Method`
  and caching.
- performs one of the optimizations mentioned above - perform `setAccessible`
  only once instead of on every method invocation. JMH suggests that this saves
  ~5% of CPU time.

Next steps are:
- evaluate peformance improvements for some of the optimizations listed above
- make PRs to apply optimizations that seem beneficial.

Closes #5704.

PiperOrigin-RevId: 206805670

4 files changed, 80 insertions, 75 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BuiltinCallable.java b/src/main/java/com/google/devtools/build/lib/syntax/BuiltinCallable.java
index aa37b35677..a7f704474f 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BuiltinCallable.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BuiltinCallable.java
@@ -125,8 +125,7 @@ public class BuiltinCallable extends BaseFunction {
     try (SilentCloseable c =
         Profiler.instance().profile(ProfilerTask.SKYLARK_BUILTIN_FN, getName())) {
       env.enterScope(this, SHARED_LEXICAL_FRAME_FOR_BUILTIN_METHOD_CALLS, ast, env.getGlobals());
-      return FuncallExpression.callMethod(
-          descriptor, getName(), obj, args, ast.getLocation(), env);
+      return descriptor.call(obj, args, ast.getLocation(), env);
     } finally {
       env.exitScope();
     }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
index ad8b50379e..8dec8f3b60 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
@@ -127,14 +127,14 @@ public final class DotExpression extends Expression {
       Optional<MethodDescriptor> method =
           Streams.stream(methods).filter(MethodDescriptor::isStructField).findFirst();
       if (method.isPresent() && method.get().isStructField()) {
-        return FuncallExpression.callMethod(
-            method.get(),
-            name,
-            objValue,
-            FuncallExpression.extraInterpreterArgs(method.get(), /* ast = */ null, loc, env)
-                .toArray(),
-            loc,
-            env);
+        return method
+            .get()
+            .call(
+                objValue,
+                FuncallExpression.extraInterpreterArgs(method.get(), /* ast = */ null, loc, env)
+                    .toArray(),
+                loc,
+                env);
       }
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
index e8df5abc0c..5994e03923 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
@@ -17,7 +17,6 @@ package com.google.devtools.build.lib.syntax;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
-import com.google.common.base.Throwables;
 import com.google.common.cache.CacheBuilder;
 import com.google.common.cache.CacheLoader;
 import com.google.common.cache.LoadingCache;
@@ -37,7 +36,6 @@ import com.google.devtools.build.lib.util.Pair;
 import java.io.IOException;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
-import java.lang.reflect.Modifier;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.HashMap;
@@ -394,60 +392,7 @@ public final class FuncallExpression extends Expression {
             || !methodDescriptor.isUseSkylarkSemantics()
             || !methodDescriptor.isUseLocation(),
         "Cannot be invoked on structField callables with extra interpreter params");
-    return callMethod(methodDescriptor, fieldName, obj, new Object[0], Location.BUILTIN, null);
-  }
-
-  static Object callMethod(MethodDescriptor methodDescriptor, String methodName, Object obj,
-      Object[] args, Location loc, Environment env) throws EvalException, InterruptedException {
-    try {
-      Method method = methodDescriptor.getMethod();
-      if (obj == null && !Modifier.isStatic(method.getModifiers())) {
-        throw new EvalException(loc, "method '" + methodName + "' is not static");
-      }
-      // This happens when the interface is public but the implementation classes
-      // have reduced visibility.
-      method.setAccessible(true);
-      Object result = method.invoke(obj, args);
-      if (method.getReturnType().equals(Void.TYPE)) {
-        return Runtime.NONE;
-      }
-      if (result == null) {
-        if (methodDescriptor.isAllowReturnNones()) {
-          return Runtime.NONE;
-        } else {
-          throw new EvalException(
-              loc,
-              "method invocation returned None, please file a bug report: "
-                  + methodName
-                  + Printer.printAbbreviatedList(
-                  ImmutableList.copyOf(args), "(", ", ", ")", null));
-        }
-      }
-      // TODO(bazel-team): get rid of this, by having everyone use the Skylark data structures
-      result = SkylarkType.convertToSkylark(result, method, env);
-      if (result != null && !EvalUtils.isSkylarkAcceptable(result.getClass())) {
-        throw new EvalException(
-            loc,
-            Printer.format(
-                "method '%s' returns an object of invalid type %r", methodName, result.getClass()));
-      }
-      return result;
-    } catch (IllegalAccessException e) {
-      // TODO(bazel-team): Print a nice error message. Maybe the method exists
-      // and an argument is missing or has the wrong type.
-      throw new EvalException(loc, "Method invocation failed: " + e);
-    } catch (InvocationTargetException e) {
-      if (e.getCause() instanceof FuncallException) {
-        throw new EvalException(loc, e.getCause().getMessage());
-      } else if (e.getCause() != null) {
-        Throwables.throwIfInstanceOf(e.getCause(), InterruptedException.class);
-
-        throw new EvalExceptionWithJavaCause(loc, e.getCause());
-      } else {
-        // This is unlikely to happen
-        throw new EvalException(loc, "method invocation failed: " + e);
-      }
-    }
+    return methodDescriptor.call(obj, new Object[0], Location.BUILTIN, null);
   }
 
   // TODO(bazel-team): If there's exactly one usable method, this works. If there are multiple
@@ -820,7 +765,7 @@ public final class FuncallExpression extends Expression {
       if (javaMethod.first.isStructField()) {
         // Not a method but a callable attribute
         try {
-          return callFunction(javaMethod.first.getMethod().invoke(obj), env);
+          return callFunction(javaMethod.first.invoke(obj), env);
         } catch (IllegalAccessException e) {
           throw new EvalException(getLocation(), "method invocation failed: " + e);
         } catch (InvocationTargetException e) {
@@ -834,7 +779,7 @@ public final class FuncallExpression extends Expression {
           }
         }
       }
-      return callMethod(javaMethod.first, method, obj, javaMethod.second.toArray(), location, env);
+      return javaMethod.first.call(obj, javaMethod.second.toArray(), location, env);
     }
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/MethodDescriptor.java b/src/main/java/com/google/devtools/build/lib/syntax/MethodDescriptor.java
index 978b15f7c4..05f0a36c5e 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/MethodDescriptor.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/MethodDescriptor.java
@@ -14,9 +14,13 @@
 
 package com.google.devtools.build.lib.syntax;
 
+import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
+import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
 import java.util.Arrays;
 
 /**
@@ -77,10 +81,6 @@ public final class MethodDescriptor {
     this.useSkylarkSemantics = useSkylarkSemantics;
   }
 
-  Method getMethod() {
-    return method;
-  }
-
   /** Returns the SkylarkCallable annotation corresponding to this method. */
   public SkylarkCallable getAnnotation() {
     return annotation;
@@ -88,6 +88,9 @@ public final class MethodDescriptor {
 
   /** @return Skylark method descriptor for provided Java method and signature annotation. */
   public static MethodDescriptor of(Method method, SkylarkCallable annotation) {
+    // This happens when the interface is public but the implementation classes
+    // have reduced visibility.
+    method.setAccessible(true);
     return new MethodDescriptor(
         method,
         annotation,
@@ -108,6 +111,64 @@ public final class MethodDescriptor {
         annotation.useSkylarkSemantics());
   }
 
+  /** @return The result of this method invocation on the {@code obj} as a target. */
+  public Object invoke(Object obj) throws InvocationTargetException, IllegalAccessException {
+    return method.invoke(obj);
+  }
+
+  /**
+   * Invokes this method using {@code obj} as a target and {@code args} as arguments.
+   *
+   * <p>{@code obj} may be {@code null} in case this method is static. Methods with {@code void}
+   * return type return {@code None} following Python convention.
+   */
+  public Object call(Object obj, Object[] args, Location loc, Environment env)
+      throws EvalException, InterruptedException {
+    try {
+      if (obj == null && !Modifier.isStatic(method.getModifiers())) {
+        throw new EvalException(loc, "method '" + getName() + "' is not static");
+      }
+      Object result = method.invoke(obj, args);
+      if (method.getReturnType().equals(Void.TYPE)) {
+        return Runtime.NONE;
+      }
+      if (result == null) {
+        if (isAllowReturnNones()) {
+          return Runtime.NONE;
+        } else {
+          throw new EvalException(
+              loc,
+              "method invocation returned None, please file a bug report: "
+                  + getName()
+                  + Printer.printAbbreviatedList(ImmutableList.copyOf(args), "(", ", ", ")", null));
+        }
+      }
+      // TODO(bazel-team): get rid of this, by having everyone use the Skylark data structures
+      result = SkylarkType.convertToSkylark(result, method, env);
+      if (result != null && !EvalUtils.isSkylarkAcceptable(result.getClass())) {
+        throw new EvalException(
+            loc,
+            Printer.format(
+                "method '%s' returns an object of invalid type %r", getName(), result.getClass()));
+      }
+      return result;
+    } catch (IllegalAccessException e) {
+      // TODO(bazel-team): Print a nice error message. Maybe the method exists
+      // and an argument is missing or has the wrong type.
+      throw new EvalException(loc, "Method invocation failed: " + e);
+    } catch (InvocationTargetException e) {
+      if (e.getCause() instanceof FuncallExpression.FuncallException) {
+        throw new EvalException(loc, e.getCause().getMessage());
+      } else if (e.getCause() != null) {
+        Throwables.throwIfInstanceOf(e.getCause(), InterruptedException.class);
+        throw new EvalException.EvalExceptionWithJavaCause(loc, e.getCause());
+      } else {
+        // This is unlikely to happen
+        throw new EvalException(loc, "method invocation failed: " + e);
+      }
+    }
+  }
+
   /** @see SkylarkCallable#name() */
   public String getName() {
     return name;
@@ -124,7 +185,7 @@ public final class MethodDescriptor {
   }
 
   /** @see SkylarkCallable#useSkylarkSemantics() */
-  public boolean isUseSkylarkSemantics() {
+  boolean isUseSkylarkSemantics() {
     return useSkylarkSemantics;
   }
 
@@ -153,12 +214,12 @@ public final class MethodDescriptor {
   }
 
   /** @return {@code true} if this method accepts extra arguments ({@code *args}) */
-  public boolean isAcceptsExtraArgs() {
+  boolean isAcceptsExtraArgs() {
     return !getExtraPositionals().getName().isEmpty();
   }
 
   /** @see SkylarkCallable#extraKeywords() */
-  public boolean isAcceptsExtraKwargs() {
+  boolean isAcceptsExtraKwargs() {
     return !getExtraKeywords().getName().isEmpty();
   }