-
Notifications
You must be signed in to change notification settings - Fork 677
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 bound dict constructors & py.run variants #3716
Conversation
CodSpeed Performance ReportMerging #3716 will degrade performances by 14.16%Comparing Summary
Benchmarks breakdown
|
bc140a8
to
5e036fa
Compare
The performance "regressions" here are because the benchmarks are now eagerly dropping the containers rather than leaving them on the pool. I'll try to adjust accordingly. The |
99f3230
to
2e85fb3
Compare
Ok, this one is now good to review. The performance regressions still reported are on the ns scale and I expect are sensitive to changes in inlining due to benchmark changes. I'm not bothered by those as they might get change again later, e.g. after #3706 The |
99 changed files in the diff, 99 changed files, |
Yes, I agree, it's a horrible diff. If you've got any suggestions on ways to split it to help review, I'm very open to trying. The problem I found is that adding these constructors basically immediately forced either adding |
... maybe I can try to see what happens if I just do |
(And for what it's worth, I have a feeling that this is one of the largest single diff PRs that we might face. These ones are used in so many tests.) |
src/types/sequence.rs
Outdated
@@ -566,6 +566,7 @@ impl<'v> crate::PyTryFrom<'v> for PySequence { | |||
} | |||
|
|||
#[cfg(test)] | |||
#[cfg_attr(not(feature = "gil-refs"), allow(deprecated))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of why some tests have this blanket applied to mod tests
, I did this for files where the GIL Ref API is tested in some detail in the contained tests, mostly inside src/types
directory. I think we want to keep the GIL Ref tests for coverage anyway, so my feelings were to just do this now as one way to keep churn down, and revisit these after 0.21 is pushed and we're heading towards 0.22.
Please don't worry. Sometimes we'll just have to pull through. I will finish the review eventually. I also understand that it is mostly tests and the changes there are mechanical so I can skim those files. I guess I will focus on the "source" changes in the any case. |
2e85fb3
to
8cd3133
Compare
I've just rebased this, sorry if that disrupts a partial review. I'm going to push a separate PR in a second which adds |
8cd3133
to
84ab6c9
Compare
@Icxolu FYI I just rebased this; it contains |
84ab6c9
to
ed50995
Compare
Thanks for the ping, I must have overlooked this one. I give it a try to see if this can reasonably be split, otherwise we might have to review it like this. |
I think one option is to merge the rest of the new |
So I have played around with this a little. I could split off |
I think that would be excellent, yes please! I can either take responsibility for rebasing this one or leave it to you to eventually replace this one too. |
Closing with thanks to @Icxolu for splitting this up and making a much better end result 🚀 |
This is based on top of #3711
The second commit adds
PyDict
bound constructors. At the same time I also addedPython::eval_bound
,Python::run_bound
, andpy_run_bound!
, because these APIs consume dictionaries and it's extremely churn heavy to change all of these. So to at least do the churn in a single hop, I've pushed it as a single commit.