encode/zstd: support compressing with a trained dictionary - #8075
sorena-paydar wants to merge 1 commit into
Conversation
Adds a `dictionary` option to the zstd encoder that loads a dictionary produced by `zstd --train` and compresses responses against it. Small payloads are where this matters. A 108 byte JSON body grows to 121 bytes under plain zstd, since the frame overhead exceeds what there is to compress, but falls to 39 bytes against a dictionary trained on similar responses. The dictionary is not carried in the frame, so only clients holding the same dictionary can decode the response. That makes this useful between services that share a dictionary out of band, and unsuitable for browser traffic, which needs the Available-Dictionary negotiation from Compression Dictionary Transport. That negotiation cannot be expressed here: Encoding.NewEncoder() takes no request, so the encoder cannot vary per client without changing a shared interface. The dictionary is read and validated when the module is provisioned rather than per request, because NewEncoder() has no way to report an error and passing raw sample data instead of a trained dictionary is an easy mistake to make. The error names `zstd --train` so it is clear what the file should be. Signed-off-by: Sorena Paydar <sorenapaydar81@gmail.com>
|
|
|
You (your AI) do not understand compression and dictionaries enough to make this change. |
|
Fails to comply with RFC 8878 and 9842 |
|
Very helpful comment, Thanks a lot !! |
|
As stated. RFC 8878 and 9842. |
|
I am more than happy to review a change but do not submit AI-written code that you do not understand the implications of to me and then expect me to explain to you why it is wrong. Be respectful in the future. For such a large change, one would generally wish to discuss further in the issue rather then submit a PR that does not work. |
Implements the dictionary option discussed in #6204.
@mholt wrote there:
This is that option, scoped to a single dictionary loaded from disk — not the full Compression Dictionary Transport negotiation the issue opens with. See Limitations below for why that part can't be done from an encoder module today.
What it does
Adds a
dictionaryoption to the zstd encoder, taking a path to a dictionary produced byzstd --train:JSON:
{"encodings": {"zstd": {"dictionary": "/etc/caddy/api.dict"}}, "handler": "encode"}Why it helps
Small, structurally similar payloads are the case that plain zstd cannot do much with, because frame overhead swamps the content. Measured on a 108-byte JSON body against a dictionary trained on 60 similar responses:
encode zstdencode zstd+ dictionaryThat is the shape of a lot of API traffic behind a reverse proxy.
Limitations, and a question
A dictionary frame is only decodable by a client holding that dictionary. The dictionary is not carried in the frame; a decoder without it fails with
unknown dictionaryrather than degrading. So this option is for traffic between parties that share a dictionary out of band — service-to-service, internal API clients — and is not safe to turn on for browser traffic.The web-facing answer to that is Compression Dictionary Transport: the client advertises
Available-Dictionary: :<sha-256>:and the server replies withContent-Encoding: dczonly when it holds the matching dictionary. That cannot be implemented from an encoder module as things stand, because the interface is:NewEncoder()receives no request, so the encoder cannot vary per client or inspectAvailable-Dictionarywithout changing a shared interface. That felt like the wrong thing to fold into this PR unprompted.So my question: is a plain
dictionaryoption the right surface, or would you rather this were gated behind a distinct encoding name so it can never be selected by a client that merely sentAccept-Encoding: zstd? I'm happy to rework it either way, including taking on the request-aware interface change if you want the negotiation properly.The doc comment on the field states the constraint so it is visible from the JSON docs.
Implementation notes
The dictionary is read and validated in
Provision, not per request, for two reasons:NewEncoder()has no way to report an error, and passing raw sample data where a trained dictionary belongs is an easy mistake —zstd.WithEncoderDictrejects it withmagic number mismatch, which is not obvious. The error nameszstd --traininstead:Testing
New
modules/caddyhttp/encode/zstd/zstd_test.gocovers:DictionaryIDProvisionrejecting a missing file and a file that is not a trained dictionary, with the error namingzstd --trainPlus a Caddyfile adapter test at
caddytest/integration/caddyfile_adapt/encode_zstd_dictionary.caddyfiletest.I checked the tests fail without the change, which caught a weak one: the round-trip test originally passed even with the dictionary wiring disabled, because a decoder holding dictionaries also reads plain frames. The
DictionaryIDassertion is what fixed it. With the wiring disabled three tests now fail; with it, all pass.testdata/sample.dictis a 2.8 KB dictionary trained on generated JSON samples;testdata/invalid.dictis plain text, for the rejection test.Assistance Disclosure
AI-assisted. I directed the design and the scoping decisions, and verified the behaviour rather than taking it on trust: the compression figures above are measured, the round-trip and frame-header assertions are in the test file, and I confirmed the tests fail when the implementation is disabled. The
Available-Dictionarylimitation above came from reading theEncodinginterface and finding it takes no request.