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

feat: Support additional compression algorithms #4282

Draft
wants to merge 3 commits into
base: mealie-next
Choose a base branch
from

Conversation

HowRuck
Copy link

@HowRuck HowRuck commented Sep 28, 2024

What type of PR is this?

  • feature

What this PR does / why we need it:

This PR introduces support for newer compression algorithms (Brotli and Zstd) in addition to the currently supported Gzip. By dynamically selecting the best compression algorithm based on the Accept-Encodings header from the request, this change aims to improve overall performance, particularly in low bandwidth scenarios

Which issue(s) this PR fixes:

Not really any issue per se, it just improves performance a little.

Special notes for your reviewer:

I've done a quick scan over the starlette-compress library used here, as it is still pretty new and very small. I haven't found any issues with it. It literally just chooses the best compression and compresses the content using standard libraries. Feel free to have another look around, in case I missed anything :-)

Testing

I've been using this change in my private mealie instance for the last week without any issues.

@hay-kot
Copy link
Collaborator

hay-kot commented Sep 28, 2024

Do you have any benchmarks for the performance difference here? If we’re going to add a dependency id be interested to know if the tradeoff is worth it.

In my mind setting up your reverse proxy to handle compression might be a better bet.

@HowRuck
Copy link
Author

HowRuck commented Sep 29, 2024

Do you have any benchmarks for the performance difference here? If we’re going to add a dependency id be interested to know if the tradeoff is worth it.

I've done some very basic testing, and so far I am coming to the conclusion that adding a new dependency is probably not worth it (benefits are there but relatively minor). The solution below, however, might be an interesting alternative 🤔 
I'll do some more testing in the coming days if needed. 

In my mind setting up your reverse proxy to handle compression might be a better bet.

That would most definitely be the best bet 👍 

The only problem with this is that traefik, or most other reverse proxies, don't compress data that is already compressed. A good solution for this would be to add an environment variable (defaulting to true) that controls whether gzip compression is active or not. If not, compression can then be handled by the reverse proxy.

This would enable the use of modern compression algorithms without adding additional dependencies, and would also allow a nice separation of concerns while increasing flexibility. 

What would you think of this approach? 🤔

@hay-kot
Copy link
Collaborator

hay-kot commented Oct 12, 2024

A good solution for this would be to add an environment variable (defaulting to true) that controls whether gzip compression is active or not. If not, compression can then be handled by the reverse proxy.

This sounds good to me, Sorry for the delay!

@michael-genson michael-genson marked this pull request as draft November 1, 2024 21:43
@michael-genson
Copy link
Collaborator

Converted to draft since it looks like the changes were reverted. Once it's ready feel free to switch it back!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants