Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Starlark: reuse positional array in native calls where possible #19708

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/main/java/net/starlark/java/eval/BuiltinFunction.java
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,16 @@ private Object[] getArgumentVector(

ParamDescriptor[] parameters = desc.getParameters();

// Fast case: reuse positional as Java argument vector
// StringModule methods, which are treated specially below, will never match this case since
// their "self" parameter is restricted to String and thus
// getPositionalsCanBeJavaArgumentVector() is false.
if (desc.getPositionalsCanBeJavaArgumentVector()
&& positional.length == parameters.length
&& named.length == 0) {
return positional;
}

// Allocate argument vector.
int n = parameters.length;
if (desc.acceptsExtraArgs()) {
Expand Down
23 changes: 23 additions & 0 deletions src/main/java/net/starlark/java/eval/MethodDescriptor.java
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ final class MethodDescriptor {
private final boolean allowReturnNones;
private final boolean useStarlarkThread;
private final boolean useStarlarkSemantics;
private final boolean positionalsCanBeJavaArgumentVector;

private enum HowToHandleReturn {
NULL_TO_NONE, // any Starlark value; null -> None
Expand Down Expand Up @@ -100,6 +101,20 @@ private MethodDescriptor(
} else {
howToHandleReturn = HowToHandleReturn.FROM_JAVA;
}

this.positionalsCanBeJavaArgumentVector =
!extraKeywords
&& !extraPositionals
&& !useStarlarkSemantics
&& !useStarlarkThread
&& Arrays.stream(parameters)
.allMatch(MethodDescriptor::paramCanBeUsedAsPositionalWithoutChecks);
}

private static boolean paramCanBeUsedAsPositionalWithoutChecks(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 +302,12 @@ String getDoc() {
boolean isSelfCall() {
return selfCall;
}

/**
* Returns true if positional arguments can be used as a Java method call argument vector without
* further checks when parameter count matches.
*/
boolean getPositionalsCanBeJavaArgumentVector() {
return positionalsCanBeJavaArgumentVector;
}
}
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 {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this test to document why I think that #12782 (comment) isn't a safe optimization, but I may have misunderstood the original comment.

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()
Loading