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

deprecate GILPool #3947

Merged
merged 4 commits into from
Mar 15, 2024
Merged

deprecate GILPool #3947

merged 4 commits into from
Mar 15, 2024

Conversation

davidhewitt
Copy link
Member

I realised this is part of the GIL Refs API yet to be deprecated!

... this one might be worth adding a CHANGELOG entry for?

src/gil.rs Outdated
not(feature = "gil-refs"),
deprecated(
since = "0.21.0",
note = "`GILPool` will have no function after PyO3's GIL Refs API is removed"
Copy link
Member

Choose a reason for hiding this comment

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

I would make this into something that is actionable in 0.21.0, i.e. GILPool does have no function if the GIL Refs API is not used.


let obj = py.eval_bound("object()", None, None).unwrap();
obj.to_object(py)
py.eval_bound("object()", None, None).unwrap().unbind()
Copy link
Member

Choose a reason for hiding this comment

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

Ah, we can finally have nice things!

pub unsafe fn new_pool(self) -> GILPool {
GILPool::new()
}
}

impl Python<'_> {
/// Creates a scope using a new pool for managing PyO3's owned references.
/// Creates a scope using a new pool for managing PyO3's GIL Refs. This has no functional
Copy link
Member

Choose a reason for hiding this comment

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

Partially editing text left?

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.

Some nits on the wording.

@adamreichold
Copy link
Member

... this one might be worth adding a CHANGELOG entry for?

Definitely. Or even a sentence in the migration guide? And in the section where GILPool usage is explained if that is not completely gone after the docs rewrite.

I think the advice would be along the lines of "the coarse-grained memory management using GILPool is replaced by each Bound fulfilling the same role of each individual object"?

@davidhewitt
Copy link
Member Author

Thanks for the review! I will aim to fixup tomorrow evening 👍

@davidhewitt
Copy link
Member Author

Sickness in the family again has left me wiped out, I am hoping to get to this tomorrow evening now...

@davidhewitt
Copy link
Member Author

Ok; that's done. As it was just wording changes I'll proceed to merge and then see what else can be tidied up ahead of the final 0.21 release.

@davidhewitt davidhewitt enabled auto-merge March 15, 2024 08:22
Copy link

codspeed-hq bot commented Mar 15, 2024

CodSpeed Performance Report

Merging #3947 will improve performances by 10.03%

Comparing davidhewitt:deprecate-pool (e8da18b) with main (989d2c5)

Summary

⚡ 1 improvements
✅ 66 untouched benchmarks

Benchmarks breakdown

Benchmark main davidhewitt:deprecate-pool Change
extract_float_downcast_fail 572.8 ns 520.6 ns +10.03%

@davidhewitt davidhewitt added this pull request to the merge queue Mar 15, 2024
Merged via the queue into PyO3:main with commit dcba984 Mar 15, 2024
41 of 42 checks passed
@davidhewitt davidhewitt deleted the deprecate-pool branch March 15, 2024 10:59
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