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

python-bindings: Release the python GIL whenever it makes sense #311

Merged

Conversation

xdustinface
Copy link
Contributor

No description provided.

Copy link
Contributor

@mariano54 mariano54 left a 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

python-bindings/pythonbindings.cpp Show resolved Hide resolved
python-bindings/pythonbindings.cpp Show resolved Hide resolved
Copy link
Contributor

@arvidn arvidn left a 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

python-bindings/pythonbindings.cpp Show resolved Hide resolved
python-bindings/pythonbindings.cpp Show resolved Hide resolved
@xdustinface
Copy link
Contributor Author

xdustinface commented Jan 25, 2022

@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);
Copy link
Contributor

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?

Copy link
Contributor Author

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)!

Copy link
Contributor Author

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).

python-bindings/pythonbindings.cpp Outdated Show resolved Hide resolved
python-bindings/pythonbindings.cpp Show resolved Hide resolved
python-bindings/pythonbindings.cpp Outdated Show resolved Hide resolved
@mariano54
Copy link
Contributor

Let's get this in after 1.3?

@xdustinface
Copy link
Contributor Author

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 :)

Copy link
Contributor

@arvidn arvidn left a 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

@wjblanke wjblanke merged commit 5f15cfb into Chia-Network:main Apr 4, 2022
UdjinM6 pushed a commit to UdjinM6/bls-signatures that referenced this pull request Sep 6, 2022
…-Network#311)

* python-bindings: Release the python GIL where possible

* python-bindings: Don't access `buffer_info::ptr` without the GIL
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