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 async_compression::Level controls, default to Level::Default #10

Merged
merged 7 commits into from
Jun 15, 2023

Conversation

JonLatane
Copy link
Contributor

@JonLatane JonLatane commented Jun 14, 2023

This builds on #8 slightly and adds async_compression::Level controls to Compress as well as the Compression and CachedCompression fairings. It also effectively switches all of the above to default to Level::Default instead of Level::Best, as I found its worst-case (first load) to average-case response times to be better for at least the CPUs of the servers I'm using. Happy to update the default either way, though I would argue making the default Level::Default has some clear merits 😄

(I'd previously mentioned trying to do progressive caching in the background but the Rocket Body lifetime makes this problematic... This works well in the meantime though!)

@Ameobea
Copy link
Owner

Ameobea commented Jun 14, 2023

Thank you so much for this!! It definitely shouldn't be compressing at max level by default for dynamic data; that was an oversight on my part.

The only thing I'd tweak with this is actually overriding async-compression's Level::Default level for brotli. Internally, the default compression maps to Brotli's "Best" level due to the way they wrap the internal brotli crate. It's an issue I hit with tower-http and I even up a PR to fix it there: tower-rs/tower-http#356

So anyway, I think that we should add some logic to map a compression level of Default to an explicitly specified value of 4 if the compression type is brotli. We can still expose it to the user as default, and we'll just swap the default to a 4 before passing it into async_compression.

Gzip is fine and defaults to a more reasonable value. It's much faster to compress than brotli as well.

Thanks again for this!

 * The library used internally by `async-compression` for brotli compression sets a level of best when default is requested.  This is unsuitable for dynamic data, so we override it with a level of 4.
 * Add helper method to construct compression fairing with specified compression level
 * Update README
@Ameobea
Copy link
Owner

Ameobea commented Jun 15, 2023

I went ahead and updated this PR with those changes. I'll tag this off as 0.5.0 and publish once it's merged.

Ty again for implementing this!

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.

2 participants