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

fix(ext/http): Use brotli compression params #19758

Merged
merged 1 commit into from
Jul 7, 2023

Conversation

mmastrac
Copy link
Contributor

@mmastrac mmastrac commented Jul 7, 2023

Fixes #19737 by adding brotli compression parameters.

Time after:

Accept-Encoding: gzip:

real    0m0.214s
user    0m0.005s
sys     0m0.013s

Accept-Encoding: br:

Before:

real    0m10.303s
user    0m0.005s
sys     0m0.010s

After:

real    0m0.127s
user    0m0.006s
sys     0m0.014s

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, is there a test that could be added to avoid regressions in the future?

@mmastrac
Copy link
Contributor Author

mmastrac commented Jul 7, 2023

LGTM, is there a test that could be added to avoid regressions in the future?

We might be able to create a timing comparison test between GZip and Brotli but I worry it might be flaky. Maybe a heuristic like "does this compress within 10x the wall-clock time of GZip", but that could be a challenge to find a good set of parameters that detect the problem and are sensitive enough to trigger if this code gets lost. I wouldn't recommend using a fixed wall-clock timeout for this because that will almost certainly trigger too often.

I think I should punt on this one because it's probably a bit too much work.

@rajsite
Copy link

rajsite commented Jul 14, 2023

@mmastrac know how to check if this change is on Deno Deploy yet? I think it may be the root cause of denoland/deploy_feedback#434

(Also not sure if you need to redeploy for updates, etc)

@lucab
Copy link
Contributor

lucab commented Jul 14, 2023

@rajsite I answered directly there. This fix was not on Deno Deploy at the time, but it is applied there too right now.

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.

Deno.serv Brotli compression incredibly slow
4 participants