Skip to content

Conversation

@jonashaag
Copy link
Contributor

@jonashaag jonashaag commented Jul 13, 2016

Previous discussion:

@fijal

I'm a bit unhappy about packing the gzip stuff into C. I can be convinced it's a good idea, but a pure python solution would make it magically work with PyPy too for example. Does not seem too hard, if you have opinions I would like to know them

@jonashaag

It's just a matter of performance, although I haven't done any benchmarking with a pure-Python solution. It shouldn't be a problem if you have multiple cores and large enough pipe buffers, so I guess a pure-Python solution would be fine as well!

@jonashaag jonashaag mentioned this pull request Jul 13, 2016
@methane
Copy link
Contributor

methane commented Jul 13, 2016

My proposal is #89.

static struct profbuf_s *volatile current_codes;
#endif

#define ENABLE_GZIP (defined(__unix__) || defined(__APPLE__))
Copy link
Contributor

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?

Copy link
Contributor Author

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

@methane
Copy link
Contributor

methane commented Jul 14, 2016

Do you think you can build binary wheel?

@jonashaag
Copy link
Contributor Author

Sure, but what does that have to do with this PR?

@fijal
Copy link
Member

fijal commented Jul 14, 2016

Hi, I'll have a look shortly, looks like a good idea in general
On 14 Jul 2016 8:56 AM, "Jonas Haag" notifications@github.com wrote:

Sure, but what does that have to do with this PR?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#88 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAFPuDZb1mZZSSRe7ueubZwI1P7Wn1qoks5qVd2GgaJpZM4JLKv0
.

@fijal
Copy link
Member

fijal commented Jul 14, 2016

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

@jonashaag
Copy link
Contributor Author

jonashaag commented Jul 14, 2016

Could you be a bit more specific?

  • gzip transparent for the user (=> implementation detail of profile format, with backwards compatibility)?
  • gzip runs as subprocess, with pipe?
  • gzip subprocess implemented as Python "binary" or use the system-level gzip (no Windows support)?
  • OR gzip after complete profile has been generated?

What do you mean by preserving the header? If we gzip the complete profile then obviously the header is gzipped as well

@fijal
Copy link
Member

fijal commented Jul 14, 2016

So, as I said, I'm ok if this is too much work, but ideally:

  • gzip would run as a subprocess (but subprocess would be call "python")
  • it would be an implementation detail with backwards compatibility using python to push data through

It requires at the very least thinking how much faster is gzip as a subprocess itself.

@jonashaag
Copy link
Contributor Author

jonashaag commented Jul 14, 2016

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.

@methane
Copy link
Contributor

methane commented Jul 14, 2016

FYI, python -m gzip is like gzip -9. It can be used for windows fallback.
On unix, I prefer gzip command to -m gzip

@jonashaag jonashaag closed this Jul 14, 2016
@jonashaag
Copy link
Contributor Author

See #90

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.

3 participants