Starlark: reuse positional array in native calls where possible

fmeum · copybara-github · commit aded06b8bd74 · 2024-01-30T13:20:43.000-08:00
This is a resubmission of @stepancheg's #12782, which became stale, with this original description: ==== For certain calls like `type(x)` or `str(x)` or `l.append(x)` we can reuse `positional` array argument of `fastcall` to pass it to the `MethodDescriptor.call` and skip all runtime checks in `BuiltinFunction`. This optimization cannot be applied if * a function has extra positional or named arguments * has arguments guarded by flag * accepts extra `StarlarkThread` parameter * has unfilled arguments with default values So this optimation won't be used for calls `list(x)` or `bool()`. For `type(1)` called in loop, the result is 15% speedup: ``` A: n=30 mean=5.705 std=0.387 se=0.071 min=5.131 med=5.827 B: n=30 mean=4.860 std=0.345 se=0.064 min=4.396 med=4.946 B/A: 0.852 0.820..0.884 (95% conf) ``` For `bool()` (no argument) called in loop, it results in 1% slowdown: ``` A: n=29 mean=9.045 std=0.603 se=0.113 min=8.096 med=9.400 B: n=29 mean=9.134 std=0.520 se=0.098 min=8.248 med=9.448 B/A: 1.010 0.976..1.045 (95% conf) ``` ==== Beyond the original PR, this includes benchmarks, a test case addressing a review comment and small adaptions to the implementation to make it work with recent changes. Benchmark results with JDK 21 and `--seconds 5`: ``` before: File src/test/java/net/starlark/java/eval/testdata/bench_call.star: benchmark ops cpu/op wall/op steps/op alloc/op bench_call_bool 67108863 100ns 100ns 3 87B bench_call_dict_get 33554431 159ns 158ns 5 143B bench_call_dict_get_none 33554431 180ns 179ns 6 143B bench_call_list_append 33554431 170ns 169ns 5 207B bench_call_type 67108863 139ns 138ns 4 111B after: File src/test/java/net/starlark/java/eval/testdata/bench_call.star: benchmark ops cpu/op wall/op steps/op alloc/op bench_call_bool 67108863 100ns 100ns 3 87B bench_call_dict_get 33554431 155ns 154ns 5 143B bench_call_dict_get_none 33554431 174ns 174ns 6 143B bench_call_list_append 67108863 141ns 141ns 5 183B bench_call_type 67108863 115ns 114ns 4 87B ``` Closes #19708. PiperOrigin-RevId: 602821608 Change-Id: If40e2eb1e09ec43d56b36ba13987aa5312fd47ec
diff --git a/src/main/java/net/starlark/java/eval/BuiltinFunction.java b/src/main/java/net/starlark/java/eval/BuiltinFunction.java
@@ -128,9 +128,11 @@ public String toString() {
    * @param thread the Starlark thread for the call
    * @param loc the location of the call expression, or BUILTIN for calls from Java
    * @param desc descriptor for the StarlarkMethod-annotated method
-   * @param positional a list of positional arguments
+   * @param positional an array of positional arguments; as an optimization, in simple cases, this
+   *     array may be reused as the method's return value
    * @param named a list of named arguments, as alternating Strings/Objects. May contain dups.
-   * @return the array of arguments which may be passed to {@link MethodDescriptor#call}
+   * @return the array of arguments which may be passed to {@link MethodDescriptor#call}. It is
+   *     unsafe to mutate the returned array.
    * @throws EvalException if the given set of arguments are invalid for the given method. For
    *     example, if any arguments are of unexpected type, or not all mandatory parameters are
    *     specified by the user
@@ -156,6 +158,16 @@ private Object[] getArgumentVector(
 
     ParamDescriptor[] parameters = desc.getParameters();
 
+    // Fast case: we only accept positionals and can reuse `positional` as the Java args vector.
+    // Note that StringModule methods, which are treated specially below, will never match this case
+    // since their `self` parameter is restricted to String and thus
+    // isPositionalsReusableAsCallArgsVectorIfArgumentCountValid() would be false.
+    if (desc.isPositionalsReusableAsJavaArgsVectorIfArgumentCountValid()
+        && positional.length == parameters.length
+        && named.length == 0) {
+      return positional;
+    }
+
     // Allocate argument vector.
     int n = parameters.length;
     if (desc.acceptsExtraArgs()) {
diff --git a/src/main/java/net/starlark/java/eval/MethodDescriptor.java b/src/main/java/net/starlark/java/eval/MethodDescriptor.java
@@ -14,6 +14,8 @@
 
 package net.starlark.java.eval;
 
+import static java.util.Arrays.stream;
+
 import com.google.common.base.Preconditions;
 import com.google.common.base.Throwables;
 import com.google.errorprone.annotations.CheckReturnValue;
@@ -46,6 +48,7 @@ final class MethodDescriptor {
   private final boolean allowReturnNones;
   private final boolean useStarlarkThread;
   private final boolean useStarlarkSemantics;
+  private final boolean positionalsReusableAsJavaArgsVectorIfArgumentCountValid;
 
   private enum HowToHandleReturn {
     NULL_TO_NONE, // any Starlark value; null -> None
@@ -100,6 +103,19 @@ private MethodDescriptor(
     } else {
       howToHandleReturn = HowToHandleReturn.FROM_JAVA;
     }
+
+    this.positionalsReusableAsJavaArgsVectorIfArgumentCountValid =
+        !extraKeywords
+            && !extraPositionals
+            && !useStarlarkSemantics
+            && !useStarlarkThread
+            && stream(parameters).allMatch(MethodDescriptor::paramUsableAsPositionalWithoutChecks);
+  }
+
+  private static boolean paramUsableAsPositionalWithoutChecks(ParamDescriptor param) {
+    return param.isPositional()
+        && param.disabledByFlag() == null
+        && param.getAllowedClasses() == null;
   }
 
   /** Returns the StarlarkMethod annotation corresponding to this method. */
@@ -287,4 +303,18 @@ String getDoc() {
   boolean isSelfCall() {
     return selfCall;
   }
+
+  /**
+   * Returns true if we may directly reuse the Starlark positionals vector as the Java {@code args}
+   * vector passed to {@link #call} as long as the Starlark call was made with a valid number of
+   * arguments.
+   *
+   * <p>More precisely, this means that we do not need to insert extra values into the args vector
+   * (such as ones corresponding to {@code *args}, {@code **kwargs}, or {@code self} in Starlark),
+   * and all Starlark parameters are simple positional parameters which cannot be disabled by a flag
+   * and do not require type checking.
+   */
+  boolean isPositionalsReusableAsJavaArgsVectorIfArgumentCountValid() {
+    return positionalsReusableAsJavaArgsVectorIfArgumentCountValid;
+  }
 }
diff --git a/src/test/java/net/starlark/java/eval/MethodLibraryTest.java b/src/test/java/net/starlark/java/eval/MethodLibraryTest.java
@@ -22,7 +22,9 @@
 import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.ImmutableList;
 import java.util.List;
+import net.starlark.java.annot.Param;
 import net.starlark.java.annot.StarlarkBuiltin;
+import net.starlark.java.annot.StarlarkMethod;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -772,6 +774,29 @@ ev.new Scenario()
             "','.join(elements=['foo', 'bar'])");
   }
 
+  @StarlarkBuiltin(name = "named_only", doc = "")
+  static final class NamedOnly implements StarlarkValue {
+    @StarlarkMethod(
+        name = "foo",
+        documented = false,
+        parameters = {
+          @Param(name = "a"),
+          @Param(name = "b", named = true, defaultValue = "None", positional = false),
+        })
+    public Object foo(Object a, Object b) {
+      return a;
+    }
+  }
+
+  @Test
+  public void testNamedOnlyArgument() throws Exception {
+    ev.new Scenario()
+        .update("named_only", new NamedOnly())
+        .testIfErrorContains(
+            "foo() accepts no more than 1 positional argument but got 2",
+            "named_only.foo([1, 2, 3], int)");
+  }
+
   @Test
   public void testStringJoinRequiresStrings() throws Exception {
     ev.new Scenario()
diff --git a/src/test/java/net/starlark/java/eval/testdata/bench_call.star b/src/test/java/net/starlark/java/eval/testdata/bench_call.star
@@ -0,0 +1,21 @@
+def bench_call_type(b):
+    for _ in range(b.n):
+        type(1)
+
+def bench_call_list_append(b):
+    for _ in range(b.n):
+        [].append("foo")
+
+def bench_call_dict_get(b):
+    d = {"foo": "bar"}
+    for _ in range(b.n):
+        d.get("baz")
+
+def bench_call_dict_get_none(b):
+    d = {"foo": "bar"}
+    for _ in range(b.n):
+        d.get("baz", None)
+
+def bench_call_bool(b):
+    for _ in range(b.n):
+        bool()
