Add lz4 and zstd compression support to the CLFUS RAM cache - #13257
Conversation
bryancall
left a comment
bryancall
left a comment
There was a problem hiding this comment.
Took a pass over this — really nice change. The liblzma lzma_stream_buffer_bound() fix is a real correctness improvement, the per-thread zstd context design is clean, and adding compression-integrity tests where there were none is great. A handful of improvement suggestions inline; none are blockers.
The one I'd most encourage looking at is the sticky-null behavior of the thread-local zstd context: a one-time allocation failure silently degrades that thread to uncompressed forever (and evicts valid entries on read) with no log or metric. Details inline.
|
Since I know you probably cant get to the CI output @phongn , here's the failure. It's in the clfus catch test: |
|
Related PR to add zstd and lz4 to our CI hosts: apache/trafficserver-ci#441 |
5d3ea03 to
aa2a6a8
Compare
cmcfarlen
left a comment
cmcfarlen
left a comment
There was a problem hiding this comment.
I think this looks good. I have a couple of concerns.
- Will this build if LZ4 or ZSTD are not found?
- I don't think CLFUS is used due to other concerns with the technique. Does this PR resolve those?
requesting changes just to get answers to the questions.
|
There was a problem hiding this comment.
The threads from my June review are all resolved and CI is green, so the only thing blocking this now is the merge conflict.
Please rebase onto master and I will do the full review. With 22 files touched, including CMakeLists.txt, both Dockerfiles and the new FindLZ4.cmake / FindZSTD.cmake modules, I would rather review the rebased tree than a version that cannot land.
Marking this as request changes until it is mergeable again. That is not a comment on the code itself, which looked good in June.
RamCacheCLFUS allocated its hash table, seen filter, and entries but had no destructor, so destroying an instance leaked them. LeakSanitizer flagged this once the new unit test started creating and destroying instances. Add a destructor that releases each entry's data, returns the entries to the allocator, and frees the table and seen filter. The synchronous RAM cache test never calls TEST_DONE(), so the event threads kept running as the process tore down static state at exit and an ET_NET thread incremented the freed Metrics singleton (heap-use-after-free). Shut the event system down from the shared cache test harness at end of run so the threads stop first. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bryancall
left a comment
bryancall
left a comment
There was a problem hiding this comment.
❌ Request changes
Thanks for the rebase. The June threads are all addressed and the rebased tree looks good, but one thing outside the diff blocks the feature from working as shipped.
Blocking: the records range check still rejects 4 and 5
src/records/RecordsConfig.cc line 889 defines proxy.config.cache.ram_cache.compress with the validity pattern [0-3]. RecYAMLDecoder runs that check on load, logs a validity warning on failure, and uses the default of 0. So setting 4 or 5 in records.yaml, as the docs now tell people to, silently leaves compression off. The unit test does not catch it because it writes cache_config_ram_cache_compress directly. Changing the pattern to [0-5] is the fix. It would be worth a one-line autest or gold-test config that sets the record to 5 and checks traffic_layout or a startup log line, so the range check cannot drift again.
Fixed: sticky-null zstd context
The one-time warning in zstd_cctx() / zstd_dctx() covers what I asked for. One non-blocking refinement: when zstd_cctx() returns null in compress_entries(), failed = true lands on Lfailed, which sets incompressible on the entry. That records a thread-level allocation condition as a permanent property of the data, which is the opposite of the (correct) choice you made in get(). goto Lcontinue there would leave the entry eligible for a later pass. A ram_cache.compress.failure counter next to the new decompress one would also make this diagnosable without log access.
Fixed: decode failures are now visible
The throttled warning plus the global and per-volume ram_cache.decompress.failure counters, with the null-context case treated as a miss, is exactly right. Non-blocking: the codec's own error is dropped on the floor in every branch. Capturing ZSTD_getErrorName() or the negative lz4 return into a local and printing it in the Lfailed warning would tell an operator whether a nonzero counter means a corrupted frame or a bookkeeping bug in len.
Fixed: test proves compression happened
256 KB payload and size_after < size_before for every non-NONE backend. Two non-blocking gaps: the incompressible test does not assert size_after == size_before, so a regression that stored a raw copy instead of marking the entry would still pass, and the single-byte case runs only under CACHE_COMPRESSION_NONE, so lz4 and zstd on tiny input (both emit frames larger than 1 byte) are untested. Parametrizing it over compression_cases() like the others would close that. Also, nothing discriminates the liblzma lzma_stream_buffer_bound change; the old code failed the encode and the new code stores raw, and both read back as NONE. Fine to leave, but the description should call it a robustness fix rather than a memory fix.
Fixed: static_asserts
Two non-blocking notes. static_assert(CACHE_COMPRESSION_ZSTD < (1 << 3)) sits inside #ifdef HAVE_ZSTD_H in the .cc, so a build without zstd never evaluates it; it belongs in RamCacheCLFUS.h directly under the bitfield, unconditional. The six pairwise asserts in Cache.h are each true by construction and would not fire if someone added CACHE_COMPRESSION_FOO 6 without the matching enumerator; one static_assert(RAM_HIT_LAST_ENTRY == CACHE_COMPRESSION_ZSTD + 2) would.
Non-blocking: the new destructor and the scheduled compressor
~RamCacheCLFUS() frees the entries, buckets and seen filter, but init() schedules a RamCacheCLFUSCompressor holding a raw back-pointer that nothing cancels. Production never destroys these objects, and the test sidesteps it by initializing with compression off, so this is latent. Either keep the Event * and cancel it in the destructor, or say in a comment that the destructor is only safe when compression was never scheduled. The comment in test_RamCacheCompressEntries.cc saying the policy has no destructor is now stale.
Not verified by running it
The new test cannot be run at the base commit since it depends on symbols this patch adds, so there is no red-without-fix result to report. CI is green on the rebased head.
Allow the new codecs to actually be configured. The validity pattern for proxy.config.cache.ram_cache.compress was still [0-3], so RecYAMLDecoder rejected 4 and 5 at load time, logged a validity warning and fell back to the default of 0 -- leaving compression off for exactly the two backends the docs now recommend. Widen it to [0-5] and add a records unit test that walks every CACHE_COMPRESSION_* value against the record's own check and pattern, so the range cannot drift behind the enum again. Distinguish a missing zstd context from incompressible data. A null ZSTD_CCtx is a thread/allocator condition, so recording it as the entry's permanent incompressible flag was the opposite of the choice already made in get(). The entry is now left eligible for a later pass. Because that failure is sticky and the compressor event is pinned to one ET_TASK thread, leaving entries eligible alone would turn a one-time allocation failure into a walk over most of the RAM cache every second -- dropping and retaking the stripe lock and allocating a compressBound()-sized buffer per entry, only to fail each time -- so compress_entries() now skips the whole pass on a thread with no context. Real compression failures and skipped passes increment a new ram_cache.compress.failure counter (global and per-volume) so they are diagnosable without log access; objects that merely did not shrink enough are deliberately not counted, since that is the ordinary outcome for already-compressed content. Report the codec's own error on a decompression failure. Every branch dropped it, so a nonzero decompress.failure counter could not tell a corrupt frame from a bookkeeping error in e->len. The throttled warning now carries ZSTD_getErrorName(), zError(), or the codec's numeric return. Move the bitfield static_assert next to the field it guards, in RamCacheCLFUS.h and unconditional; inside #ifdef HAVE_ZSTD_H it was never evaluated by a build without zstd. Add one count assert in Cache.h that fires if a CACHE_COMPRESSION_* is added without its RAM_HIT_COMPRESS_* enumerator; the pairwise asserts are kept because they catch a reordering of either sequence, which the count assert does not. Strengthen the compression tests: the incompressible case now asserts the entry's footprint is unchanged (the 256 KB payload is a power of two, so there is no padding for the pass to legitimately reclaim), and the single-byte case is parametrized over every backend instead of running only under CACHE_COMPRESSION_NONE. Document why ~RamCacheCLFUS() is only safe for a cache that never scheduled the background compressor, and correct the now-stale comment in test_RamCacheCompressEntries.cc. Cancelling the event is not enough to fix this: the continuation carries no mutex, so it can run compress_entries() concurrently with the destructor. Making that safe means giving the compressor the stripe mutex and requiring the destructor to hold it, which is not worth it while production never destroys a RamCacheCLFUS. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Thanks — good catch on the range check, that one would have shipped a feature nobody could turn on. All six points are addressed in 421b518. Details, including two places where I went past what you asked and one where I deliberately didn't. Blocking: records range check — fixed
For the drift guard I used a records unit test rather than an autest: TEST_CASE("ram_cache.compress accepts every compression backend", "[librecords][RecUtils]")
{
const auto *record = GetRecordElementByName("proxy.config.cache.ram_cache.compress");
...
for (int i = CACHE_COMPRESSION_NONE; i <= CACHE_COMPRESSION_ZSTD; i++) {
INFO("CACHE_COMPRESSION_* value: " << i);
REQUIRE(RecordValidityCheck(std::to_string(i).c_str(), record->check, record->regex));
}
REQUIRE_FALSE(RecordValidityCheck(std::to_string(CACHE_COMPRESSION_ZSTD + 1).c_str(), record->check, record->regex));
}Two reasons over a gold test: it iterates the real This one I did verify red-without-fix: reverted the pattern to Sticky-null zstd context — fixed, and the fix is bigger than the one you suggested
So #ifdef HAVE_ZSTD_H
if (cache_config_ram_cache_compress == CACHE_COMPRESSION_ZSTD && zstd_cctx() == nullptr) {
ts::Metrics::Counter::increment(cache_rsb.ram_cache_compress_failures);
ts::Metrics::Counter::increment(stripe->cache_vol->vol_rsb.ram_cache_compress_failures);
return;
}
#endifEntries are untouched, so they stay eligible exactly as you wanted; one counter increment per skipped pass keeps it visible without flooding. The per-entry
Decode failures — codec error now reportedAll seven One wording change while I was in there: the Tests — strengthened, with one honest limitationIncompressible case asserts On the footprint assertion — it is weaker than it looks and I'd rather say so than let it read as more than it is. It catches "stored the expanded compressed blob", but it cannot distinguish "marked incompressible and left alone" from "re-stored a raw copy", because the 256 KB payload is a power of two and carries no buffer padding, so both land on the same You're also right that nothing discriminates the liblzma change, and for a stronger reason than the test being weak: static_asserts — fixed, with one deviationBitfield assert moved to Added your count assert. I did not remove the six pairwise ones, though: they catch a reordering of either sequence (reorder the Destructor and the scheduled compressor — took the comment optionI went with documenting the constraint rather than cancelling, because cancelling isn't sufficient and I didn't want to ship something that looks safe and isn't. If you'd rather have the mutex change, I'd prefer it as its own PR against the shared-compression refactor, where the compressor continuation moves anyway. VerificationThree configurations, all green: default, Worth noting for the record: this host has lz4 1.9.3 and zstd 1.5.5, so all six cases actually ran (79 assertions, versus the 45 you'd have seen in the Rocky CI log where lz4 was missing and the test Deliberately not in this PRThe libz and liblzma decode paths check only the library return code, never that the output length equals |
clang-analyzer flagged the "no detail" initializer for codec_error as a dead store, correctly: every path to Lfailed assigned a real detail string first, so the sentinel was never read. Dropping just the initializer would have left a genuine uninitialized read the first time someone added a goto Lfailed without setting it. The sentinel and the function-scope char buffer only existed because the detail string had to survive a goto, so report the failure at each site instead. note_decompress_failure() now carries the throttled warning and both counters, each branch formats its detail in its own scope, and Lfailed is back to freeing the buffer and destroying the entry. Behavior is unchanged: one process-wide throttler, and the entry is still intact when the warning reads its fields. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Follow-up in aaf29f1 for the two CI failures on 421b518. Clang-Analyzer — fixed
Correct, and the sentinel was load-bearing for nothing: all seven paths to Instead I removed the reason the sentinel existed. The if (l != rc) {
char detail[128];
snprintf(detail, sizeof(detail), "LZ4_decompress_safe returned %d, expected %d", rc, l);
note_decompress_failure(stripe, key, e, detail);
goto Lfailed;
}A new file-local This supersedes the Verified locally with AuTest 2of4 — not this PRThe only failing test in that shard is That is #13679, opened today: the queue precondition races a TLS handshake on a 0.3s timer. The shard was also heavily loaded on this run (worker 1 took ~1456s), which is the condition that issue describes. Nothing in this PR reaches the SNI rate limiter — the diff is RAM cache compression, a records validity pattern, and two counters — and every other shard passed, as did all nine platform builds. I'd rather not rebase just to reroll the dice on a known flake, so unless you want it rerun for a clean board, the new push should sort it on its own. |
None of these change RAM cache behavior. The zstd config-mode alias block could never fire, the decompression warning reimplemented the site-wide log throttling it should have called, and the compress.failure doc claimed a per-entry count that the pass-level skip does not produce. The rpm spec and the contrib images also lacked lz4 and zstd, so a documented compress value would be Fatal at startup on those builds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Several were mine and hid real behavior: compress.failure counted skipped passes as entry failures, a transient codec allocation error marked an entry permanently incompressible, the destructor skipped the byte accounting _destroy() does, and the new destructor left the copy operations implicit. Codec validation now happens once where the value is read instead of twice after the compressor is already scheduled, and the Find modules are the tree's own rather than a BSD import. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Correcting something I told you in my earlier reply, since the claim is wrong and it is load-bearing for a decision you may make later. On the destructor thread, I wrote:
The stripe mutex is exactly the wrong mutex for this, and not for the reason I gave. The right shape is the one used elsewhere in the tree: give Worth being explicit about why the cancel is needed at all, which I was vague about: I have not made that change here — it is a behavioural change to production scheduling and belongs with the shared-compression refactor, where the compressor continuation moves anyway. I have queued it there, and corrected the comment in |
bryancall
left a comment
bryancall
left a comment
There was a problem hiding this comment.
✅ Approving. The blocker from my last review is fixed and it now has a test that catches it.
I rebuilt and ran this rather than reading it: configured 132ddc21 on macOS/clang with lz4 1.10.0 and zstd 1.5.7, ran RamCacheCLFUS (79 assertions, all six backends) and test_records "[RecUtils]", then reconfigured with both CMAKE_DISABLE_FIND_PACKAGE_* set and reran. Everything I call verified below was run.
I reverted src/records/RecordsConfig.cc:889 to [0-3], rebuilt, and got test_RecUtils.cc:235: FAILED ... CACHE_COMPRESSION_* value: 4. Restored: 89 assertions green. The upper bound is 5 and CACHE_COMPRESSION_ZSTD is 5, so there is no off-by-one hiding zstd.
All of my earlier asks have landed. Two worth calling out as better than what I asked for: the null-cctx handling became a pass-level skip at RamCacheCLFUS.cc:549 plus a transient flag routing allocation errors to Lcontinue, and the error-text capture covers all five codecs rather than the new two. On the incompressible case you used size_after <= size_before instead of == with a comment about padding: I ran it and it is exactly equal at 262280 both sides, your reasoning is right, leave it.
Also verified, since these are the things most likely to be wrong in a change like this: the zstd contexts are thread_local std::unique_ptr with ZSTD_freeCCtx/ZSTD_freeDCtx deleters so they release at thread exit rather than leaking, a per-call ZSTD_CCtx_reset() is genuinely unnecessary because ZSTD_compress2() starts a fresh frame and the level is sticky, the read path treats every codec failure as a miss with the entry intact and no double free, and buffer sizing uses LZ4_compressBound() / ZSTD_compressBound() with the codec's own return stored rather than the bound.
Non-blocking
1. Deleting the zstd config-mode shim is a regression against master
a8a1c45a removed the alias block because it "could never fire". It does fire.
With CMAKE_FIND_PACKAGE_PREFER_CONFIG=ON, find_package(ZSTD 1.4.0) at CMakeLists.txt:496 resolves through zstd's own config package instead of cmake/FindZSTD.cmake. ZSTD_FOUND is set so HAVE_ZSTD_H is true, but the exported target is zstd::libzstd_shared, not zstd::zstd. cmake/FindZSTD.cmake:70 never runs, nothing creates the alias, and the four places linking zstd::zstd fail at generate time (src/iocore/cache/CMakeLists.txt:66, src/iocore/net/CMakeLists.txt:137, src/traffic_layout/CMakeLists.txt:35, plugins/compress/CMakeLists.txt:29).
Reproduced standalone against this tree's cmake/ directory:
-- PREFER_CONFIG ZSTD_FOUND=1 ZSTD_CONFIG=/opt/homebrew/lib/cmake/zstd/zstdConfig.cmake
-- TARGET zstd::libzstd_shared EXISTS
-- *** zstd::zstd MISSING
CMake Error at CMakeLists.txt:10 (target_link_libraries):
zstd::zstd
It is a non-default setting and no ATS CI job sets it, which is why CI is green, so I am not blocking. I would still fix it before merge: it is four lines after find_package(ZSTD 1.4.0), aliasing whichever of zstd::libzstd_shared / zstd::libzstd_static / zstd::libzstd exists.
The description still says the alias shim is kept for CMAKE_FIND_PACKAGE_PREFER_CONFIG builds. Please either restore it or drop that sentence, and fix the description before re-requesting review, since the next reviewer reads it alongside the diff.
2. Nothing tests the decompress failure paths
note_decompress_failure() at RamCacheCLFUS.cc:295 and its five call sites are the visible part of this change on the read path, and no test reaches any of them. I proved it: replacing the ZSTD_isError(ll) check at line 398 and the l != rc check at line 377 with if (false) still gives "All tests passed (79 assertions in 4 test cases)". Deleting both codecs' error detection is invisible to the suite.
A corrupted-frame case closes it: store, run compress_entries(), flip a byte in the stored blob, get(), then assert a miss, an intact entry, and ram_cache_decompress_failures == 1. The entries are private so it needs a small test hook or a friend declaration. Worth it, because the counter and the warning are the operator-facing half of this feature and right now they are only proven to compile.
3. A real round-trip failure will report as a stringification crash
CHECK(r.out == payload) at test_RamCacheCLFUS.cc:206, 238 and 265 compares two 256 KB std::vector<char>. Catch2 stringifies both operands to report an assertion and on this payload that throws. ./RamCacheCLFUS -s gives due to unexpected exception with messages: compression backend: zstd / basic_string.
Green without -s, so not a failing test today. The problem is the day a codec genuinely breaks the round trip and CI shows unexpected exception: basic_string instead of which backend and which byte. Compare into a bool first, or assert on sizes plus memcmp.
4. The lz4 version floor is below the API the tree uses
CMakeLists.txt:501 sets 1.7.0, but src/traffic_layout/info.cc:273 calls LZ4_versionString(), which lz4.h documents as requiring v1.7.5+. Against 1.7.0 through 1.7.4 the find module reports success and traffic_layout then fails to link. Raise the floor to 1.7.5, or print only LZ4_VERSION_STRING. The zstd 1.4.0 floor is correctly enforced: find_package(ZSTD 99.0.0) gives Could NOT find ZSTD: Found unsuitable version "1.5.7".
5. The Fatal message does not say what to change
Cache.cc:884 and 889 say "lz4 not available for RAM cache compression". The operator hitting this just set a number in records.yaml and now has a process that will not start. Name the record and the value the way the default: case at line 893 does.
6. Smaller things
NOTICEgains one trailing blank line and no content. Drop it from the diff.ci/docker/yum/Dockerfilerenameszstd-develtolibzstd-develon a line unrelated to lz4. Probably the right Fedora name, but it is an unrelated change riding along.doc/developer-guide/cache-architecture/ram-cache.en.rstfixesuncompressibletoincompressibleand leaves "it mat be compressed" on the next line.traffic_layout info --versionsprints zstd as the runtime version and lz4 as the compile-time one with a separatelz4.run. Harmless, but similar names now mean different things.
One thing to answer before this merges
@cmcfarlen's dismissed review asked two questions. The first, whether this builds without lz4 or zstd, I have verified myself: it does, cleanly, and the test skips with a WARN rather than failing. The second, whether this addresses the other concerns with the CLFUS technique, has no answer anywhere in the thread or the description. He is a requested reviewer again, so please answer it in the PR rather than leaving my approval to carry the change past it.
On CI
All 14 checks pass on 132ddc21. One caveat worth stating: the unit test WARNs rather than failing when a backend is absent, so a green run does not by itself prove the new code was exercised anywhere in CI, and your own description notes the Fedora image still needs lz4-devel. That is why I built and ran it locally.
Restoring the config-mode alias is a fix for my own regression: with CMAKE_FIND_PACKAGE_PREFER_CONFIG the lookup resolves through zstd's config package on a case-insensitive filesystem, which exports zstd::libzstd_shared rather than zstd::zstd, so deleting the alias broke macOS builds at generate time. The new corrupted-frame test reaches the decompression failure paths, which nothing exercised before, and the round-trip checks no longer hand Catch2 two 256 KB operands to stringify, which threw instead of reporting the mismatch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks — and thank you for rebuilding it rather than reading it. All six are addressed in 1. The zstd shim — you are right, and it was my regressionI deleted it on a Linux-only check and generalised. CMake's config-mode search looks for I reproduced it here rather than taking it on faith, by simulating the config package on Linux and toggling only the filename:
So Restored as a loop over Description fixed too, and expanded — the sentence you flagged was true again once the shim came back, but it did not say why the shim exists, which is how it got deleted. It now spells out the case-insensitive filename match. 2. Decompress failure paths — test added, and it earned its placeYou were right that nothing reached them. It found more than coverage: it exercises the per-codec detail text for real, and all five produce a distinct, useful message. I re-ran your mutation. 3. The 256 KB compare — worse than green-without-
|
bryancall
left a comment
bryancall
left a comment
There was a problem hiding this comment.
Verdict: ✅ Approve
Reviewer effort: Re-review of head ebb773b8 against my approval on 132ddc21, which was dismissed. One new commit, parent is exactly 132ddc21 and the merge base is still f3c3ac9b, so this is not a rebase and the only new surface is the eight files that commit touches. I built three trees on macOS/clang with lz4 1.10.0 and zstd 1.5.7: libraries on, libraries off, and ASan. I ran RamCacheCLFUS (131 assertions), the same with -s, RamCacheCompressEntries, test_records "[RecUtils]" (89 assertions), five mutation builds, and four standalone cmake reproducers including a full-tree CMAKE_FIND_PACKAGE_PREFER_CONFIG=ON configure with and without the restored shim. Everything below marked verified was run.
I also have one correction to my own previous review to make, in item 6.
Previous review, resolved
| Ask | State | Evidence on the current head |
|---|---|---|
1. Restore the zstd::zstd config-mode shim |
Landed | CMakeLists.txt:505. Handles all three target names, and degrades with a warning instead of a generate error when the config package exports none of them. Verified end to end, both directions |
| 1b. Fix the PR description | Landed | It now says why the shim exists, which is the part that let it get deleted. Quoted below |
| 2. Test the decompress failure paths | Landed | test_RamCacheCLFUS.cc:302, parametrized over all five codecs. It asserts on the counter, not on a bare miss. My mutation now fails the suite |
| 3. A round-trip failure must report a diff, not a stringification crash | Landed | first_difference() at test_RamCacheCLFUS.cc:160, used at lines 238, 271 and 299 alongside a size check. -s is clean, and a real mismatch now names the byte |
| 4. lz4 version floor below the API the tree uses | Landed | CMakeLists.txt:524 is find_package(LZ4 1.7.5). Floor verified enforced in both directions |
5. The Fatal message should name the record and the value |
Landed | Cache.cc:879, 885, 890 and 895. All four messages now name proxy.config.cache.ram_cache.compress and print the value |
6a. Trailing blank line in NOTICE |
Landed | git diff f3c3ac9b HEAD -- NOTICE is empty. The file is byte-identical to master |
6b. Unrelated zstd-devel rename in the yum Dockerfile |
Landed | Reverted to master's spelling. The only change left on that line is lz4-devel. The author raised the EL9 package naming as a separate pre-existing bug and is right to keep it out of this PR. I did not verify the EL9 package names myself |
| 6c. "it mat be compressed" | Landed | ram-cache.en.rst:77 |
6d. lz4 compile-time versus zstd runtime under similar names |
Landed | info.cc:273 prints LZ4_versionString() and lz4.run is gone, so lz4 and zstd now mean the same thing |
| 7. cmcfarlen's second question, on the other concerns with CLFUS | Rebutted, and the rebuttal is correct | It was answered in this comment on 2026-07-08. I read it: the answer is that this PR does not resolve the CLFUS concerns, and the intent is to refactor so compression works with any algorithm. I missed it and said it was unanswered anywhere in the thread, which was wrong. My apologies |
On the description, the sentence I asked to be fixed now reads:
The
zstd::zstdalias shim is kept, and it is not dead code. WithCMAKE_FIND_PACKAGE_PREFER_CONFIGthe lookup can resolve through zstd's own config package rather than the module: CMake searches forZSTDConfig.cmake, and on a case-insensitive filesystem that matches thezstdConfig.cmakezstd installs. That package exportszstd::libzstd_shared/_static, so without the alias the four targets linkingzstd::zstdfail at generate time on macOS. The shim now also disables zstd with a warning if the config package exports none of the names it knows, rather than failing the generate.
Every clause of that is true of the code at this head, including the warning behavior, which I tested separately.
New in this round
None that I would hold the PR for. Three notes:
- The friend hook is the right shape.
RamCacheCLFUS.h:108declaresfriend struct RamCacheCLFUSTestAccesswith a comment saying nothing in the product uses it, which matches the existingSSLNetVConnectionAsyncEpTestAccesspattern insrc/iocore/net/P_SSLNetVConnection.h:120. The test-side lookup at test_RamCacheCLFUS.cc:43 computes the bucket the same wayget()does atRamCacheCLFUS.cc:311, and it is guarded by aREQUIREon a non-null result, so a future divergence fails loudly rather than making the final "entry is gone" check pass for the wrong reason. - The corruption is a full overwrite of the blob rather than a single flipped byte. I think that is the better choice: none of these codecs carry a content checksum, so a one-byte flip inside an lz4 block can decode to the right length with wrong bytes and would make the test flaky. The
memsetis bounded bye->compressed_len, andcompress_entries()allocates that buffer at exactlylbytes atRamCacheCLFUS.cc:732, so the write is in bounds. ASan agrees. info.ccstill prints anlzmaandlzma.runpair while lz4 and zstd are now runtime-only, so two conventions live in the same function. That is pre-existing for lzma, and I would rather not churnzstdfor symmetry given someone may be parsing it. No action.
Regressions from the rework
None found. Here is how I checked:
- History.
ebb773b8^is132ddc21, the head I approved, and the merge base is unchanged atf3c3ac9b. Nothing was rebased, so no hunk could have been silently dropped. The diff is confined to eight files. - The normal module path still works. A plain configure reports
Found ZSTD: /opt/homebrew/lib/libzstd.dylib (found suitable version "1.5.7", minimum required is "1.4.0")andFound LZ4: ... "1.10.0", minimum required is "1.7.5",HAVE_ZSTD_H 1andHAVE_LZ4_H 1land in the generatedink_config.h, and the shim block does not fire becausecmake/FindZSTD.cmake:70has already createdzstd::zstd. - Both libraries absent still builds, and the tests skip rather than fail. With
-DCMAKE_DISABLE_FIND_PACKAGE_ZSTD=ON -DCMAKE_DISABLE_FIND_PACKAGE_LZ4=ONI get/* #undef HAVE_ZSTD_H */and/* #undef HAVE_LZ4_H */, a clean build ofRamCacheCLFUS,RamCacheCompressEntriesandtraffic_layout, andAll tests passed (83 assertions in 5 test cases)withwarning: zstd is not compiled in; the zstd RAM cache compression backend is NOT tested.ZSTD_FOUNDis false there, so the new shim block is skipped entirely and cannot affect this configuration. - The new
Fatalstrings compile in the configuration that uses them. The lz4 and zstdFatalcalls only exist when the library is absent, and that is the build above, which compiled clean. - The blocking fix from two rounds ago is untouched.
src/records/RecordsConfig.ccis not in the new commit, andtest_records "[RecUtils]"still givesAll tests passed (89 assertions in 3 test cases), the same count as on132ddc21. - Nothing links a target the shim invented by accident. All four consumers gate on
HAVE_ZSTD_H, so the "no usable target" branch that clearsHAVE_ZSTD_His consistent: nothing tries to linkzstd::zstdin that case. Only bundled third-party libraries inlib/useinstall(EXPORT), and none of them link zstd, so the non-importedzstd_zstdinterface target has nothing to break. That is also exactly how master did it before this PR.
Verified
- The restored shim is load-bearing, and it fixes the case I reported. Configuring the full tree with
-DCMAKE_FIND_PACKAGE_PREFER_CONFIG=ONnow succeeds:ZSTD_DIR=/opt/homebrew/lib/cmake/zstd(the config package, not the module),HAVE_ZSTD_H 1, and the link line resolves to/opt/homebrew/lib/libzstd.1.5.7.dylib, which is the config target's path rather than the module's. I then deleted only the shim block from the same tree and reconfigured: generate fails atsrc/iocore/cache/CMakeLists.txt:66,src/iocore/net/CMakeLists.txt:137,src/traffic_layout/CMakeLists.txt:35andplugins/compress/CMakeLists.txt:29withTarget ... links to zstd::zstd but the target was not found. Restored, it generates. - All three real target names are handled, not just Homebrew's. I extracted the shim block verbatim and ran it against four simulated config packages with
ZSTD_DIRpointed at each. Exportingzstd::libzstd_shared, or onlyzstd::libzstd_static, or onlyzstd::libzstd, each giveszstd::zstd EXISTSwithHAVE_ZSTD_H=1. A package exporting none of the three gives the warningzstd found but it exports no target this build can use; building without zstd,HAVE_ZSTD_H=FALSE, and a successful generate. - The 1.4.0 zstd floor is still enforced on the config path. With
CMAKE_FIND_PACKAGE_PREFER_CONFIG=ONagainst real Homebrew zstd,find_package(ZSTD 99.0.0)givesZSTD_FOUND=0andfind_package(ZSTD 1.4.0)givesZSTD_FOUND=1. - The new decode-failure test bites, and it bites on the counter. I re-ran my exact mutation. Replacing the lz4
l != rccondition atRamCacheCLFUS.cc:375withif (false)now fails the suite:test cases: 5 | 4 passed | 1 failed, withCHECK(hit == 0)reporting5 == 0,CHECK(... ram_cache_decompress_failures) == before + 1)reporting3 == 4, andCHECK(find_entry(rc, key) == nullptr)reporting a live pointer, all three taggedcompression backend: lz4. So it asserts on the counter and on the entry's removal, not merely on a miss. - The zstd half needs both checks disabled, and the author already flagged this. Mutating only
ZSTD_isError(ll)atRamCacheCLFUS.cc:398leaves the suite green, because the adjacentl != llcheck at line 404 catches the same corrupt frame. That is a property of my mutation, not a coverage gap: the corrupt frame is still never served. Mutating both lines toif (false)does fail the suite, with the same three assertions taggedcompression backend: zstdandhitreported as6 == 0. The author reached the same conclusion independently and wrote it up in their comment. - All five call sites of
note_decompress_failure()are now exercised, each with a distinct message. One run prints all five:fastlz_decompress produced 0 bytes, expected 262144,uncompress: data error,lzma_stream_buffer_decode returned 7, wrote 0 of 262144 output bytes,LZ4_decompress_safe returned -1051, expected 262144, andZSTD_decompressDCtx: Unknown frame descriptor. Yesterday no test reached any of them. -sis clean and a genuine round-trip break now reports the offset../RamCacheCLFUS -sgivesAll tests passed (131 assertions in 5 test cases)with nobasic_stringexception. To prove the new reporting is useful rather than merely quiet, I mutated the zstd read path to flip one byte of the decompressed output. The failure readsCHECK(first_difference(r.out, payload) == payload.size())with expansion12345 (0x3039) == 262144 (0x40000)and the messagecompression backend: zstd, which is the byte offset I corrupted.- The lz4 floor is enforced in both directions. Against Homebrew lz4 1.10.0 with this tree's
cmake/FindLZ4.cmake:find_package(LZ4 99.0.0)givesCould NOT find LZ4: Found unsuitable version "1.10.0", but required is at least "99.0.0", andfind_package(LZ4 1.7.5)givesfound suitable version "1.10.0", minimum required is "1.7.5". - ASan is clean on the new test. An ASan build of
RamCacheCLFUSgivesAll tests passed (131 assertions in 5 test cases). That matters here because the new test writes into anIOBufferDataand then drives_destroy(e)on all five codecs, so a bad bound or a use-after-free on the failure path would show up. - Assertion counts cross-check. 131 with both libraries, 83 with both disabled but liblzma present. The delta is 48 for two backends, so 24 per compressing backend and 11 for the uncompressed case. That makes the author's reported 59 the same tree with liblzma also absent (11 + 2 × 24), so their number and mine agree rather than conflict.
- CI is green on
ebb773b8. All 14 checks pass: 4 AuTest shards, CentOS, Clang-Analyzer, Debian, Docs, Fedora, Format, OSX, RAT, Rocky, Ubuntu. I did not retrigger anything. The same caveat as last time applies: the unit test warns rather than fails when a backend is absent, so a green run does not by itself prove the new code was compiled anywhere in CI, which is why I built and ran it locally. apache/trafficserver-ci#441 is what closes that gap for the Fedora image.
One local note, so nobody chases it: my host has a stray OpenSSL 3.4.0 in /usr/local/include that wins over the Homebrew 3.6.4 headers and breaks src/iocore/net/SSLStats.cc on SSL_CTX_get0_implemented_groups. That is my machine, not this PR. I worked around it with an include shim to get the tree to build.
4 participants
Add lz4 and zstd compression support to the CLFUS RAM cache
Summary
This adds two modern compression backends to the CLFUS RAM cache, as optional build-time dependencies:
proxy.config.cache.ram_cache.compress: 4) — a replacement for fastlz: strictly better compression ratio at substantially higher throughput.proxy.config.cache.ram_cache.compress: 5, level 3) — a replacement for libz/deflate-6: comparable ratio at roughly 10× the compression speed and 3× the decompression speed.The existing fastlz/libz/liblzma backends are unchanged and remain valid for their config values; the docs now recommend lz4 over fastlz and zstd over libz.
Benchmarks (lzbench, silesia XML corpus, Xeon Gold 6338, one thread):
Implementation notes
LZ4_compress_default/LZ4_decompress_safeAPI.thread_localZSTD_CCtx/ZSTD_DCtx) withZSTD_compress2and a sticky compression level, avoiding a context allocation per call — relevant since decompression sits on the cache-hit path. This requires zstd ≥ 1.4.0 (the first release with the advanced one-shot API stable); the version floor is enforced infind_package.Fatal, matching the existing liblzma behavior.cmake/FindLZ4.cmakeandcmake/FindZSTD.cmakemodules, written in the same shape as the tree's otherFind*.cmake(Apache header,find_library/find_path,INTERFACE IMPORTEDtarget) plus the header-version parse needed to enforce a floor. zstd detection previously usedfind_package(zstd CONFIG)only, which fails on distributions that don't ship a cmake config package; it now resolves via the module.zstd::zstdalias shim is kept, and it is not dead code. WithCMAKE_FIND_PACKAGE_PREFER_CONFIGthe lookup can resolve through zstd's own config package rather than the module: CMake searches forZSTDConfig.cmake, and on a case-insensitive filesystem that matches thezstdConfig.cmakezstd installs. That package exportszstd::libzstd_shared/_static, so without the alias the four targets linkingzstd::zstdfail at generate time on macOS. The shim now also disables zstd with a warning if the config package exports none of the names it knows, rather than failing the generate.LZ4_compress_default(), andLZ4_versionString(), whichtraffic_layoutreports, needs 1.7.5.traffic_layout infonow reportsTS_HAS_LZ4, and--versionsreports the lz4 runtime version underlz4, matching what the existingzstdkey means.Testing
The
RamCacheCLFUSclass definition moved from the.ccinto a new private header so unit tests can drivecompress_entries()synchronously. A new Catch2 test (test_RamCacheCLFUS) does store → compress → read-back roundtrips across all five codecs plus the uncompressed case, asserting byte-for-byte equality and the expectedRAM_HIT_COMPRESS_*state, plus incompressible-fallback and small-payload cases. No prior test verified compression data integrity for any backend.Drive-by fixes
e->leninstead oflzma_stream_buffer_bound(e->len); it now matches the other backends, with the existingREQUIRED_COMPRESSION/REQUIRED_SHRINKthresholds deciding what to keep. To be precise about what this fixes: it is a robustness fix, not a memory-safety or memory-saving one.lzma_easy_buffer_encode()is told the output size and returnsLZMA_BUF_ERRORrather than overrunning, so the old code was never unsafe — it just failed the encode for any object that didn't shrink, and that failure marked the entryincompressible. Old code and new code both end up reading back asRAM_HIT_COMPRESS_NONEfor such an object (the old one by failing, the new one by storing it raw), so no test can discriminate the two; what changes is that objects liblzma can compress but whose bound exceedse->lennow get compressed instead of being written off.proxy.config.cache.ram_cache.compresshad validity pattern[0-3]inRecordsConfig.cc, soRecYAMLDecoderrejected the new values 4 and 5 at load time, logged a validity warning and fell back to the default of0. Without this, setting4or5inrecords.yamlas the docs describe would silently leave compression off. Widened to[0-5], with a unit test that walks everyCACHE_COMPRESSION_*value against the record's own check and pattern so the range cannot drift behind the enum again.compress: 2was configured — zlib is a required dependency and always available.Observability
Two new counters, global and per-volume:
ram_cache.decompress.failure— an entry failed to decompress on read. The entry is dropped and the read becomes a miss, and a throttledWarningcarries the codec's own diagnosis (ZSTD_getErrorName(),zError(), or the codec's numeric return) so a nonzero counter can be told apart from a corrupt frame versus a bookkeeping error ine->len. Previously any decode failure was indistinguishable from an ordinary miss outside a debug build.ram_cache.compress.failure— an entry the compression library could not compress. Objects that merely did not shrink enough are deliberately not counted; that is the ordinary outcome for already-compressed content and would swamp the signal. A thread whose zstd context cannot be allocated skips its compression pass entirely and is not counted here either, since that would make the counter climb once a second per stripe for the life of the process; the one-timeWarningreports that condition.CI / packaging
liblz4-dev/lz4-develadded to the deb and yum CI images. The Fedora CI image will needlz4-develadded for build coverage of the new backend (zstd-devel is already present).Future work
Compression is currently implemented inside CLFUS only. A follow-up refactor will extract it into a shared layer so all RAM cache algorithms can use it; the new unit test is structured to migrate there.