-
Notifications
You must be signed in to change notification settings - Fork 56
Gzip subproc #88
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
Gzip subproc #88
Conversation
|
My proposal is #89. |
| static struct profbuf_s *volatile current_codes; | ||
| #endif | ||
|
|
||
| #define ENABLE_GZIP (defined(__unix__) || defined(__APPLE__)) |
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.
Why not simply use gzip all the time, and make it an implementation detail of the profile format?
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.
That's what I'm doing in this PR; the flag is only so that you can disable it if zlib is not available on your platform. But if we were to implement the gzip "binary" in Python then there would be no way to disable gzip. (Well, maybe it could be added later if it causes performance problems for someone.)
|
Do you think you can build binary wheel? |
|
Sure, but what does that have to do with this PR? |
|
Hi, I'll have a look shortly, looks like a good idea in general
|
|
So I like the approach like #89, but I would prefer (as mentioned there) if it's just a notch more transparent - that is that the header stays the same and the contents are gzipped, so user does not even see it being a visible feature |
|
Could you be a bit more specific?
What do you mean by preserving the header? If we gzip the complete profile then obviously the header is gzipped as well |
|
So, as I said, I'm ok if this is too much work, but ideally:
It requires at the very least thinking how much faster is gzip as a subprocess itself. |
|
Ok, then essentially this is how it's done in this pull request but with a Python gzip implementation. I'll implement that. I can do some benchmarking Python gzip vs C gzip. I'll also add an option to disable gzip entirely. |
|
FYI, |
|
See #90 |
Previous discussion:
@fijal
@jonashaag