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

Brotli compression Support #476

Closed
alexander-schranz opened this issue Jan 15, 2024 · 13 comments · Fixed by #524
Closed

Brotli compression Support #476

alexander-schranz opened this issue Jan 15, 2024 · 13 comments · Fixed by #524

Comments

@alexander-schranz
Copy link
Contributor

alexander-schranz commented Jan 15, 2024

I did see on the awesome FrankenPHP website that FrankenPHP supports beside gzip also zstandard / zstd. The zstd is not yet very supported by browsers: https://caniuse.com/zstd but did you think about adding also support to brotli which is a little bit more supported: https://caniuse.com/brotli as an alternative to Gzip.

The offical c lib is: https://github.com/google/brotli

@alexander-schranz alexander-schranz changed the title Broetli compression Support Brotli compression Support Jan 15, 2024
@dunglas
Copy link
Owner

dunglas commented Jan 15, 2024

This isn't currently supported by Caddy (caddyserver/caddy#5194), but this could be added to FrankenPHP because we already have the infrastructure in place for cgo.

The best way to achieve this would be to create a dedicated Caddy plugin using the C library, then measure the potential gains, and finally include it in the FrankenPHP distribution if it is worth it.

cc @mholt @francislavoie @mohammed90

@nickchomey
Copy link

My impression here is that this shouldn't be a high priority for Frankenphp. Gzip is already pretty good at reducing egress bandwidth (which is often unlimited with good hosts) and if you use something like Cloudflare, they'll automatically convert gzip to brotli when sending to the client (which is where bandwidth and file size really matters). So having brotli from the server is more of a "nice to have"

@dunglas
Copy link
Owner

dunglas commented Jan 15, 2024

@nickchomey on the other hand, I'm pretty sure that it's an easy pick!

@nickchomey
Copy link

Ah ok. I was under the impression that it would be more difficult. I'll leave it to the experts!

@mholt
Copy link
Contributor

mholt commented Jan 17, 2024

Yeah, we don't typically support cgo plugins in Caddy for numerous reasons, but with FrankenPHP it's already using C ... nothing in Caddy forbids the use of cgo, strictly speaking. (Hence why FrankenPHP is possible.) You could write a brotli plugin that uses cgo. So if it's worth it to you, go for it! :)

@dunglas
Copy link
Owner

dunglas commented Jan 17, 2024

I'm on it!

@jjsaunier
Copy link

jjsaunier commented Jan 17, 2024

to add on @nickchomey brotli compression is CPU intensive and should be avoided for "on fly" compression use case, it burns the CPU and really degrades performance, so it promotes a potential miss-usage ending in "frakenphp poor performance" (and probably open a DDoS door) Its good to serve pre-compressed assets from CDN/fs (decompression is performant) - may be a built-in fs cache is something to consider (but it assume it's static content, no dynamic HTML etc - so it's not perfect either)

@mohammed90
Copy link

may be a built-in fs cache is something to consider (but it assume it's static content, no dynamic HTML etc

For this use case, Caddy already supports precompressed side-car files as part of the file_server handler.

@dunglas
Copy link
Owner

dunglas commented Jan 17, 2024

@withinboredom
Copy link
Collaborator

I'd be curious how the impact of stack switching from go/c/go affects performance here (which I assume has to be done nearly per-packet? no idea how this low of the stack works.)

@dunglas
Copy link
Owner

dunglas commented Jan 17, 2024

@withinboredom We really need an easy-to-use benchmark to test the impact of such changes!

@francislavoie
Copy link
Contributor

Hmm, isn't it risky to enable it by default? Should there be some benchmarks first to make sure it doesn't bring a server to its knees given a reasonable amount of traffic?

@dunglas
Copy link
Owner

dunglas commented Jan 30, 2024

@francislavoie I have some other features to finish and merge first, but I plan to do a benchmark before the release (don't hesitate if you want to run one).

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 a pull request may close this issue.

8 participants