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

Add param to enable ws per-message deflate #365

Merged

Conversation

mvollrath
Copy link
Contributor

Tornado has its own per-message deflate compression option, which
compresses each WebSocket message. The compression level should be
roughly equivalent to PNG compression, depending on whether the message is
JSON or binary (CBOR). The encoding/decoding time will be much faster
than protocol PNG compression.

This param should be enabled when wire size is important, e.g. not
connecting to localhost.

Tornado has its own per-message deflate compression option, which
compresses each WebSocket message.  The compression level should be
roughly equivalent to PNG compression, depending on whether the message is
JSON or binary (CBOR).  The encoding/decoding time will be much faster
than protocol PNG compression.

This param should be enabled when wire size is important, e.g. not
connecting to localhost.
if not cls.use_compression:
return None

return {}
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to document the behaviour of this? At least a copy paste from Tornado's docs can help:

If this method returns None (the default), compression will be disabled. If it returns a dict (even an empty one), it will be enabled.

Should we expose the compression and memory level as well? IMO at least a comment describing the defaults would suffice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't see any of the websocket server's params documented anywhere, I'm happy to add a note if you can point me at a place to leave it. I assume the Tornado docs will be the first point of reference for anybody looking at this.

Compression and memory level only became available in Tornado 4.5, so if we exposed them it might take extra work to support kinetic. I didn't feel like adjusting those settings would be particularly valuable.

@mvollrath mvollrath mentioned this pull request Nov 8, 2018
4 tasks
@viktorku viktorku merged commit c86fe4b into RobotWebTools:develop Nov 9, 2018
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

2 participants