Faster pycall. Adds pycall! #492

JobJob · 2018-04-13T15:56:08Z

Speeds up pycalls. For functions with 1-7 arguments it reduces the overhead of calling the function from Julia by about 10-40x

Adds pycall!(ret, o, ...) to avoid allocating a PyObject for the return value each time the function is called.

One query: I'm not 100% about the const pyarg_tuples = PyPtr[], and growing it when functions have more arguments. Maybe we should just initialise it to a certain size (32 arguments?), check for isassigned(pyarg_tuples, nargs+1), and if not assigned, create the tuple, and grow the pyarg_tuples vector if someone requires more than the 32 arg default.

For reasons that I'll need to track down over the weekend, one of the tuples that we use for passing args, is ending up with a refcount of 2, which is not allowed when setting an item, since tuples are supposed to be immutable (but when there's only one ref, setting an item is allowed):
https://github.com/python/cpython/blob/23ab5ee667a9b29014f6f7f01797c611f63ff743/Objects/tupleobject.c#L166
so this is WIP until I solve that.

~~Happens after the @pydef type Doubler, see the @test_broken in runtests. Any ideas appreciated.~~

Creating new builtin exceptions increment the refcount of the arguments tuple passed to it. The Doubler class in that test was subclassing AssertionError and instantiating triggered this. Handled this situation (callees incrementing the refcount of args tuple) now.

to-do:

fix a bug in the wrap ~~and possibly some others about.~~ (there was another minor bug in the test for having the only reference to the (empty) arguments tuple)
maybe remove the wrap, since maybe it's not faster after all... it's not, always. Let's remove it for now.
remove pycall_legacy when everyone's happy with the PR (was just left in for benchmarking comparison purposes)

Benchmark results (Mean times for different arities)

pycall_legacy ()                 	 TrialEstimate(946.804 ns) (on master when this PR was opened)
pycall ()                        	 TrialEstimate(725.172 ns) (this PR)
pycall! ()                       	 TrialEstimate(566.083 ns) (this PR, pycall!) 

pycall_legacy (1,)               	 TrialEstimate(8.582 μs)
pycall (1,)                      	 TrialEstimate(1.454 μs)
pycall! (1,)                     	 TrialEstimate(1.262 μs)

pycall_legacy (1, 1)             	 TrialEstimate(12.365 μs)
pycall (1, 1)                    	 TrialEstimate(1.657 μs)
pycall! (1, 1)                   	 TrialEstimate(1.684 μs)

pycall_legacy (1, 1, 1)          	 TrialEstimate(16.136 μs)
pycall (1, 1, 1)                 	 TrialEstimate(1.694 μs)
pycall! (1, 1, 1)                	 TrialEstimate(1.764 μs)

pycall_legacy 7*(1,1,...)        	 TrialEstimate(29.335 μs)
pycall 7*(1,1,...)               	 TrialEstimate(2.881 μs)
pycall! 7*(1,1,...)              	 TrialEstimate(2.794 μs)

JobJob · 2018-04-13T16:04:52Z

src/PyCall.jl


 """
 Low-level version of `pycall(o, ...)` that always returns `PyObject`.
 """
-function _pycall(o::Union{PyObject,PyPtr}, args...; kwargs...)
+function _pycall_legacy(o::Union{PyObject,PyPtr}, args...; kwargs...)


This is just here for the benchmark comparison, the whole function, and the pycall_legacy methods below, need to be removed before merging.

JobJob · 2018-05-25T16:56:55Z

Anything else you need doing for this (and other open PRs)? Test failures on Windows here to do with importing NumPy, not sure what to do for those? 0.7 failures seem unrelated.

worked out why it was benchmarking slightly slower than the closure version. I think the struct version is clearer.

JobJob · 2018-06-28T00:05:20Z

@stevengj any news here?

Should I bother trying to rebase #487 (again) yet? Or wait a bit for more 0.7 related changes? Seems this and the others don't have merge conflicts (so I rebased them).

Apologies if the file rearranging made this or the others harder to review. I can revert if needed I suppose.

stevengj · 2018-06-28T21:56:31Z

Looks like this is breaking nightly?

JobJob · 2018-07-02T11:17:42Z

Hmm, I can't replicate the nightly errors locally, both w julia on the commit that travis used 8e7e6fc00a, and master from yesterday, tests pass with juliadev -e 'Pkg.test("PyCall"; coverage=true)'

edit: with python3 on mac
edit 2: Duh. I was doing it wrong with Pkg3 - replicated locally now

Attempt to fix 0.7 issue

JobJob · 2018-07-02T13:21:03Z

Ok, pushed a fix. There's a seemingly unrelated error in the Conda Mac build on 0.6, but the rest of the travis builds are good.

Most of the Windows builds are complaining about numpy not being installed - anything I can do about those?

stevengj · 2018-07-02T16:18:37Z

Don't rely on numpy features in the tests, or only test the numpy aspects if PyCall.npy_initialized is true.

JobJob · 2018-07-03T12:05:54Z

I've changed the tests to not use NumPy anymore.

However, for #487's most of the tests need numpy, so I'll just not run those if pyimport("numpy") fails. Btw PyCall.npy_initialized is always false while running the tests on my machine - so I'll just use try pyimport("numpy") catch.

stevengj · 2018-07-03T16:43:49Z

Btw PyCall.npy_initialized is always false while running the tests

It's false in the beginning because numpy is loaded lazily, but it should be true after other array tests execute.

However, you can also do ispynull(pyimport_e("numpy")) to avoid the try/catch

codecov-io · 2018-07-03T19:26:06Z

Codecov Report

Merging #492 into master will decrease coverage by 0.32%.
The diff coverage is 91.48%.

@@            Coverage Diff             @@
##           master     #492      +/-   ##
==========================================
- Coverage    65.5%   65.17%   -0.33%     
==========================================
  Files          18       19       +1     
  Lines        1487     1522      +35     
==========================================
+ Hits          974      992      +18     
- Misses        513      530      +17

Impacted Files	Coverage Δ
src/PyCall.jl	`67.04% <40%> (-6.78%)`	⬇️
src/pyfncall.jl	`97.61% <97.61%> (ø)`
src/pyeval.jl	`69.79% <0%> (+0.31%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e89bc4...8ed2888. Read the comment docs.

stevengj · 2018-07-03T19:39:44Z

src/pyfncall.jl

+(`pyargsptr`). Sets `ret.o` to the result of the call, and returns `ret::PyObject`.
+"""
+function _pycall!(ret::PyObject, o::Union{PyObject,PyPtr}, args,
+  nargs::Int=length(args), kw::Union{Ptr{Nothing}, PyObject}=C_NULL)


Shouldn't this be a PyPtr rather than Ptr{Nothing}?

No that's for the C_NULL when there aren't any kwargs. But good - that nudged me to add some tests for the kwargs case.

Ptr{Nothing} is an implementation detail; it's best to use Ptr{Cvoid} (Base in 0.7, available from Compat in 0.6)

JobJob · 2018-07-04T00:04:48Z

"Everyone" happy with this? Should I remove the pycall_legacy stuff?

JobJob · 2018-07-04T00:10:08Z

Oh and btw re

It's false in the beginning because numpy is loaded lazily, but it should be true after other array tests execute.

I thought I was checking late in the tests, but perhaps not, it's fine now.

stevengj · 2018-07-10T16:06:42Z

Yes, go ahead and remove pycall_legacy

JobJob mentioned this pull request Apr 13, 2018

Add PyFuncWrap #483

Closed

JobJob commented Apr 13, 2018

View reviewed changes

JobJob changed the title ~~WIP: Faster pycall. Adds pycall!, pywrapfn~~ Faster pycall. Adds pycall!, pywrapfn Apr 15, 2018

JobJob changed the title ~~Faster pycall. Adds pycall!, pywrapfn~~ WIP: Faster pycall. Adds pycall!, pywrapfn Apr 16, 2018

JobJob changed the title ~~WIP: Faster pycall. Adds pycall!, pywrapfn~~ Faster pycall. Adds pycall! Apr 18, 2018

JobJob force-pushed the pycall-bang branch 2 times, most recently from 6122bcd to b6db3b1 Compare April 30, 2018 07:21

JobJob added 16 commits June 28, 2018 02:09

Add PyFuncWrap

dbc8105

Add pydecref to old ret value, and fix bug in benchmark

fc9fe81

pycall!

98db8eb

Faster generic pycall

e0ca826

PyFuncWrap -> pywrapfn

573a43e

rename to pycalls.jl

cc3a3af

More tests, better docstrings

f297a61

Fix some tests and debugging stuff...

be758c3

no longer needed tuplen

23267f5

Revert pywrapfn back to a struct

3edb8f0

worked out why it was benchmarking slightly slower than the closure version. I think the struct version is clearer.

rename pycalls.jl to pyfncall.jl

3fc1093

Ensure only single ref to pyargsptr tuple

b1a9899

Fix bug in PyWrapFn and change to vararg

db6d000

Fix pyargsptr check for empty tuple

88baa71

Remove PyWrapFn

82335aa

Add pycall! tests. Remove pywrapfn tests

8a52d6f

JobJob force-pushed the pycall-bang branch from b6db3b1 to 8a52d6f Compare June 27, 2018 23:10

Remove kwargs type in _pycall!

b8a087f

Attempt to fix 0.7 issue

Stop using NumPy in tests

c2c7c5a

0.7 Deprecations

c67166c

stevengj reviewed Jul 3, 2018

View reviewed changes

Add tests for kwargs

ee4d6f7

Ptr{Nothing} to Ptr{Cvoid}

8ed2888

Move pycall_legacy and pywrapfn to benchmarks

b7c07e1

stevengj merged commit e42d165 into JuliaPy:master Jul 11, 2018

findmyway mentioned this pull request Jul 16, 2018

update PyCall to support pycall! findmyway/Environment.jl#1

Closed

stevengj mentioned this pull request Aug 21, 2018

re-entrancy issue in new pycall! #533

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Faster pycall. Adds pycall! #492

Faster pycall. Adds pycall! #492

JobJob commented Apr 13, 2018 •

edited

Loading

JobJob Apr 13, 2018 •

edited

Loading

JobJob commented May 25, 2018

JobJob commented Jun 28, 2018

stevengj commented Jun 28, 2018

JobJob commented Jul 2, 2018 •

edited

Loading

JobJob commented Jul 2, 2018 •

edited

Loading

stevengj commented Jul 2, 2018

JobJob commented Jul 3, 2018 •

edited

Loading

stevengj commented Jul 3, 2018 •

edited

Loading

codecov-io commented Jul 3, 2018 •

edited

Loading

stevengj Jul 3, 2018

JobJob Jul 3, 2018 •

edited

Loading

iamed2 Jul 4, 2018

JobJob Jul 4, 2018

JobJob commented Jul 4, 2018

JobJob commented Jul 4, 2018

stevengj commented Jul 10, 2018

Faster pycall. Adds pycall! #492

Faster pycall. Adds pycall! #492

Conversation

JobJob commented Apr 13, 2018 • edited Loading

JobJob Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

JobJob commented May 25, 2018

JobJob commented Jun 28, 2018

stevengj commented Jun 28, 2018

JobJob commented Jul 2, 2018 • edited Loading

JobJob commented Jul 2, 2018 • edited Loading

stevengj commented Jul 2, 2018

JobJob commented Jul 3, 2018 • edited Loading

stevengj commented Jul 3, 2018 • edited Loading

codecov-io commented Jul 3, 2018 • edited Loading

Codecov Report

stevengj Jul 3, 2018

Choose a reason for hiding this comment

JobJob Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

iamed2 Jul 4, 2018

Choose a reason for hiding this comment

JobJob Jul 4, 2018

Choose a reason for hiding this comment

JobJob commented Jul 4, 2018

JobJob commented Jul 4, 2018

stevengj commented Jul 10, 2018

JobJob commented Apr 13, 2018 •

edited

Loading

JobJob Apr 13, 2018 •

edited

Loading

JobJob commented Jul 2, 2018 •

edited

Loading

JobJob commented Jul 2, 2018 •

edited

Loading

JobJob commented Jul 3, 2018 •

edited

Loading

stevengj commented Jul 3, 2018 •

edited

Loading

codecov-io commented Jul 3, 2018 •

edited

Loading

JobJob Jul 3, 2018 •

edited

Loading