RFC: Add feature controlling the global reference pool to enable avoiding its overhead.

[adamreichold]

I don't think leaking references in Drop is nice because it is quite easy for this to be called without the GIL being held, especially in embedding scenarios. But it should be safe nevertheless. It might call for an inverted feature to explicitly opt out of the reference pool by enabling e.g. disable-reference-pool.

[codspeed-hq[bot]]

CodSpeed Performance Report

Merging #4095 will improve performances by 10.33%

_{Comparing opt-ref-pool (04f0f05) with main (444be3b)}

Summary

⚡ 1 improvements ✅ 68 untouched benchmarks

Benchmarks breakdown

	Benchmark	main	opt-ref-pool	Change
⚡	sequence_from_tuple	296.7 ns	268.9 ns	+10.33%

[alex]

Why print vs. panic in drop?

[adamreichold]

Why print vs. panic in drop?

It is quite hard to avoid dropping without the GIL, especially in embedding situations. And in contrast to Clone, leaking is sound.

[adamreichold]

That said, I would also be fine with panicking in Drop and put the onus of avoiding it on the extension author as this is already a bit of a "I-don't-care-just-make-it-fast" knob.

[davidhewitt]

I quite like the idea of this feature. I wonder, maybe instead of leaking, without the pool enabled the drop & clone code can just unconditionally call Python::with_gil?

That way the program is correct whether or not the feature is enabled, but users get to control this knob according to the performance characteristics of their program.

[davidhewitt]

(and hopefully with nogil in the long term the reference pool feature might just then become deprecated for better options)

[adamreichold]

without the pool enabled the drop & clone code can just unconditionally call Python::with_gil?

Isn't this much too prone to deadlocks?

[davidhewitt]

Ungh, yes that's true. With nogil what I suggest would be viable. 😭

[Icxolu]

Could we provide the Clone impl for Py only under that feature flag? That would eliminate the panic and make it a compile error. Cloning would still be possible by going through Bound, with the guarantee that the gil is held.

[adamreichold]

Could we provide the Clone impl for Py only under that feature flag? That would eliminate the panic and make it a compile error. Cloning would still be possible by going through Bound, with the guarantee that the gil is held.

The problem I see is that you might want to still clone Py values to e.g. fulfil trait requirements of Rust types but ensure by context that you have the GIL.

[adamreichold]

Added a section in the performance chapter of the guide. @inducer @matthiasdiener As this was originally reported by you, what do you think about the feature and its explanation in the guide?

[Icxolu]

The problem I see is that you might want to still clone Py values to e.g. fulfil trait requirements of Rust types but ensure by context that you have the GIL.

Yes, I think it's extremely handy to be able to #[derive(Clone)] in particular, which would require Py<T>: Clone.

Hmm, I can definitely see that point, but in that case I feel like having this as opt-out of "safe" behavior, rather than opt-in to "i-know-what-im-doing" is not ideal, since this has "spooky" side effects. Starting with no-default-features, or turning them off later and forgetting to turn this one back on, will lead to very different behavior, which won't be noticed until runtime. Arguably this is probably not a very common scenario currently, but it feels like an easy footgun nevertheless.

[adamreichold]

Hmm, I can definitely see that point, but in that case I feel like having this as opt-out of "safe" behavior, rather than opt-in to "i-know-what-im-doing" is not ideal, since this has "spooky" side effects. Starting with no-default-features, or turning them off later and forgetting to turn this one back on, will lead to very different behavior, which won't be noticed until runtime. Arguably this is probably not a very common scenario currently, but it feels like an easy footgun nevertheless.

I share that sentiment which is why I included

It might call for an inverted feature to explicitly opt out of the reference pool by enabling e.g. disable-reference-pool.

in the cover letter.

Do note one downside of this though: Rust's features are additive so there is a different kind of spooky side effect. One crate in dependency graph of an extension can enable affecting everything else. It would be bad style to enable it in a non-leaf crate, but it is possible.

So if we really want to make this hard to enable accidentally as possible, we should probably go for a raw --cfg flag enabled by wrangling compiler flags to which I would also be amenable.

But I think we should first hear from the original reporters whether they actually want this. Just dropping the PR because it does not really help them is also an option.

[davidhewitt]

Following up from https://github.com/PyO3/pyo3/issues/4105#issuecomment-2068092437

It seems that we may need to migrate to a strategy where Clone without the GIL will always panic. We could gate the whole Clone implementation behind a feature, given that the panic is inconvenient and users may not want to deal with the possibility of runtime crashes. (Instead they would likely need to write Clone implementations by hand.)

For Drop - I think that deferring reference count decreases is still safe (worst it can do is cause leaks).

Maybe we can move the overhead away from all PyO3 functions, and instead if there is a deferred drop we signal up a worker thread to wake, which can attempt to acquire the GIL and drop all currently pending deferred drops? Moving the overheads off every function call may be a good enough solution until GIL-less Python can give us an alternative option.

[davidhewitt]

(I know I already suggested this reference-counting-thread idea in https://github.com/PyO3/pyo3/issues/3827#issuecomment-1942438315 and @adamreichold correctly pointed out that deferring the work is dangerous.

With what we are now seeing particularly about deferred Clone being incorrect, my thought is the reference-counting-thread idea may be viable when only used for Drop.)

[adamreichold]

Maybe we can move the overhead away from all PyO3 functions, and instead if there is a deferred drop we signal up a worker thread to wake, which can attempt to acquire the GIL and drop all currently pending deferred drops? Moving the overheads off every function call may be a good enough solution until GIL-less Python can give us an alternative option.

But isn't this mainly gaming the benchmarks? Our call overhead will reduce, but the actual work to be done will increase (at least there is an additional acquisition of the GIL, and on another thread to which all these objects may be cache-cold).

This could improve throughput at the cost of increase CPU usage, but only if the other threads actually release the GIL for reasonable amounts of time. And if that worker thread never actually gets the GIL, we could see unbounded memory growth.

While deferred drop is certainly not as dangerous a deferred clone, I fear we are inventing a garbage collector here. And if do decide to do that, I think we should go for established solutions, e.g. epoch-based reclamation or hazard pointers which at least have some support crates available.

Personally, I don't think we should make this work. This is adding completely additional semantics that Python itself does not (yet) support which will always be difficult to get right as long as Python does not cooperate in these memory management tasks.

So while I could live without the Clone impl, I would like to avoid making the Drop do more than leak or panic/abort. If we are unused about this, then I think adding a feature flag to control whether Clone is unavailable and Drop leaks versus Clone and Drop both panic/abort makes more sense to avoid any surprises. Adding a panicking Clone also sounds like it fits the additive feature model.

[davidhewitt]

But isn't this mainly gaming the benchmarks? Our call overhead will reduce, but the actual work to be done will increase (at least there is an additional acquisition of the GIL, and on another thread to which all these objects may be cache-cold).

This could improve throughput at the cost of increase CPU usage, but only if the other threads actually release the GIL for reasonable amounts of time. And if that worker thread never actually gets the GIL, we could see unbounded memory growth.

Very true. Ok, I hereby agree to drop (heh) the reference-counting-thread idea 👍

I think there's still some open question on whether we should be trying to keep the drop pool around. I feel like for sake of compatibility we can't migrate users immediately from the existing pool to leaking on drop without GIL, as silently adding memory leaks to their program may break confidence in PyO3. The abort-on-drop is similarly unappealing for migration but is at least a noisy failure mode.

Whether to leak / abort, some prior art is that pybind11 throws exceptions if objects are dropped without the GIL held - https://github.com/pybind/pybind11/blob/19a6b9f4efb569129c878b7f8db09132248fbaa1/include/pybind11/pytypes.h#L269

Given the concern about testing for leaks being hard, I'd be tempted to suggest that we add this as a disable-reference-pool feature which replaces the reference pool with aborts.

I think we are now on a course to release 0.22 ASAP, given that there are various behavioural changes afoot here.

So I propose the following for 0.22:

• We get the next step of the gil-refs deprecation complete.
• We gate a panicking Py::clone implementation behind a py-clone feature.
• We add disable-reference-pool to abort on drop instead of pooling.
• (Maybe) we also complete the changes to Python::allow_threads

[adamreichold]

We gate a panicking Py::clone implementation behind a py-clone feature.

I added a new commit here which removes delayed refcnt increments and adds said feature.

We add disable-reference-pool to abort on drop instead of pooling.

Added another commit which renames the feature proposed here.

(Maybe) we also complete the changes to Python::allow_threads

I would be glad if we did.

Note that the tests currently pass only with py-clone enabled and disable-reference-pool disabled. I will have to work on this, but I thought I push what I have so far to enable review of the more tricky parts of the code and especially the associated documentation.

[davidhewitt]

👍 I will do my best to find time to give this a full review tomorrow. Am pretty loaded with family responsibility at the moment so my opportunities to focus are a bit rare this month.

[mejrs]

Do we have metrics showing the performance differences between this and the current implementation? I'd be uncomfortable with this if there was not a noticeable performance benefit.

So if we really want to make this hard to enable accidentally as possible, we should probably go for a raw --cfg flag enabled by wrangling compiler flags to which I would also be amenable.

I would very much prefer this over feature flags.

[adamreichold]

Do we have metrics showing the performance differences between this and the current implementation? I'd be uncomfortable with this if there was not a noticeable performance benefit.

Removing deferred clones/reference count increments is necessary for correctness and py-clone is additive hiding the now necessarily panicking Clone impl to avoid surprises when people update, i.e. they have to enable the feature if they want to continue to use Clone and at least get a chance to read about the implications.

The other feature to disable the reference pool is motivated by the CPU profiles from #4085 where it is the single largest contributor which we could do away with. The use case is a bit extreme, but especially for the extension use case the reference pool is mainly an additional convenience users can do without. But of course, it makes usage more difficult so it is opt-in.

[adamreichold]

I have pushed a new version which should address the review comments so far.

I also added a separate commit which turns the disable-reference-pool feature into the disable_pyo3_reference_pool conditional compilation flag. I did opt to keep it documented in features.md though.

So assuming no further issues are found, before this can be merged we need to reach consensus on two open questions as far as I can see:

• [ ] Do we want to use a feature or a conditional compilation flag. We did decide on inverted semantics in both cases, i.e. the default is safe and the dangerous but fast path must be explicitly enabled. The feature has significantly better usability, but can be abused in non-leaf crates. The conditional compilation flag is intentionally harder to use (which is the intention for using it at all) and set as a global property of the build by design.
• [ ] Do we want another flag/feature/environment variable to turn aborting the process on dropping without the GIL into leaks? Aborts are better for testing but leaking is always safe even though performance can suffer. So maybe we want to couple this to cfg(debug_assertions) and abort in debug builds but leak in release builds?

[adamreichold]

Any thoughts on two remaining design questions? Otherwise, this should be good to go after conflict resolution.

[davidhewitt]

Sorry for the delay, I got delayed by a pile of family responsibility in April which left me quite burned out.

I think a conditional compile flag is good and adding a second one for the downgrade of aborts to leaks seems ok to me too.

I somewhat wonder if we should split the py-clone feature into a separate PR, as I think that changes user experience noticeably and I worry if it is slightly lost in the discussion here. Having a new PR just with that diff and a clearer opening post with references to these threads may help users with archaeology later.

[adamreichold]

I think a conditional compile flag is good and adding a second one for the downgrade of aborts to leaks seems ok to me too.

So explicitly, this means we do not want to tie this to cfg(debug_assertions) and I will rather make this leak by default and add another flag disable_pyo3_reference_pool_and_abort_on_drop which will upgrade the leaks to aborts?

I somewhat wonder if we should split the py-clone feature into a separate PR, as I think that changes user experience noticeably and I worry if it is slightly lost in the discussion here. Having a new PR just with that diff and a clearer opening post with references to these threads may help users with archaeology later.

There are somewhat functionally intertwined, so I would glad if I could instead mitigate this by updating the cover letter here to retrace the steps and decisions.

[davidhewitt]

I think a conditional compile flag is good and adding a second one for the downgrade of aborts to leaks seems ok to me too.

So explicitly, this means we do not want to tie this to cfg(debug_assertions) and I will rather make this leak by default and add another flag disable_pyo3_reference_pool_and_abort_on_drop which will upgrade the leaks to aborts?

Hmmm. I think in my mind leaking is the worst case (as it implies GC / memory overhead for the rest of program runtime, which may be hard for users to notice). So I'd prefer abort by default and the second flag changes from aborting to leaking.

If we do move ahead with #4174 then having just one flag to get to aborting seems slightly better too.

I somewhat wonder if we should split the py-clone feature into a separate PR, as I think that changes user experience noticeably and I worry if it is slightly lost in the discussion here. Having a new PR just with that diff and a clearer opening post with references to these threads may help users with archaeology later.

There are somewhat functionally intertwined, so I would glad if I could instead mitigate this by updating the cover letter here to retrace the steps and decisions.

👍 Yep that's reasonable to me!

[adamreichold]

Hmmm. I think in my mind leaking is the worst case (as it implies GC / memory overhead for the rest of program runtime, which may be hard for users to notice). So I'd prefer abort by default and the second flag changes from aborting to leaking.

There are now two cfg flags: pyo3_disable_reference_pool and pyo3_leak_on_drop_without_reference_pool.

👍 Yep that's reasonable to me!

I have updated the cover letter.

[alex]

nox > Session check-feature-powerset aborted: some features missing from full meta feature: {'py-clone'}.