Skip to content

port Python::get_type to Bound API#3846

Merged
davidhewitt merged 2 commits intoPyO3:mainfrom
Icxolu:get-type
Feb 18, 2024
Merged

port Python::get_type to Bound API#3846
davidhewitt merged 2 commits intoPyO3:mainfrom
Icxolu:get-type

Conversation

@Icxolu
Copy link
Member

@Icxolu Icxolu commented Feb 16, 2024

Part of #3684

This ports Python::get_type to the Bound API. I implemented this on a rebase of #3705 (with minor changes to adapt for changes since creation), so this should compile already. All the changes related to the get_type conversion are in a separate commit. I'll mark this as draft and rebase this when #3705 lands.

@codspeed-hq
Copy link

codspeed-hq bot commented Feb 16, 2024

CodSpeed Performance Report

Merging #3846 will degrade performances by 10.2%

Comparing Icxolu:get-type (4b88d8e) with main (f04ad56)

Summary

⚡ 4 improvements
❌ 1 regressions
✅ 74 untouched benchmarks

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

Benchmarks breakdown

Benchmark main Icxolu:get-type Change
extract_float_extract_success 500.6 ns 445 ns +12.48%
extract_str_downcast_success 728.9 ns 645.6 ns +12.91%
list_via_extract 364.4 ns 308.9 ns +17.99%
not_a_list_via_downcast 244.4 ns 272.2 ns -10.2%
sequence_from_tuple 267.2 ns 239.4 ns +11.6%

@davidhewitt davidhewitt added the CI-skip-changelog Skip checking changelog entry label Feb 17, 2024
@davidhewitt
Copy link
Member

Great, many thanks! I think #3705 is finally in a good state to review again, after a long deviation to get #3831 done first...

@Icxolu Icxolu marked this pull request as ready for review February 18, 2024 10:30
@Icxolu
Copy link
Member Author

Icxolu commented Feb 18, 2024

Rebased

Copy link
Contributor

@LilyFirefly LilyFirefly left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks, overall looks great! Got a suggestion for the FIXME...

assert!(sub_ty.is_subclass(base_ty).unwrap());
assert!(sub_ty.is_subclass(&base_ty).unwrap());

// FIXME: Whats the new API to do this?
Copy link
Member

Choose a reason for hiding this comment

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

I think you're looking for Py::new or Bound::new, I guess we should deprecate PyCell::newin favour of one of these.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, nice and easy! (And thanks for catching, I totally forget that i put that there)

I guess we should deprecate PyCell::new in favour of one of these.

That's a good idea, I'll look into that 👍

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Perfect, thanks!

@davidhewitt davidhewitt added this pull request to the merge queue Feb 18, 2024
Merged via the queue into PyO3:main with commit 4ce9c35 Feb 18, 2024
@Icxolu Icxolu deleted the get-type branch February 18, 2024 19:36
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.

3 participants