Skip to content

Conversation

@davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Jan 29, 2024

Split from #3606

This makes Bound::from_owned_ptr and variants part of the public API, instead of pub(crate).

This is useful for the occasional use case outside of PyO3 which deals with raw FFI. These APIs also have equivalents with the same names which already exist on Py<T>, so it seems to me that we might as well have these.

@davidhewitt davidhewitt added the CI-skip-changelog Skip checking changelog entry label Jan 29, 2024
@codspeed-hq
Copy link

codspeed-hq bot commented Jan 29, 2024

CodSpeed Performance Report

Merging #3779 will improve performances by 20.41%

Comparing davidhewitt:bound-from-ptr (86f294f) with main (ecb4ecb)

🎉 Hooray! pytest-codspeed just leveled up to 2.2.0!

A heads-up, this is a breaking change and it might affect your current performance baseline a bit. But here's the exciting part - it's packed with new, cool features and promises improved result stability 🥳!
Curious about what's new? Visit our releases page to delve into all the awesome details about this new version.

Summary

⚡ 4 improvements
✅ 75 untouched benchmarks

Benchmarks breakdown

Benchmark main davidhewitt:bound-from-ptr Change
list_via_downcast 185 ns 157.2 ns +17.67%
sequence_from_list 355.6 ns 300 ns +18.52%
mapping_from_dict 327.8 ns 272.2 ns +20.41%
not_a_list_via_downcast 242.8 ns 215 ns +12.92%

@davidhewitt
Copy link
Member Author

I suppose the one design question here is whether we think the simpler Bound::from_ptr, Bound::from_ptr_or_opt etc would be better(i.e. dropping the _owned bit).

Nicer names maybe, but they then lose the consistency with Py.

@Icxolu
Copy link
Contributor

Icxolu commented Feb 5, 2024

I think it makes sense to have these as a complement to the ones on Py. I would suggest to keep the names the same. It's not that much longer to keep _owned and I think its easier to reason about for users if the names are consistent.

I would also suggest to deprecate the corresponding functions on Python, which look like the predecessors of these.

@davidhewitt
Copy link
Member Author

Thanks, that's enough support for me to merge forward with these as-is for now 👍

I would also suggest to deprecate the corresponding functions on Python, which look like the predecessors of these.

Agreed, let me see if I can throw together something for those quickly now...

@davidhewitt
Copy link
Member Author

Agreed, let me see if I can throw together something for those quickly now...

Actually there's still quite a lot of work to migrate off these internally, for now I've just added bullets to #3684 under a deprecations section.

@davidhewitt davidhewitt added this pull request to the merge queue Feb 5, 2024
Merged via the queue into PyO3:main with commit 020ed39 Feb 5, 2024
@davidhewitt davidhewitt deleted the bound-from-ptr branch February 5, 2024 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI-skip-changelog Skip checking changelog entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants