CLN/WIP: use conditional nogil

[jbrockmendel]

This is largely an experiment to see how much code we could de-duplicate once cython 0.30.0 comes out and conditional with nogil becomes available. Its a decent amount, less than I'd expected.

Hopefully this can help cython kick the tires.

[jbrockmendel]

@scoder the "kicking the tires" i mentioned in the OP largely is about this spot. When the commented-out code is enabled here, I get a compile-time error:

                    if rank_t is object:
                        raise ValueError('first not supported for '
                       ^
------------------------------------------------------------

pandas/_libs/algos.pyx:895:24: Trying to acquire the GIL while it is already held.

Is the issue that the compiler doesn't know its in a not-nogil case if it gets here?

[scoder]

I think the GIL acquisition around the raise might happen too early here, before the fused type specialisation, so that it always tries to acquire the GIL even if the with nogil was removed for the specific type.

I think Cython should mark the automatically injected GilStatNode as something internal in InjectGilHandling (module ParseTreeTransforms.py), and then discard the redundant ones in GilCheck instead of throwing this error for them. That would avoid the need for any special casing of fused functions later on. PR welcome.

[jbrockmendel]

@WillAyd anywhere else I missed where we can rip out code using conditional nogil (zero hurry). I'd be interested in your thoughts on the apparently-untested TIEBREAK_FIRST/object code path.

@pilkibun IIRC #27373 was a proof of concept for things we could clean up once cython 0.30.0 is out. Any other suggestions?

[jbrockmendel]

Still interested in thoughts from @WillAyd and @pilkibun (see previous comment), but this PR was just a proof of concept, so closing.

[WillAyd]

Yep sorry; great idea but admittedly didn't put a ton into review - waiting for Cython to actually release first

[jbrockmendel]

@WillAyd the part that I thought you'd be interested in is that we don't have any tests that go through the TIEBREAK_FIRST/object path. I'm hoping we can either a) un-disallow that and not raise mid-loop or b) raise up-front pre-loop. Pinging you on it is based on a vague recollection that you had worked on it before.

[WillAyd]

Hmm maybe I just forgot but don't recall touching the algos code (did something similar in groupby) so maybe don't fully grasp the context but from glancing at it your suggestion to raise outside of the loop seems logical