From aded06b8bd74c39d916bfb10645cf83add42f749 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Tue, 30 Jan 2024 13:19:13 -0800 Subject: [PATCH] Starlark: reuse positional array in native calls where possible 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 --- .../starlark/java/eval/BuiltinFunction.java | 16 ++++++++-- .../starlark/java/eval/MethodDescriptor.java | 30 +++++++++++++++++++ .../starlark/java/eval/MethodLibraryTest.java | 25 ++++++++++++++++ .../java/eval/testdata/bench_call.star | 21 +++++++++++++ 4 files changed, 90 insertions(+), 2 deletions(-) create mode 100644 src/test/java/net/starlark/java/eval/testdata/bench_call.star diff --git a/src/main/java/net/starlark/java/eval/BuiltinFunction.java b/src/main/java/net/starlark/java/eval/BuiltinFunction.java index aab7a8d6ea2f89..c7c4b5eff3c13e 100644 --- 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 index 53c4f5265dbceb..627c3a846d7d96 100644 --- 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. + * + *

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 index 0e76b265fe7a48..f2204aee53676e 100644 --- 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 new file mode 100644 index 00000000000000..5c0661947868de --- /dev/null +++ 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()