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

Transport compression support #3036

Merged
merged 7 commits into from
Nov 18, 2024
Merged

Conversation

fgardt
Copy link
Contributor

@fgardt fgardt commented Nov 14, 2024

Reworks how the current payload compression is handled and adds support for transport compression.

  • implement zlib-stream decompression
  • implement zstd-stream decompression

Should 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 yet have.

@github-actions github-actions bot added the gateway Related to the `gateway` module. label Nov 14, 2024
Copy link
Member

@GnomedDev GnomedDev left a 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.

@fgardt
Copy link
Contributor Author

fgardt commented Nov 15, 2024

zlib-stream decompression does work now.
Tested by running e01_basic_ping_bot with all gateway intents.

Trying to get zstd-stream decompression working next.

@github-actions github-actions bot added framework Related to the `framework` and `framework::standard` modules and/or the `command_attr` crate model Related to the `model` module. client Related to the `client` module. builder Related to the `builder` module. cache Related to the `cache`-feature. http Related to the `http` module. utils Related to the `utils` module. examples Related to Serenity's examples. ci Related to the continuous integration. labels Nov 15, 2024
@fgardt fgardt force-pushed the transport-compression branch from c6d267e to 2e07df3 Compare November 15, 2024 20:28
@fgardt fgardt marked this pull request as ready for review November 15, 2024 20:28
@github-actions github-actions bot removed framework Related to the `framework` and `framework::standard` modules and/or the `command_attr` crate model Related to the `model` module. client Related to the `client` module. builder Related to the `builder` module. cache Related to the `cache`-feature. http Related to the `http` module. utils Related to the `utils` module. examples Related to Serenity's examples. ci Related to the continuous integration. labels Nov 15, 2024
@fgardt fgardt requested a review from GnomedDev November 15, 2024 20:28
Copy link
Member

@GnomedDev GnomedDev left a 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.

Comment on lines +209 to +210
let mut inflater = ZstdInflater::create();
inflater.init().expect("Failed to initialize Zstd decompressor");
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

@GnomedDev GnomedDev left a 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.

@fgardt
Copy link
Contributor Author

fgardt commented Nov 18, 2024

I retested the current state with e01_basic_ping_bot and all compression settings worked without any issues.

@GnomedDev GnomedDev merged commit b20151f into serenity-rs:next Nov 18, 2024
21 checks passed
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request Dec 8, 2024
Co-authored-by: GnomedDev <david2005thomas@gmail.com>
arqunis pushed a commit to arqunis/serenity that referenced this pull request Jan 16, 2025
Co-authored-by: GnomedDev <david2005thomas@gmail.com>
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request Feb 1, 2025
Co-authored-by: GnomedDev <david2005thomas@gmail.com>
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request Feb 14, 2025
Co-authored-by: GnomedDev <david2005thomas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gateway Related to the `gateway` module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants