ARROW-2052: [C++ / Python] Rework OwnedRef, remove ScopedRef

[pitrou]

OwnedRef API cleaned up:

• doesn't try to take the GIL in its destructor anymore
• reset() decrefs the underlying pointer
• detach() unowns the pointer without decrefing it

Add OwnedRefNoGIL which forcefully takes the GIL in its destructor, at the expense of runtime performance.

[pitrou]

This PR has an interesting performance impact here:

$ python -m timeit -s "import pyarrow as pa; ty=pa.int64(); data=[None]*1000000" "pa.array(data, type=ty)"

• before: 27.1 msec per loop
• after: 14.9 msec per loop

Probably because of less calls to PyGILState_Ensure.

[wesm]

On the subject of performance, our Airspeed / ASV setup is in a state of disrepair. We should start building a suite of microbenchmarks so we can monitor the performance of operations like these in a more systematic way. There are already some JIRAs open about getting this cleaned up and well-documented for other developers, would you be interested in taking a look?

[pitrou]

There are already some JIRAs open about getting this cleaned up and well-documented for other developers, would you be interested in taking a look?

Definitely, can you give me some pointers?

[wesm]

See https://issues.apache.org/jira/browse/ARROW-1861

There is already an asv.conf.json in https://github.com/apache/arrow/tree/master/python and a couple of benchmarks, but I tried to run it myself some time ago and ran into issues. It's not documented at all AFAICT. pandas has a pretty deep suite of benchmarks https://github.com/pandas-dev/pandas/tree/master/asv_bench

[cpcloud]

@pitrou LGTM

Is it correct to say that now callers must be more careful about acquiring the GIL before making calls into the Python C API (assuming use of OwnedRef)?

[wesm]

I need to take a closer look at our various usages of OwnedRef -- there are places where we have OwnedRef as member variables of classes, so the risk is that an OwnedRef might be destructed at the same time that a PyAcquireGIL lock is being destructed, so we could be relying on the order of destruction to make sure the GIL is held. We'll need to prevent this by making sure that no OwnedRef can be destructed at the same time as a GIL is released by RAII

[pitrou]

I need to take a closer look at our various usages of OwnedRef -- there are places where we have OwnedRef as member variables of classes, so the risk is that an OwnedRef might be destructed at the same time that a PyAcquireGIL lock is being destructed

The PR already takes care of that, by using OwnedRefNoGIL in those places (perhaps I've missed some?).

[pitrou]

Is it correct to say that now callers must be more careful about acquiring the GIL before making calls into the Python C API (assuming use of OwnedRef)?

The GIL rules for OwnedRef are now the same as they were for ScopedRef. If you're not sure the GIL will remain held at the right times, you can use OwnedRefNoGIL instead.

[wesm]

+1, thanks for doing this, cleaner now!