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

encode: Tweak compression settings #4215

Merged
merged 2 commits into from
Jun 18, 2021

Conversation

klauspost
Copy link
Contributor

zstd: Limit window sizes to 128K to keep memory in control both server and client size.
zstd: Write 0 length frames. This may be needed for compatibility.
zstd: Create fewer encoders. Small memory improvement.
gzip: Allow -2 (Huffman only) and -3 (stateless) compression modes.

zstd: Limit window sizes to 128K to keep memory in control both server and client size.
zstd: Write 0 length frames. This may be needed for compatibility.
zstd: Create fewer encoders. Small memory improvement.
gzip: Allow -2 (Huffman only) and -3 (stateless) compression modes.
@CLAassistant
Copy link

CLAassistant commented Jun 18, 2021

CLA assistant check
All committers have signed the CLA.

@francislavoie francislavoie changed the title Tweak compression settings encode: Tweak compression settings Jun 18, 2021
@francislavoie francislavoie added the under review 🧐 Review is pending before merging label Jun 18, 2021
@francislavoie francislavoie added this to the v2.4.4 milestone Jun 18, 2021
francislavoie
francislavoie previously approved these changes Jun 18, 2021
Update docs.

Co-authored-by: Francis Lavoie <lavofr@gmail.com>
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Ah, thanks. Always a pleasure to accept your contributions. Appreciate the work you do on your compress libs!

@mholt mholt merged commit 69c9144 into caddyserver:master Jun 18, 2021
@mholt mholt removed the under review 🧐 Review is pending before merging label Jun 18, 2021
@polarathene
Copy link

zstd: Limit window sizes to 128K to keep memory in control both server and client size.

Is there any citation for clients that have trouble with the 8MB window size?

Since Chrome landed stable support for zstd content encoding in March 2024 releases of Chrome, they've been following RFC 8878 guidance and limiting the allowed window size to 8MB: #6140 (comment)

Their brotli support has a 16MB window, so for browsers there's likely no concern about the 8MB window limit? Where is the 128KB limit needed? Some embedded devices? Perhaps it'd be better to clarify/document that, potentially allowing for this to be configurable in some manner?

zstd: Create fewer encoders. Small memory improvement.

Likewise it could be useful to support more than a single thread, although I guess larger static content could now be supported with pre-compressed zstd assets instead. This doesn't affect the client decoder right? Is the memory improvement likely to make that much of an improvement on the server at the expense of reduced encoding speed?


Granted that for many small files it's unlikely to matter much adjusting these. Just curious about these fixed settings and decisions behind them.

@klauspost klauspost deleted the compression-tweaks branch April 16, 2024 07:34
@klauspost
Copy link
Contributor Author

klauspost commented Apr 16, 2024

@polarathene

Where is the 128KB limit needed?

It is not needed - but you don't gain much from the additional window, so it is just to avoid mostly needless memory use, since most encodes will be small anyway.

Likewise it could be useful to support more than a single thread

Likewise it is pointless in this context. There is currently limited concurrency for streaming encodes anyway - and it will just add overhead.

Smooth operation should go ahead of "complete efficiency".

@mholt
Copy link
Member

mholt commented Apr 16, 2024

Thanks for chiming in @klauspost ! Your info has been very helpful.

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.

5 participants