Skip to content
This repository has been archived by the owner. It is now read-only.

Callables should handle null args and kwargs #864

Merged
merged 28 commits into from Jul 26, 2018

Conversation

@patiences
Copy link
Contributor

@patiences patiences commented Jul 3, 2018

When functions are called, two data structures are created to hold their arguments: a java array for the args and a java hashmap for the kwargs. These are created even when the callable takes no arguments. We can prevent creating these empty data structures by making sure there are sufficient null checks when the callable is invoked.

(This is a copy of this PR)

@patiences patiences changed the title Use preallocated vars for functions without args Callables should handle null args and kwargs Jul 3, 2018
@patiences
Copy link
Contributor Author

@patiences patiences commented Jul 3, 2018

Current status:

I'm unable to show that this change results in a performance boost, and I think this is because while not creating some arrays and hashmaps that will just be empty is an improvement, the extensive null checking on invocation cancels out the savings. If that is the case, I'm not sure this PR is a good idea because it could mean that performance worsens a bit for callables that do take arguments but we have to do the null checking anyway. Thoughts @freakboy3742? :-)

Copy link
Member

@freakboy3742 freakboy3742 left a comment

The code that's here is good; but I think you've missed a couple of opportunities for optimisation, which possibly explains why you're not seeing an speedup. You've optimized the bytecode creation around invoking the function call, but the logic handling the invocation is just as complex (possibly more so, because there extra branch instructions).

I think if you dig into the invoke methods and to find paths that avoid creating new empty arrays, you might see a speedup.

java.lang.Class<?>[] arg_types = new java.lang.Class<?>[args.length];
int n_args = args.length;
int n_args = (args == null) ? 0 : args.length;
java.lang.Class<?>[] arg_types = new java.lang.Class<?>[n_args];
Copy link
Member

@freakboy3742 freakboy3742 Jul 3, 2018

This may be the place where the performance hit is actually taking place (or, at least, somewhere else you can potentially optimise); every 0-arg call is creating an array of size 0.

Copy link
Contributor Author

@patiences patiences Jul 4, 2018

Done!

@@ -381,9 +381,10 @@ public static MatchType parameterMatch(java.lang.Class<?> from_type, java.lang.C
}

public java.lang.Object[] adjustArguments(java.lang.reflect.Method method, org.python.Object[] args, java.util.Map<java.lang.String, org.python.Object> kwargs) {
java.lang.Object[] adjusted = new java.lang.Object[args.length];
int n_args = (args == null) ? 0 : args.length;
java.lang.Object[] adjusted = new java.lang.Object[n_args];
Copy link
Member

@freakboy3742 freakboy3742 Jul 3, 2018

Similarly - this is an empty array being created.

Copy link
Member

@freakboy3742 freakboy3742 Jul 3, 2018

In fact, I don't think there's no need to call this method at all if there aren't any arguments.

Copy link
Contributor Author

@patiences patiences Jul 4, 2018

You're absolutely right! Done

@patiences
Copy link
Contributor Author

@patiences patiences commented Jul 23, 2018

It appears that the reason why I can't write a benchmarking test to show the performance improvement is because the preparation for making a function invocation (the site of this optimization) is miniscule compared to the cost of making a function call. I reached this conclusion because a) the bytecode of the benchmark test I wrote is smaller (see gist here) and b) there is no noticeable performance difference between invoking functions that don't take arguments and functions that do (see gist here).

Copy link
Member

@freakboy3742 freakboy3742 left a comment

👍 Even without any appreciable speedup, it's a nice cleanup of the bytecode, and there's less throwaway empty objects being created, so I'll call that a net win.

@freakboy3742 freakboy3742 merged commit 47335f6 into beeware:master Jul 26, 2018
5 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants