-
Notifications
You must be signed in to change notification settings - Fork 599
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
Transport compression support #3036
Conversation
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.
Just some initial comments, alongside this I don't think you should implement buffer resizing logic if not necessary as lowering the buffer size isn't going to lead to a noticeable memory usage improvement compared to the code complexity.
Trying to get |
c6d267e
to
2e07df3
Compare
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.
Hmm, this looks good, just a small concern with the zstd library.
let mut inflater = ZstdInflater::create(); | ||
inflater.init().expect("Failed to initialize Zstd decompressor"); |
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 kind of create
and init
pattern is making me iffy about zstd_safe
, especially not being able to find any documentation on what it possibly errors on. Is there another library for it?
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 is restating things that have been said on discord for completeness sake.
zstd_safe
is using bindings to the zstd reference implementation in C (https://github.com/facebook/zstd). The (to my knowledge) only other crate for zstd decompression that is not relying on zstd_safe
in some way is ruzstd
. Looking at the docs for ruzstd
it does not look that nice to work with in our context at least but maybe I'm wrong about that. The performance hit compared to the C implementation is unclear, should probably be tested.
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.
Okay, I'm good with merging this as-is, if it has been tested.
I retested the current state with |
Co-authored-by: GnomedDev <david2005thomas@gmail.com>
Co-authored-by: GnomedDev <david2005thomas@gmail.com>
Co-authored-by: GnomedDev <david2005thomas@gmail.com>
Co-authored-by: GnomedDev <david2005thomas@gmail.com>
Reworks how the current payload compression is handled and adds support for transport compression.
zlib-stream
decompressionzstd-stream
decompressionShould close #693.
And unlike #1508 it concats the WS query params with
&
instead of?
.#1508 had some logic to periodically resize/shrink buffers (even tho it seems like it was not hooked up to be called anywhere?) which this PR does not
yethave.