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

Simplify ReleasePool, remove parking_lot dependency. #899

Merged
merged 1 commit into from
May 5, 2020

Conversation

m-ou-se
Copy link
Contributor

@m-ou-se m-ou-se commented May 4, 2020

Replaces the parking_lot Mutex by a simple AtomicBool spinlock, and simplifies the ReleasePool to contain one Vec instead of pointers to two Vecs.

@m-ou-se
Copy link
Contributor Author

m-ou-se commented May 4, 2020

@davidhewitt Since you wrote this part, can you review this?

I'm curious if there's any specific reason that I'm missing for why

  • the UnsafeCells contained an *mut Vec instead of Vec,
  • the pointers_being_dropped was contained within the ReleasePool,
  • .set_len(0) was used instead of .clear().

This PR changes those things, but let me know if i'm missing something here. (In that case we should probably add a comment about it.)

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.

@m-ou-se thanks so much for all of the cleanups you have been pushing for pyo3 today!

The original two vector code existed long before me, though I was the most recent person to touch it up 😄 . My theory is that there were two reasons for this:

  • While the pointers_to_drop mutex is locked, other Rust threads which might be dropping PyObjects or Py<T> might be blocked from making progress. In all honesty it seems like a very rare case that other threads would be doing anything with PyObjects at the same time, as no other thread would have the GIL.
  • When the Py_DECREF call is made, arbitrary drop code can run, which might lead to further PyObject being dropped. It used to be the case before a recent change of mine that these PyObject would be put straight into the release pool. If the release pool Mutex was still locked then the thread would deadlock.

This second reason in particular forced the need for a two-vector solution, and would only hold the mutex while swapping between them so that the Py_DECREF calls could then be made without the mutex locked. (Using *mut Vec made it cheap to swap between the two vectors.)

But as I recently changed PyObjects to not use the release pool if the gil is held (see register_pointer function), this deadlock problem no longer applies. (Unless people are using the unsafe function assume_gil_aquired when pyo3 doesn't actually have any GILPool set. And they'll be getting much more noticeable crashes if they're doing that!)

As for set_len(0) - that's just something I missed in refactoring, using .clear() looks very sensible to me.

So, in summary, thanks for cleaning this up - I'm in favour of this change 👍

Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

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

Thank you!
As @davidhewitt says, we need this simplification as the situtation changed.

src/gil.rs Outdated Show resolved Hide resolved
Replaces the parking_lot Mutex by a simple AtomicBool spinlock,
and simplifies the ReleasePool to contain one Vec instead of pointers to
two Vecs.
@kngwyu
Copy link
Member

kngwyu commented May 5, 2020

Thanks for the update!

@kngwyu kngwyu merged commit 626268d into PyO3:master May 5, 2020
@programmerjake programmerjake mentioned this pull request May 8, 2020
@programmerjake
Copy link
Contributor

This pull request should have been changed to avoid spinlocks, see #916

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.

None yet

4 participants