-
-
Notifications
You must be signed in to change notification settings - Fork 427
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 optional gzip compression #1389
base: master
Are you sure you want to change the base?
Conversation
|
should_compress = false; | ||
} | ||
} | ||
header += "\r\n"; |
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.
This use of std::strings causes unnecessary copying of buffers.
Rewrite both the gzip compressor and this to write directly to m_buffer, pass e.g. a lambda function to gzip_compressor that does the writing.
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.
This causes a chicken-and-egg problem where we need to know the Content-Length to figure out where the write cursor in m_buffer should start from (given that there's a more than decent chance we might drop a digit in the size string in the course of compression), but we don't know the length until the compression itself is complete.
The only way I can think to work around that is to first write the compressed output directly a little ways into the m_buffer and std::memmove
it back to the correct position after the headers have been written. I'll go in that direction unless you tell me otherwise.
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.
Don't see why you need to do it in such a convoluted way, just pass a lambda function that does the writing and have it resize m_buffer
if more is needed.
You can change m_buffer
to std::vector to make it cleaner.
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.
Clang-Tidy
found issue(s) with the introduced code (1/1)
e74b4fe
to
8de5eca
Compare
@@ -1,14 +1,15 @@ | |||
#include "rpc/scgi_task.h" | |||
#include "config.h" |
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.
config.h
should always be the first header included and be separate from the others.
This introduces a single new value command
network.gzip_response_min_size
for determining if the response should be gzipped (I'm very open to better names for that variable). It'll additionally require the SCGI client to announce support for gzip responses in theACCEPT_ENCODING
header. Since this is of dubious benefit to most use cases, it's disabled by default via setting it to a value of less than 0. This also technically makes zlib a new dependency for rtorrent, but since it's already a hard dependency for libtorrent that didn't seem unreasonable.A fun little interaction is that calling
network.gzip_response_min_size
over RPC impacts the response immediately.