Skip to content

Commit

Permalink
Starlark: reuse positional array in native calls where possible
Browse files Browse the repository at this point in the history
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
  • Loading branch information
fmeum authored and copybara-github committed Jan 30, 2024
1 parent e57d93f commit aded06b
Show file tree
Hide file tree
Showing 4 changed files with 90 additions and 2 deletions.
16 changes: 14 additions & 2 deletions src/main/java/net/starlark/java/eval/BuiltinFunction.java
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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()) {
Expand Down
30 changes: 30 additions & 0 deletions src/main/java/net/starlark/java/eval/MethodDescriptor.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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. */
Expand Down Expand Up @@ -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;
}
}
25 changes: 25 additions & 0 deletions src/test/java/net/starlark/java/eval/MethodLibraryTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()
Expand Down
21 changes: 21 additions & 0 deletions src/test/java/net/starlark/java/eval/testdata/bench_call.star
Original file line number Diff line number Diff line change
@@ -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()

0 comments on commit aded06b

Please sign in to comment.