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

Override default brotli compression level 11 -> 4 #356

Merged
merged 2 commits into from Jul 19, 2023

Conversation

Ameobea
Copy link
Contributor

@Ameobea Ameobea commented Apr 14, 2023

Additional discussion: #287

Motivation

The brotli crate used by async-compression has a default compression level of 11, which is the maximum for brotli. This causes extremely slow compression performance for many response bodies and is definitely an inappropriate compression level for dynamic content.

There's currently an open issue (dropbox/rust-brotli#93) on the brotli crate's repo to change this default, but it hasn't happened at this time.

Solution

This change adds a special case to convert a provided compression level of default to a compression level of 4, which is a reasonable default for dynamic content.

The `brotli` crate used by `async-compression` has a default compression level of 11, which is the maximum for brotli.  This causes extremely slow compression performance for many response bodies and is definitely an inappropriate compression level for dynamic content.

There's currently an open issue (dropbox/rust-brotli#93) on the `brotli` crate's repo to change this default, but it hasn't happened at this time.

This change adds a special case to convert a provided compression level of default to a compression level of 4, which is a reasonable default for dynamic content.
 * The test code was built assuming that brotli would be running using best compression level.  Since this is no longer the case by default, the test code has been updated to set it manually.
@Ameobea
Copy link
Contributor Author

Ameobea commented Apr 17, 2023

An alternative way of implementing this that I just considered is altering the into_async_compression function. We could create a configurable default level for each compression algorithm and then pass the algorithm type as an argument to the function.

It's a bit less hacky and allows us to close the loop for all the other algorithms as well. Will have a bigger impact though.
Lmk if you'd prefer that and I can change over to doing that instead.

@Ameobea
Copy link
Contributor Author

Ameobea commented Jun 14, 2023

@davidpdrsn bumping this; I think this is something that should def. still go in.

Lmk if you'd like this implemented a different way or something like that.

@Logarithmus
Copy link

Logarithmus commented Jun 22, 2023

@davidpdrsn Recently we discovered a sudden performance drop in our axum web API and after troubleshooting pin-pointed it to clients switching from gzip to brotli, and further investigation led us here. For now we've overriden CompressionLayer::quality to fastest, but it would be nice to merge this so other people won't suddenly encounter this issue like us.

3 months ago the dropbox/rust-brotli#93 has been created, but no reply at all. Also latest commit has been back in January. Looks like fixing this upstream won't happen. So I think you should accept this PR and create a new patch release 0.4.2, so the whole rust web ecosystem could benefit from it.

@jplatte jplatte merged commit 23de275 into tower-rs:master Jul 19, 2023
11 checks passed
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

4 participants