Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add extract_bound method to FromPyObject #3706

Merged
merged 2 commits into from
Jan 28, 2024

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Dec 27, 2023

This PR proposes a way that we can support migration of FromPyObject off the GIL Refs API in a backwards-compatible fashion.

It does so by adding a new extract_bound method to FromPyObject<'py>, which takes &Bound<'py, PyAny>. Note that the lifetime 'py in the trait becomes only the 'py lifetime and not the lifetime for which the Bound smart pointer is alive. This is important for backwards-compatibility; maybe in the future we could have FromPyObject<'a, 'py> and the argument could be &'a Bound<'py, PyAny>, but let's cross that bridge another time.

Also for backwards-compatibility's sake, both extract and extract_bound have default implementations in terms of each other. This allows for users to migrate from using one set of implementations to the other at their own pace. The documentation encourages to implement just extract_bound, as the default implementation requires a .into_gil_ref() call to insert into the pool.

To avoid a huge diff here, I've pushed a second commit which just includes a couple of randomly-selected implementations which I migrate from extract to extract_bound, to give a feeling for how this works.

If we like this and merge it, I can work on one or more follow-ups which migrate the rest of our internals to extract_bound (which will go a long way towards getting PyO3's internals off the pool).

@davidhewitt davidhewitt added the CI-skip-changelog Skip checking changelog entry label Dec 27, 2023
@davidhewitt davidhewitt removed the CI-skip-changelog Skip checking changelog entry label Dec 27, 2023
Copy link

codspeed-hq bot commented Dec 27, 2023

CodSpeed Performance Report

Merging #3706 will degrade performances by 32.02%

Comparing davidhewitt:frompyobject2 (ffaa03e) with main (eb8d11f)

Summary

⚡ 4 improvements
❌ 9 regressions
✅ 67 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main davidhewitt:frompyobject2 Change
tuple_get_item_unchecked 12.6 ms 16.9 ms -25.21%
sequence_from_list 425 ns 341.7 ns +24.39%
iter_list 23.1 ms 31.6 ms -26.94%
sequence_from_tuple 425 ns 369.4 ns +15.04%
list_get_item_unchecked 12.8 ms 17.1 ms -24.96%
tuple_get_item 15.8 ms 20.1 ms -21.23%
list_via_extract 340 ns 308.9 ns +10.07%
list_get_item 15.8 ms 20.1 ms -21.23%
tuple_get_borrowed_item 14.5 ms 18.9 ms -23.42%
iter_tuple 18.4 ms 27.1 ms -32.02%
tuple_get_borrowed_item_unchecked 11.3 ms 15.7 ms -28.13%
not_a_list_via_downcast 211.7 ns 183.9 ns +15.11%
iter_set 40.7 ms 54.1 ms -24.69%

Comment on lines +223 to +237
/// Extracts `Self` from the source GIL Ref `obj`.
///
/// Implementors are encouraged to implement `extract_bound` and leave this method as the
/// default implementation, which will forward calls to `extract_bound`.
fn extract(ob: &'source PyAny) -> PyResult<Self> {
Self::extract_bound(&ob.as_borrowed())
}

/// Extracts `Self` from the bound smart pointer `obj`.
///
/// Implementors are encouraged to implement this method and leave `extract` defaulted, as
/// this will be most compatible with PyO3's future API.
fn extract_bound(ob: &Bound<'source, PyAny>) -> PyResult<Self> {
Self::extract(ob.clone().into_gil_ref())
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default implementation has infinite recursion if a user implements neither method. This is a slightly clunky footgun but I'm fine with it as a temporary measure; I think in 0.22 once we start pushing harder then we can just default extract and require an implementation of extract_bound.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does implementing this using only the defaults trigger a warning due to unconditional mutual recursion?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly no, it just leads to a stack overflow at runtime. I've tried to repeat the expectation in documentation that implementors should implement extract_bound.

Hopefully it's a fairly obvious programming error, and at least we can get rid of the .extract_bound() default in 0.22 and remove this footgun again.

@davidhewitt
Copy link
Member Author

Unless we can think of alternatives here, I'd quite like to merge this PR, as merging this unblocks us to migrate the rest of PyO3's implementations of FromPyObject off the GIL Refs API. I think that also is a significant chunk of where PyO3 will be using the old API. (And a lot of that I've got stacked in #3606, so I can split that out into PRs once this lands.)

Copy link
Member

@adamreichold adamreichold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the general approach is the best we can do for 0.21. I think this should be highlighted in the migration guide though, either as part of this PR or as a task item on the tracking issue.

@davidhewitt
Copy link
Member Author

Thank you for all the reviews today! I'll have plenty of PRs to push once this one goes in ;)

Completely agree that this is worthy of going in the migration guide. I'll add it to this PR; I'll rebase once #3755 goes in and add some detail before merging this.

@davidhewitt davidhewitt added this pull request to the merge queue Jan 28, 2024
Merged via the queue into PyO3:main with commit c54d897 Jan 28, 2024
36 of 38 checks passed
@davidhewitt davidhewitt deleted the frompyobject2 branch January 28, 2024 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants