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

Callables should handle null args and kwargs #864

Merged
merged 28 commits into from Jul 26, 2018

Conversation

Projects
None yet
2 participants
@patiences
Contributor

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 from Use preallocated vars for functions without args to Callables should handle null args and kwargs Jul 3, 2018

@patiences

This comment has been minimized.

Contributor

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? :-)

@freakboy3742

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];

This comment has been minimized.

@freakboy3742

freakboy3742 Jul 3, 2018

Member

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.

This comment has been minimized.

@patiences

patiences Jul 4, 2018

Contributor

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];

This comment has been minimized.

@freakboy3742

freakboy3742 Jul 3, 2018

Member

Similarly - this is an empty array being created.

This comment has been minimized.

@freakboy3742

freakboy3742 Jul 3, 2018

Member

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

This comment has been minimized.

@patiences

patiences Jul 4, 2018

Contributor

You're absolutely right! Done

@patiences patiences force-pushed the patiences:callable-opt branch from ca6af4b to a8c2113 Jul 4, 2018

patiences added some commits Jul 4, 2018

Revert "Merge commit '244438ff' into callable-opt"
This reverts commit 287f436, reversing
changes made to a8c2113.
@patiences

This comment has been minimized.

Contributor

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).

@freakboy3742

👍 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 pybee:master Jul 26, 2018

5 checks passed

beekeeper:0/beefore:javacheckstyle Java lint checks passed.
Details
beekeeper:0/beefore:pycodestyle Python lint checks passed.
Details
beekeeper:1/smoke-test Smoke build (Python 3.4) passed.
Details
beekeeper:2/full-test:py3.5 Python 3.5 tests passed.
Details
beekeeper:2/full-test:py3.6 Python 3.6 tests passed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment