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

Nogil #116

Merged
merged 3 commits into from
Oct 24, 2016
Merged

Nogil #116

merged 3 commits into from
Oct 24, 2016

Conversation

robbmcleod
Copy link
Contributor

Permits release of Python Global Interpreter Lock (GIL) inside of c-blosc calls. Tested with bench/threadpool.py using the multiprocesing.ThreadPool class to map the operations in compressing frames of an image stack (aka movie). Intended to be used in situations where other bounds (e.g. file I/O, network I/O) could be significant, such that asychronous read/write/compress operations could make the compression step essentially free.

Usage:
blosc.set_releasegil(True)

Operate multiple blosc streams at a time

@esc
Copy link
Member

esc commented Oct 24, 2016

LGTM

@esc
Copy link
Member

esc commented Oct 24, 2016

I'll use the zeromq approach and just merge. I had a brief (not too detailed look) and it looks good to me. Merging it means it's in master, if you find any issues or amendments, just make another pull-request and I'll merge that too!

@esc esc merged commit 78a55cd into Blosc:master Oct 24, 2016
{
int gilstate, old_gilstate;

if (!PyArg_ParseTuple(args, "i:gilstate", &gilstate))
Copy link
Member

Choose a reason for hiding this comment

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

Question: is this all that is required to release the GIL?

@@ -189,15 +209,38 @@ compress_helper(void * input, size_t nbytes, size_t typesize,
}

/* Compress */
cbytes = blosc_compress(clevel, shuffle, typesize, nbytes,
input, PyBytes_AS_STRING(output),
// RAM: I don't think this macro requires the Python interpreter but let's leave it outside
Copy link
Member

Choose a reason for hiding this comment

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

You could remove the RAM comments and persist your thoughts. We can always use git blame to figure out who wrote the comments. This is valid for all your comments.

@esc
Copy link
Member

esc commented Oct 24, 2016

Note: this reminds me a little of the bcolz commit that releases the GIL.

@esc
Copy link
Member

esc commented Oct 24, 2016

Also, a pull-request that updates the docs would be nice.

@robbmcleod
Copy link
Contributor Author

Ok I'll work on an update to the docs and then revert my initials in the comments sometime soon, thanks.

@esc
Copy link
Member

esc commented Oct 24, 2016

Cool, just make more pull-requests and I'll happily merge them.

@mrocklin
Copy link

Thanks for working on this @robbmcleod . There are many others who will benefit :)

@mrocklin mrocklin mentioned this pull request Oct 24, 2016
@robbmcleod
Copy link
Contributor Author

No problem @mrocklin

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

3 participants