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

Deflate extension #5

Closed
nhooyr opened this issue Sep 6, 2018 · 18 comments
Closed

Deflate extension #5

nhooyr opened this issue Sep 6, 2018 · 18 comments

Comments

@nhooyr
Copy link
Owner

nhooyr commented Sep 6, 2018

See https://tools.ietf.org/html/rfc7692

Investigate whether we want this.

@nhooyr nhooyr added the p2 label Sep 6, 2018
@nhooyr
Copy link
Owner Author

nhooyr commented Sep 7, 2018

See gorilla/websocket#3

@nhooyr nhooyr removed the p2 label Mar 14, 2019
@nhooyr
Copy link
Owner Author

nhooyr commented Mar 29, 2019

We probably do want this. Maybe just add a Compress method to MessageWriter to allow compressing messages and transparently decompress compressed messages being read.

@nhooyr nhooyr added this to the v1.0.0 milestone Mar 30, 2019
@nhooyr
Copy link
Owner Author

nhooyr commented Apr 7, 2019

Definitely something like #36

Would work just like browsers then which would be nice.

@nhooyr nhooyr modified the milestones: v1.0.0, v1.1.0 Apr 12, 2019
@nhooyr
Copy link
Owner Author

nhooyr commented Apr 16, 2019

@nhooyr nhooyr changed the title Compression support WebSocket Deflate Extension Apr 20, 2019
@nhooyr nhooyr changed the title WebSocket Deflate Extension Support Deflate Extension Apr 20, 2019
@nhooyr nhooyr changed the title Support Deflate Extension Deflate Extension Apr 20, 2019
@nhooyr nhooyr changed the title Deflate Extension Deflate extension Apr 20, 2019
@nhooyr nhooyr removed this from the v1.1.0 milestone Apr 26, 2019
@nhooyr
Copy link
Owner Author

nhooyr commented May 30, 2019

@coadler this issue is also ripe for work if you're interested.

@nhooyr
Copy link
Owner Author

nhooyr commented May 30, 2019

Though I would suggest you wait for #81 as it is a rather large refactoring of the internals.

@nhooyr
Copy link
Owner Author

nhooyr commented May 30, 2019

I think the move here is to expose zero flags. We should automatically compress text frames larger than X bytes, where X is determined experimentally. My guess is anywhere from 128 -512 bytes. We should also test every level and see which works best. Last time I tested, the lowest level had the most bang for buck.

For binary frames, I'm thinking we shouldn't compress at all as most binary protocols do not compress very well afaik unless you have a lot of duplicate data.

These configuration would work fantastic out of the box for most setups and require zero knobs.

@nhooyr nhooyr removed the complex label May 31, 2019
@nhooyr
Copy link
Owner Author

nhooyr commented May 31, 2019

So after some testing, I think the sweet spot is going to be 256 bytes for text. Above that, we should automatically use deflate compression at the lowest level if the peer supports it. Should be good for 99.99% of applications.

For binary messages, we should use a cutoff of 512 bytes. The reason we compress anyway for binary protocols too is I'm guessing that when the message size exceeds 512 bytes, its extremely likely the message can be compressed well.

Should also document this on Conn and link to this issue requesting feedback.

Thoughts @coadler

This should reduce your bandwidth significantly with the discord gateway for your bot.

@nhooyr
Copy link
Owner Author

nhooyr commented May 31, 2019

Disabling compression entirely would be as easy as setting "Sec-WebSocket-Extension" to "" on the response or request header, if needed.

@nhooyr
Copy link
Owner Author

nhooyr commented May 31, 2019

I think the deflate extension is not worth support at all.

See my investigation in gorilla/websocket#203 (comment)

It will cause a large memory spike as every flate writer takes up a ridiculous 1.2 megabytes in memory.

@coadler
Copy link
Contributor

coadler commented May 31, 2019

If deflate is out of the question, could we potentially still support zlib/zstd? Or would these be best implemented as some sort of wrapper library. For zstd we'd need https://github.com/DataDog/zstd or similar which now brings an external dep and cgo into the mix.

I'm not overly familiar with how websocket compression negotiation works, but for Discord it's picked based on a query param e.g. wss://gateway.discord.gg/?v=6&compress=zlib-stream. So if this was done inside the library, we'd need a way to manually specify the algorithm within *Conn.

@nhooyr
Copy link
Owner Author

nhooyr commented May 31, 2019

Would be best as a wrapper lib. I'm happy to link to it from the README.md though. Also, why not https://golang.org/pkg/compress/zlib/?

@nhooyr
Copy link
Owner Author

nhooyr commented May 31, 2019

It doesn't have the same allocation overhead as compress/flate so it should have solid performance.

@coadler
Copy link
Contributor

coadler commented May 31, 2019

I only meant zstd would incur an external dependency because zlib has a std package

@nhooyr
Copy link
Owner Author

nhooyr commented May 31, 2019

Ah I wasn't aware there is a difference between the two. There's also https://www.reddit.com/r/golang/comments/be7esc/a_pure_go_port_of_the_zstandard_zstd_decompression/

@nhooyr
Copy link
Owner Author

nhooyr commented May 31, 2019

Another issue with WebSocket compression: https://bugs.chromium.org/p/chromium/issues/detail?id=163882

Looks like browsers will unconditionally compress every message, even smaller ones, thereby increasing their size.

@nhooyr
Copy link
Owner Author

nhooyr commented May 31, 2019

Interestingly, Firefox will compress all payloads but only send the ones smaller than the original payload: https://hg.mozilla.org/mozilla-central/diff/724f0a71d62171da1357e6c1f93453359e54206b/netwerk/protocol/websocket/WebSocketChannel.cpp#l1.277

@nhooyr nhooyr closed this as completed in 15b8365 May 31, 2019
@nhooyr
Copy link
Owner Author

nhooyr commented Sep 29, 2019

To summarize:

We do not support the deflate compression extension because Go's compress/flate library is very memory intensive and browsers do not handle WebSocket compression intelligently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants