feature gate deprecated APIs for GILPool

[alex]

No description provided.

[alex]

Some pieces of this don't feel super idiomatic -- GILGuard::acquire() returns None if the GIL is already acquired, so I didn't have an easy solution like adding a python() method to it.

[adamreichold]

Some pieces of this don't feel super idiomatic -- GILGuard::acquire() returns None if the GIL is already acquired, so I didn't have an easy solution like adding a python() method to it.

Yeah, that one is one me. My only defence is that this is the result of actually collapsing another layer of indirection, i.e. #3166.

But also have a look at #3685 where I introduced WithGIL as an RAII to use without GILPool, i.e. a type that we would keep in 0.23 when the GILPool is actually fully dropped.

[codspeed-hq]

CodSpeed Performance Report

Merging #4181 will improve performances by 11.02%

_{Comparing alex:remove-gil-pool (16f8a02) with main (1c64a03)}

Summary

⚡ 1 improvements
✅ 67 untouched benchmarks

Benchmarks breakdown

	Benchmark	main	alex:remove-gil-pool	Change
⚡	drop_many_objects	66.9 µs	60.3 µs	+11.02%

[alex]

I think there's a design question here, which is whether we want a new type, or we should just have GILGuard::acquire always return an instance?

[adamreichold]

I think there's a design question here, which is whether we want a new type, or we should just have GILGuard::acquire always return an instance?

The separate type seemed like the easier evolution but that approach has landed us in this mess in the first place.

If GILGuard should always return an instance, then I think it should itself become an Option-like enum with a variant containing PyGILState_STATE and a GILPool (or not depending on the feature, but possibly) and a None-like variant when there is nothing to be done.

[adamreichold]

IIRC, the main point was that GILGuard does not really help for the entry points when we know that GIL is already held and just want to set the correct lifetimes and update our own GIL count tracking. For this, the bare WithGIL type was rather helpful.

[adamreichold]

Ah, exactly, this is calling PyGILState_Ensure for which there is no reason to in the entry points.

[davidhewitt]

I think a WithGIL new type seems preferable to me; making the GILGuard be purely RAII seems like a better separation of concerns even if it works out a bit more verbose.

[davidhewitt]

Actually given both types will also be RAII on the internal GIL_COUNT, on reflection I like the enum-like GILGuard too. I'd suggest Held and Assumed for the two variant names, given we use the term assume elsewhere too.

[adamreichold]

I'd suggest Held and Assumed for the two variant names, given we use the term assume elsewhere too.

I would suggest calling the Held variant Ensured as it is the one which involves a call to PyGILState_Ensure.

One potential downside I would mention (but I have not measured it), is that this will mean that we could end up allocating unused stack space for Python's thread state in every entry point. Probably depends on whether the compiler will SROA-explode the type or not...

[alex]

I'll maybe get back to this today, or maybe not for a few days. If anyone else is champing at the bit to do that refactor, don't feel compelled to wait for me. Or I'll get to it later this week :-)

[davidhewitt]

One potential downside I would mention (but I have not measured it), is that this will mean that we could end up allocating unused stack space for Python's thread state in every entry point. Probably depends on whether the compiler will SROA-explode the type or not...

Ah, true. I think the state returned by PyGILState_Ensure is just an integer, but still a downside.

Actually given both types will also be RAII on the internal GIL_COUNT, on reflection I like the enum-like GILGuard too. I'd suggest Held and Assumed for the two variant names, given we use the term assume elsewhere too.

Thinking further, I realise this is a non-argument as GilGuard can just contain a WithGIL, and WithGIL can then always be the RAII on GIL_COUNT. So I guess the answer is let's go with whatever feels cleaner in code.

[alex]

This is rebased and should be ready for review now.

[davidhewitt]

🎉 wonderfully trivial now, thanks!