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

Add Py::get for GIL-independent access to frozen classes. #3158

Merged
merged 1 commit into from
May 18, 2023
Merged

Conversation

adamreichold
Copy link
Member

@adamreichold adamreichold commented May 16, 2023

@davidhewitt Is this what you had in mind for #3154?

The name is an obvious candidate for bikeshedding.

Trying to write an example, I noticed that making PyCell::get_frozen public is most likely not useful as there is no way to safely get a &PyCell without acquiring the GIL first?

@adamreichold adamreichold force-pushed the get-frozen branch 3 times, most recently from 0d5e834 to 63acd0c Compare May 16, 2023 20:00
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, yes this is what I was thinking!

Trying to write an example, I noticed that making PyCell::get_frozen public is most likely not useful as there is no way to get safely get a &PyCell without acquiring the GIL first?

Good point. It may still be nice to have this method for symmetry with Py? Also, it might be more performant (it can avoid the .borrow() / .try_borrow() / PyRef dance entirely, though that is probably optimised out anyway for frozen classes).

As for name... I'd quite like to have this just be something concise like .get() or .as_ref(), though maybe those names make more sense when frozen is the default, and it's good to have frozen in the name for now?

src/instance.rs Outdated Show resolved Hide resolved
@adamreichold
Copy link
Member Author

Good point. It may still be nice to have this method for symmetry with Py? Also, it might be more performant (it can avoid the .borrow() / .try_borrow() / PyRef dance entirely, though that is probably optimised out anyway for frozen classes).

I was also thinking that the thread and borrow checking would be optimized out, but then again it does not hurt to give the compiler less work to do. Will add it.

As for name... I'd quite like to have this just be something concise like .get() or .as_ref(), though maybe those names make more sense when frozen is the default, and it's good to have frozen in the name for now?

I think get is fine and will change the name.

Or I wonder if we should add a new section to the guide for frozen talking about the tradeoff (i.e. you get access to this new method but lose the ability for &mut self in pymethods and borrow_mut).

Indeed, if we want to push the whole ecosystem towards this as a new default, we should probably talk about it in the docs. Its not my favourite part but I'll give it a try.

(We could potentially also disable PyCell::borrow and Py::borrow for frozen classes, to make the distinction greater that frozen does not need dynamic borrow checking.)

I think we should keep borrow as this allows to handle non-frozen (molten?) and frozen classes the same in generic context. A human should not write code borrowing frozen classes directly, but a generic type using PyRef<T> also should not have to care whether the class is frozen. I will add a note linking get to the docs of (try_)borrow though.

@adamreichold adamreichold changed the title Add Py::get_frozen for GIL-independent access to frozen classes. Add Py::get for GIL-independent access to frozen classes. May 17, 2023
@adamreichold
Copy link
Member Author

adamreichold commented May 17, 2023

Ok, so I gave the guide part a try. Please have another look when convenient.

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 looking great, just some small suggestions to wording. Needs tests!

guide/src/class.md Outdated Show resolved Hide resolved
guide/src/class.md Outdated Show resolved Hide resolved
guide/src/class.md Outdated Show resolved Hide resolved
guide/src/class.md Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Member

EDIT: I see the examples function as tests, it's unfortunate they don't contribute to coverage yet.

@adamreichold
Copy link
Member Author

EDIT: I see the examples function as tests, it's unfortunate they don't contribute to coverage yet.

Yes, this is also a big issue for rust-numpy but I don't think there is anything we can do until taiki-e/cargo-llvm-cov#2 is resolved.

I will add some UI tests nevertheless as the duplication should be harmless. Especially tests that this cannot be used on unfrozen classes should be useful.

@adamreichold
Copy link
Member Author

Coverage appears reasonable now.

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!

bors r+

@bors
Copy link
Contributor

bors bot commented May 18, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit c2986df into main May 18, 2023
@bors bors bot deleted the get-frozen branch May 18, 2023 08:25
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