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

Support multiple algorithms; support quality factors #7

Merged
merged 4 commits into from
Oct 5, 2020

Conversation

peter-slovak
Copy link
Contributor

This PR makes it possible to specify multiple compression algorithms in the COMPRESS_ALGORITHM option - either in string format (for backwards compatibility) or in a more Pythonic list-of-strings format.

Since we can use multiple algorithms now, we should also let the client let us know about their preference. This is done by implementing the "quality factor" associated with each algorithm in the Accept-Encoding header: https://developer.mozilla.org/en-US/docs/Glossary/Quality_Values

The main incentive is supporting a wide spectrum of clients, some of which are able to do br compression, but those that are not can still benefit from a fallback like gzip.

Tests and docs have been added/modified accordingly.

@peter-slovak peter-slovak changed the title Support multiple algorithms; support quality factors [WIP] Support multiple algorithms; support quality factors Sep 28, 2020
@peter-slovak peter-slovak changed the title [WIP] Support multiple algorithms; support quality factors Support multiple algorithms; support quality factors Sep 28, 2020
@KelSolaar
Copy link
Member

Nice and LGTM overall! I haven't tested the code though and will let @alexprengere chime in!

@alexprengere
Copy link
Collaborator

Really high quality PR, thanks! I am going to merge this, and probably make a release in a few days, once I have run some benchmarks. My only tiny concern is performance, because inevitably _choose_compress_algorithm is executed for each request.

Since most browsers will have a similar Accept-Encoding, and enabled_algorithms is not expected to change, I think it would be reasonable to make _choose_compress_algorithm a function that can be cached with functools.lru_cache (or something custom). But still, I have to measure the actual overhead on a real world use case 😉

@peter-slovak
Copy link
Contributor Author

Thanks 🙂 Yeah I was also considering the performance penalty of _choose_compress_algorithm(), but came to a conclusion that both the server-side number of enabled algorithms and the client-requested number of algorithms will be very small, resulting in at most 2-3 for loops overall until we find (or don't find) a match.

What could be maybe optimized is the per-request server_algo_set = set(self.enabled_algorithms) transofmation, we could save that as an attribute at instance construction, but again, I don't expect the number of server-supported algos to reach even two-digit numbers.

But by all means, please do conduct the benchmarks and let's make a data-backed decision 👍

@alexprengere
Copy link
Collaborator

So after a few tests, _choose_compress_algorithm runs in about 10µs with a value of Accept-Encoding set to "gzip, deflate, br", which is the default used by Chrome/Firefox on HTTPS connections.
I guess this is small enough to not bother caching the output, since most page load time will be at minimum 10ms due to network latency, so 1000 times more. Clearly this PR is not slowing things down 😉

@alexprengere alexprengere merged commit dcd96d6 into colour-science:master Oct 5, 2020
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.

3 participants