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

port Python::eval to Bound API #3806

Merged
merged 1 commit into from
Feb 9, 2024
Merged

port Python::eval to Bound API #3806

merged 1 commit into from
Feb 9, 2024

Conversation

Icxolu
Copy link
Contributor

@Icxolu Icxolu commented Feb 6, 2024

Part of #3684 and split from #3716

This converts to the Python::eval function to the new API. For easier reviewing this focuses purely on eval/eval_bound and leaves the very similar Python::run method as well as the PyDict methods for followup PRs.

Copy link

codspeed-hq bot commented Feb 6, 2024

CodSpeed Performance Report

Merging #3806 will degrade performances by 15.63%

Comparing Icxolu:python-eval (33dc33e) with main (9bb0011)

Summary

⚡ 1 improvements
❌ 3 regressions
✅ 75 untouched benchmarks

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

Benchmarks breakdown

Benchmark main Icxolu:python-eval Change
list_via_downcast 157.2 ns 185 ns -15.02%
not_a_list_via_downcast 272.2 ns 244.4 ns +11.36%
sequence_from_tuple 232.8 ns 260.6 ns -10.66%
sequence_from_list 300 ns 355.6 ns -15.63%

@Icxolu Icxolu force-pushed the python-eval branch 2 times, most recently from cfbc3ce to 6f2019f Compare February 6, 2024 21:09
@davidhewitt
Copy link
Member

Sorry for the slight delay, I will do my best to review this tonight.

@davidhewitt davidhewitt added the CI-skip-changelog Skip checking changelog entry label Feb 8, 2024
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.

Fantastic, this is a much more reviewable first step. Thanks again and sorry I was a bit stretched for time in the last couple of days!

Comment on lines +645 to +647
globals.map(PyNativeType::as_borrowed).as_deref(),
locals.map(PyNativeType::as_borrowed).as_deref(),
Copy link
Member

Choose a reason for hiding this comment

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

👍 very tidy temporary solution!

@davidhewitt davidhewitt added this pull request to the merge queue Feb 8, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 8, 2024
@davidhewitt
Copy link
Member

Uff rust nightly change broke the merge.

@Icxolu
Copy link
Contributor Author

Icxolu commented Feb 9, 2024

Rebased, to include CI fixes

@davidhewitt davidhewitt enabled auto-merge February 9, 2024 16:58
@davidhewitt
Copy link
Member

👍 sorry I forgot to rerun the merge queue, it should have automatically picked them up but I just got sidetracked.

@davidhewitt davidhewitt added this pull request to the merge queue Feb 9, 2024
Merged via the queue into PyO3:main with commit 2fedea2 Feb 9, 2024
32 of 38 checks passed
@Icxolu Icxolu deleted the python-eval branch February 9, 2024 18:00
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