-
Notifications
You must be signed in to change notification settings - Fork 214
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
python-bindings: Release the python GIL whenever it makes sense #311
python-bindings: Release the python GIL whenever it makes sense #311
Conversation
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.
I can't really say whether it's going to crash or not like last time, but we might as well merge it and start testing
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.
I think you should review all places where we copy from (and mayby into) python objects, and ensure the GIL is held
fe0db80
to
c8f397a
Compare
@arvidn @mariano54 Good points 👍 See here. |
@@ -55,17 +45,19 @@ PYBIND11_MODULE(blspy, m) | |||
throw std::invalid_argument( | |||
"Length of bytes object not equal to PrivateKey::SIZE"); | |||
} | |||
py::gil_scoped_release release; | |||
auto data_ptr = reinterpret_cast<const uint8_t *>(info.ptr); |
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.
info is still in python right?
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.
Hmm i assumed that the PyObject_GetBuffer
in buffer::request
copies it over but it might not from a quick deeper look at the code.. will review again tomorrow (or just move the release because it anyway doesn't really matter that much in this case)!
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.
Okay changed it, see 353a623. There were even some more cases were info.ptr
was passed into Bytes
and then into ::FromBytes
which still would have been an access to that memory without the GIL (Bytes
only stores the pointer).
Let's get this in after 1.3? |
Yeah sure good to me, im anyway waiting for 1.3 that i can start to bug again about all the GIL release PRs in the other repos too :) |
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.
it would be nice to have a stress test where we run a bunch of these operations in a loop in multiple threads simultaneously
…-Network#311) * python-bindings: Release the python GIL where possible * python-bindings: Don't access `buffer_info::ptr` without the GIL
No description provided.