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 Clone for PyObject / Py<T> #908

Merged
merged 1 commit into from
May 11, 2020
Merged

Conversation

davidhewitt
Copy link
Member

I had this idea suddenly that similar to how we store pending Py_DECREF, I believe we can also store pending Py_INCREF, as long as all increfs are done before decrefs.

This allows Py and PyObject to have Clone implemented. If the GIL is held the reference count is updated immediately, otherwise the reference count increase is saved for the next time a thread acquires the GIL.

I have been mulling this over in my head and cannot find a way that this produces a dangling PyObject. Please do challenge me on this one though - I would prefer to be proved wrong and we don't merge this if it is not safe.

I will add tests later - gotta go to work! Just wanted to share it with you first as I think having Clone for these types will open the way for simplifying some things. (e.g. I think we could potentially add blanket impl ToPyObject for all T: Clone? Haven't thought too hard about that.)

@davidhewitt davidhewitt mentioned this pull request May 6, 2020
@kngwyu
Copy link
Member

kngwyu commented May 6, 2020

I had this idea suddenly that similar to how we store pending Py_DECREF, I believe we can also store pending Py_INCREF, as long as all increfs are done before decrefs.

Wow. Awesome 👍

I have been mulling this over in my head and cannot find a way that this produces a dangling PyObject

The logic seems correct to me, though we need to test it carefully.

@skreborn
Copy link

skreborn commented May 6, 2020

I think we could potentially add blanket impl ToPyObject for all T: Clone

That sounds excellent! I would love to remove a bunch of those straightforward implementations from my code base.

@davidhewitt
Copy link
Member Author

FYI now that we have merged #912 and #913 I should be able to finish writing tests for this tomorrow 👍

@davidhewitt davidhewitt force-pushed the clone-pyobject branch 2 times, most recently from 869d5df to f28176d Compare May 10, 2020 10:09
@davidhewitt
Copy link
Member Author

Pushed some tests - ready for review!

@@ -501,32 +544,23 @@ mod test {

#[test]
fn test_allow_threads() {
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to simplify this test because otherwise it could have race conditions with test_clone_in_allow_threads, as both of them are testing the same global state.

Copy link
Member Author

Choose a reason for hiding this comment

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

EDIT: I had to remove test_clone_in_allow_threads anyway, because it was very brittle checking the global state with potentially many other tests all changing the state. But I still think the simplification I did here makes sense.

@davidhewitt davidhewitt force-pushed the clone-pyobject branch 4 times, most recently from a40225e to cd889e8 Compare May 10, 2020 10:59
// Since we have a valid PyString and replace any surrogates, assume success.
debug_assert!(!py_bytes.is_null());
// ensure DECREF will be called
gil::register_pointer(NonNull::new(py_bytes).unwrap());
Copy link
Member Author

Choose a reason for hiding this comment

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

Usage of register_pointer was broken since my change to pointer drop immediately with GIL, suprised this test wasn't failing (probably luck).

I have rewritten this to use usual owned references.

Copy link
Member

Choose a reason for hiding this comment

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

👍

// Since we have a valid PyString and replace any surrogates, assume success.
debug_assert!(!py_bytes.is_null());
// ensure DECREF will be called
gil::register_pointer(NonNull::new(py_bytes).unwrap());
Copy link
Member

Choose a reason for hiding this comment

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

👍

@kngwyu
Copy link
Member

kngwyu commented May 11, 2020

Thank you!

@kngwyu kngwyu merged commit 5e7ce95 into PyO3:master May 11, 2020
@davidhewitt davidhewitt deleted the clone-pyobject branch August 10, 2021 07:19
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.

3 participants