Skip to content

Conversation

joux3
Copy link
Contributor

@joux3 joux3 commented May 30, 2016

I started wondering why very similar packets didn't compress better. Turns out the Node.js API for zlib is a tad bit misleading: the kind of flush needs to be specified when calling flush even if it's been specified in the options (see https://nodejs.org/api/zlib.html#zlib_zlib_flush_kind_callback).

This pull requests calls flush with the correct level and adds a test that checks that the compression actually makes similar packets smaller.

Note: I gave this only a very preliminary testing. Please do testing across more browsers before even considering merging.

@JacksonTian
Copy link
Contributor

It seems OK.

@BenV
Copy link

BenV commented Jul 28, 2016

Would be great to get this merged, we also noticed the poor compression ratio.

@BenV
Copy link

BenV commented Jul 29, 2016

I'm not against that, but in this case I think Z_SYNC_FLUSH is "correct", it doesn't quite make sense to restart the compression for each packet, if that is the desired behavior it can be controlled with the server/client context takeover options, right?

Note that the flush level (https://nodejs.org/api/zlib.html#zlib_zlib_flush_kind_callback) is not the same as the compression level. I was actually thinking of doing a pull for configuring the compression level this weekend, as we would like to change ours to fastest.

More on the flush modes can be found here: http://www.bolet.org/~pornin/deflate-flush.html, in particular: A full flush degrades the compression efficiency since it removes sequence sharing opportunities for the next 32 kB of data

@lpinca
Copy link
Member

lpinca commented Nov 7, 2016

@joux3 can you please rebase this?

@joux3 joux3 force-pushed the flush-with-proper-level branch from 28700e1 to 88d4636 Compare November 11, 2016 22:26
@joux3
Copy link
Contributor Author

joux3 commented Nov 11, 2016

Done @lpinca. Further review for the Sender.test.js is needed, I just changed the asserts as the actual byte counts changed after my fix. I'm not familiar enough with the codebase and WebSocket framing to verify if that is indeed OK.

@lpinca
Copy link
Member

lpinca commented Nov 12, 2016

I merge this, it should be easy to revert/fix if we find that something isn't working as expected.

@lpinca lpinca merged commit bdcd18b into websockets:master Nov 12, 2016
@lpinca
Copy link
Member

lpinca commented Nov 12, 2016

Thanks!

@joux3 joux3 deleted the flush-with-proper-level branch November 15, 2016 19:15
@lpinca lpinca mentioned this pull request Dec 12, 2016
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.

4 participants