bryancall commented on code in PR #13257:
URL: https://github.com/apache/trafficserver/pull/13257#discussion_r3398037994
##########
src/iocore/cache/RamCacheCLFUS.cc:
##########
@@ -298,6 +285,31 @@ RamCacheCLFUS::get(CryptoHash *key, Ptr<IOBufferData>
*ret_data, uint64_t auxkey
ram_hit_state = RAM_HIT_COMPRESS_LIBLZMA;
break;
}
+#endif
+#ifdef HAVE_LZ4_H
+ case CACHE_COMPRESSION_LZ4: {
+ int l = static_cast<int>(e->len);
+ if (l != LZ4_decompress_safe(e->data->data(), b,
e->compressed_len, l)) {
+ goto Lfailed;
+ }
+ ram_hit_state = RAM_HIT_COMPRESS_LZ4;
+ break;
+ }
+#endif
+#ifdef HAVE_ZSTD_H
+ case CACHE_COMPRESSION_ZSTD: {
+ size_t l = static_cast<size_t>(e->len);
+ ZSTD_DCtx *dctx = zstd_dctx();
+ if (dctx == nullptr) {
+ goto Lfailed;
+ }
Review Comment:
A null `dctx` here is a *thread/allocator* condition, not entry corruption,
but `goto Lfailed` destroys an otherwise-valid compressed entry — so a
transient allocation failure evicts good cache entries. Consider treating a
null `dctx` as a miss (`goto Lerror`, leaving the entry intact) rather than
`Lfailed`.
More broadly: both the null-`dctx` and the `ZSTD_isError(ll) || l != ll`
paths fall into `Lfailed` with only the debug-gated `Z_ERR` trace, so a genuine
decompression/integrity error is indistinguishable from an ordinary cache miss
in production. A rate-limited `Warning` + a distinct metric at the shared
`Lfailed` label (reading the entry fields into locals before `_destroy(e)`)
would make corruption visible. Note this is the same pattern the existing
fastlz/libz paths already have, so it's a pre-existing gap now extended to two
more codecs — worth fixing once for all of them.
##########
src/iocore/cache/unit_tests/test_RamCacheCLFUS.cc:
##########
@@ -0,0 +1,229 @@
+/** @file
+
+ Catch-based unit tests for RAM cache (CLFUS) compression roundtrips.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include "main.h"
+
+#include "../RamCacheCLFUS.h"
+#include "../P_CacheInternal.h"
+
+#include "iocore/cache/Cache.h"
+#include "tscore/ink_config.h"
+
+#include <cstdint>
+#include <cstring>
+#include <string>
+#include <vector>
+
+// Required by main.h
+int cache_vols = 1;
+bool reuse_existing_cache = false;
+
+namespace
+{
+
+// A compression backend to exercise, along with the RAM_HIT_* state get()
+// should report once a compressible object has been stored compressed.
+struct CompressionCase {
+ int config; // CACHE_COMPRESSION_*
+ int expected_hit; // RAM_HIT_COMPRESS_* reported by get() for
compressible data
+ const char *name;
+};
+
+std::vector<CompressionCase>
+compression_cases()
+{
+ std::vector<CompressionCase> cases{
+ {CACHE_COMPRESSION_NONE, RAM_HIT_COMPRESS_NONE, "none" },
+ {CACHE_COMPRESSION_FASTLZ, RAM_HIT_COMPRESS_FASTLZ, "fastlz"},
+ {CACHE_COMPRESSION_LIBZ, RAM_HIT_COMPRESS_LIBZ, "libz" },
+ };
+#ifdef HAVE_LZMA_H
+ cases.push_back({CACHE_COMPRESSION_LIBLZMA, RAM_HIT_COMPRESS_LIBLZMA,
"liblzma"});
+#endif
+#ifdef HAVE_LZ4_H
+ cases.push_back({CACHE_COMPRESSION_LZ4, RAM_HIT_COMPRESS_LZ4, "lz4"});
+#endif
+#ifdef HAVE_ZSTD_H
+ cases.push_back({CACHE_COMPRESSION_ZSTD, RAM_HIT_COMPRESS_ZSTD, "zstd"});
+#endif
Review Comment:
These cases are gated on `HAVE_LZ4_H`/`HAVE_ZSTD_H`, and
`find_package(LZ4/ZSTD)` is non-`REQUIRED`, so a build without the dev libs (or
zstd < 1.4.0) skips the lz4/zstd cases and the suite still passes green — i.e.
the test can't fail for the two backends it was written to protect. The CI
Dockerfiles do add the libs, but nothing asserts they were actually compiled
in. A skip-with-`WARN` (or a static check on the CI image) would make a missing
backend visible in the test output instead of silently reducing coverage.
##########
src/iocore/cache/unit_tests/test_RamCacheCLFUS.cc:
##########
@@ -0,0 +1,229 @@
+/** @file
+
+ Catch-based unit tests for RAM cache (CLFUS) compression roundtrips.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include "main.h"
+
+#include "../RamCacheCLFUS.h"
+#include "../P_CacheInternal.h"
+
+#include "iocore/cache/Cache.h"
+#include "tscore/ink_config.h"
+
+#include <cstdint>
+#include <cstring>
+#include <string>
+#include <vector>
+
+// Required by main.h
+int cache_vols = 1;
+bool reuse_existing_cache = false;
+
+namespace
+{
+
+// A compression backend to exercise, along with the RAM_HIT_* state get()
+// should report once a compressible object has been stored compressed.
+struct CompressionCase {
+ int config; // CACHE_COMPRESSION_*
+ int expected_hit; // RAM_HIT_COMPRESS_* reported by get() for
compressible data
+ const char *name;
+};
+
+std::vector<CompressionCase>
+compression_cases()
+{
+ std::vector<CompressionCase> cases{
+ {CACHE_COMPRESSION_NONE, RAM_HIT_COMPRESS_NONE, "none" },
+ {CACHE_COMPRESSION_FASTLZ, RAM_HIT_COMPRESS_FASTLZ, "fastlz"},
+ {CACHE_COMPRESSION_LIBZ, RAM_HIT_COMPRESS_LIBZ, "libz" },
+ };
+#ifdef HAVE_LZMA_H
+ cases.push_back({CACHE_COMPRESSION_LIBLZMA, RAM_HIT_COMPRESS_LIBLZMA,
"liblzma"});
+#endif
+#ifdef HAVE_LZ4_H
+ cases.push_back({CACHE_COMPRESSION_LZ4, RAM_HIT_COMPRESS_LZ4, "lz4"});
+#endif
+#ifdef HAVE_ZSTD_H
+ cases.push_back({CACHE_COMPRESSION_ZSTD, RAM_HIT_COMPRESS_ZSTD, "zstd"});
+#endif
+ return cases;
+}
+
+// Minimal CacheDisk wiring needed to construct a StripeSM. Mirrors the helper
+// in test_Stripe.cc.
+void
+init_disk(CacheDisk &disk)
+{
+ disk.path = static_cast<char *>(ats_malloc(1));
+ disk.path[0] = '\0';
+ disk.disk_stripes = static_cast<DiskStripe
**>(ats_malloc(sizeof(DiskStripe *)));
+ disk.disk_stripes[0] = nullptr;
+ disk.header = static_cast<DiskHeader
*>(ats_malloc(sizeof(DiskHeader)));
+ disk.header->num_volumes = 0;
+}
+
+// The CLFUS get/put/compress paths touch only these metrics and the stripe
+// mutex, so that is all the stripe needs for these tests.
+void
+wire_stripe(StripeSM &stripe, CacheVol &cache_vol)
+{
+ stripe.cache_vol = &cache_vol;
+
+ cache_rsb.ram_cache_bytes =
ts::Metrics::Gauge::createPtr("unit_test.clfus.ram_cache.bytes");
+ cache_rsb.ram_cache_hits =
ts::Metrics::Counter::createPtr("unit_test.clfus.ram_cache.hits");
+ cache_rsb.ram_cache_misses =
ts::Metrics::Counter::createPtr("unit_test.clfus.ram_cache.misses");
+ cache_vol.vol_rsb.ram_cache_bytes =
ts::Metrics::Gauge::createPtr("unit_test.clfus.vol.ram_cache.bytes");
+ cache_vol.vol_rsb.ram_cache_hits =
ts::Metrics::Counter::createPtr("unit_test.clfus.vol.ram_cache.hits");
+ cache_vol.vol_rsb.ram_cache_misses =
ts::Metrics::Counter::createPtr("unit_test.clfus.vol.ram_cache.misses");
+}
+
+Ptr<IOBufferData>
+make_buffer(const std::vector<char> &bytes)
+{
+ int64_t idx = iobuffer_size_to_index(bytes.size(),
MAX_BUFFER_SIZE_INDEX);
+ Ptr<IOBufferData> data{make_ptr(new_IOBufferData(idx, MEMALIGNED))};
+ std::memcpy(data->data(), bytes.data(), bytes.size());
+ return data;
+}
+
+// Highly compressible: a short repeating pattern.
+std::vector<char>
+compressible_bytes(std::size_t len)
+{
+ std::vector<char> bytes(len);
+ for (std::size_t i = 0; i < len; i++) {
+ bytes[i] = static_cast<char>('A' + (i % 26));
+ }
+ return bytes;
+}
+
+// Effectively incompressible: a deterministic xorshift byte stream.
+std::vector<char>
+incompressible_bytes(std::size_t len)
+{
+ std::vector<char> bytes(len);
+ uint32_t state = 0x9e3779b9;
+ for (std::size_t i = 0; i < len; i++) {
+ state ^= state << 13;
+ state ^= state >> 17;
+ state ^= state << 5;
+ bytes[i] = static_cast<char>(state & 0xff);
+ }
+ return bytes;
+}
+
+// Store payload under a fresh key, force a synchronous compression pass with
+// `config`, then read it back. Returns the RAM_HIT_* state reported by get()
+// and the bytes that were returned.
+int
+store_compress_get(StripeSM &stripe, int config, const std::vector<char>
&payload, std::vector<char> &out)
+{
+ // Initialize with compression disabled so init() does not schedule the
+ // background compressor (which would retain a pointer to this stack object).
+ cache_config_ram_cache_compress = CACHE_COMPRESSION_NONE;
+ cache_config_ram_cache_compress_percent = 100;
+ cache_config_ram_cache_use_seen_filter = 0;
+
+ RamCacheCLFUS rc;
+ rc.init(1 << 20, &stripe);
+
+ Ptr<IOBufferData> in = make_buffer(payload);
+ uint32_t len = static_cast<uint32_t>(payload.size());
+
+ static uint64_t salt = 0;
+ ++salt;
+ CryptoHash key;
+ key.u64[0] = 0xc0ffee00 + salt;
+ key.u64[1] = 0xdeadbeef + salt;
+
+ REQUIRE(rc.put(&key, in.get(), len) == 1);
+
+ cache_config_ram_cache_compress = config;
+ rc.compress_entries(this_ethread());
+
+ Ptr<IOBufferData> ret;
+ int hit = rc.get(&key, &ret);
+ REQUIRE(ret.get() != nullptr);
+ out.assign(ret->data(), ret->data() + len);
+ return hit;
+}
+
+} // namespace
+
+TEST_CASE("CLFUS compressible objects roundtrip cleanly",
"[cache][ramcache][compress]")
+{
+ CacheDisk disk;
+ init_disk(disk);
+ StripeSM stripe{&disk, 10, 0};
+ CacheVol cache_vol;
+ wire_stripe(stripe, cache_vol);
+
+ auto payload = compressible_bytes(8192);
+ const CompressionCase c = GENERATE(from_range(compression_cases()));
+ INFO("compression backend: " << c.name);
+
+ std::vector<char> out;
+ int hit = store_compress_get(stripe, c.config, payload, out);
+
+ CHECK(hit == c.expected_hit);
+ CHECK(out == payload);
Review Comment:
This asserts the read-back bytes and the reported `RAM_HIT_*` state, but not
that compression actually happened — a backend that "compresses" to a
same-or-larger blob but still tags the type would pass. Adding
`CHECK(e->compressed_len < len)` for the compressible case asserts the
feature's actual contract (it saves memory). A larger payload (say >= 128 KB)
would also exercise the `*_compressBound()` math and the `uint32_t` casts in
`compress_entries()`, which the 8 KB / 1-byte cases don't reach.
##########
src/iocore/cache/RamCacheCLFUS.cc:
##########
@@ -35,6 +36,45 @@
#ifdef HAVE_LZMA_H
#include <lzma.h>
#endif
+#ifdef HAVE_LZ4_H
+#include <lz4.h>
+#endif
+#ifdef HAVE_ZSTD_H
+#include <zstd.h>
+#include <memory>
+constexpr int CLFUS_ZSTD_LEVEL = 3;
+
+namespace
+{
+
+// One-shot ZSTD_compress/ZSTD_decompress allocate and free a context on every
+// call, so reuse a per-thread context instead. May return nullptr if zstd
+// fails to allocate one. The compression level is a sticky parameter set once
+// here; no explicit ZSTD_CCtx_reset() is needed because ZSTD_compress2()
+// starts a new session on every call (resets are only for interrupting the
+// streaming API or changing sticky parameters).
+ZSTD_CCtx *
+zstd_cctx()
+{
+ thread_local std::unique_ptr<ZSTD_CCtx, size_t (*)(ZSTD_CCtx *)> ctx = [] {
+ std::unique_ptr<ZSTD_CCtx, size_t (*)(ZSTD_CCtx *)> c{ZSTD_createCCtx(),
ZSTD_freeCCtx};
+ if (c && ZSTD_isError(ZSTD_CCtx_setParameter(c.get(),
ZSTD_c_compressionLevel, CLFUS_ZSTD_LEVEL))) {
+ c.reset();
+ }
+ return c;
+ }();
+ return ctx.get();
Review Comment:
The context is a `thread_local` initialized once via the IIFE lambda, so a
failed `ZSTD_createCCtx()`/`ZSTD_CCtx_setParameter()` (e.g. OOM) is **sticky
for the life of the thread** — the lambda never re-runs, so `zstd_cctx()`
returns `nullptr` on every subsequent call on that thread.
Downstream that's silent: in `compress_entries()` a null `cctx` sets `failed
= true`, which marks the entry `incompressible` and stores it uncompressed and
never retried. So a transient allocation failure permanently degrades every
object on that thread to uncompressed, with no log line or metric to explain
the lost compression. Consider a rate-limited `Warning` (and ideally a distinct
metric) the first time a context can't be allocated, so this is diagnosable
rather than invisible. Same applies to `zstd_dctx()` on the get path (see the
get() comment).
##########
include/iocore/cache/Cache.h:
##########
@@ -43,8 +43,18 @@ static constexpr ts::ModuleVersion CACHE_MODULE_VERSION(1,
0);
#define CACHE_COMPRESSION_FASTLZ 1
#define CACHE_COMPRESSION_LIBZ 2
#define CACHE_COMPRESSION_LIBLZMA 3
-
-enum { RAM_HIT_COMPRESS_NONE = 1, RAM_HIT_COMPRESS_FASTLZ,
RAM_HIT_COMPRESS_LIBZ, RAM_HIT_COMPRESS_LIBLZMA, RAM_HIT_LAST_ENTRY };
+#define CACHE_COMPRESSION_LZ4 4
+#define CACHE_COMPRESSION_ZSTD 5
+
+enum {
+ RAM_HIT_COMPRESS_NONE = 1,
+ RAM_HIT_COMPRESS_FASTLZ,
+ RAM_HIT_COMPRESS_LIBZ,
+ RAM_HIT_COMPRESS_LIBLZMA,
+ RAM_HIT_COMPRESS_LZ4,
+ RAM_HIT_COMPRESS_ZSTD,
+ RAM_HIT_LAST_ENTRY
+};
Review Comment:
`CACHE_COMPRESSION_*` (0..5) and `RAM_HIT_COMPRESS_*` (1..6) are two
parallel integer sequences kept in sync by hand with a +1 offset, and the
`compressed : 3` bitfield in `RamCacheCLFUSEntry` must hold the max
`CACHE_COMPRESSION_*` value. Both are correct today but unguarded. A couple of
`static_assert`s would pin the invariants at compile time, e.g.
`static_assert(RAM_HIT_COMPRESS_ZSTD == CACHE_COMPRESSION_ZSTD + 1);` and
`static_assert(CACHE_COMPRESSION_ZSTD < (1u << 3));` next to the bitfield — so
the next codec addition can't silently desync or overflow the field.
##########
doc/developer-guide/cache-architecture/ram-cache.en.rst:
##########
@@ -147,17 +147,23 @@ since we need to make a copy anyway. Those not tagged
``copy`` are inserted
uncompressed in the hope that they can be reused in uncompressed form. This is
a compile time option and may be something we want to change.
-There are 3 algorithms and levels of compression (speed on an Intel i7 920
-series processor using one thread):
+There are 5 algorithms and levels of compression (speed on an Intel Xeon Gold
+6338 processor using lzbench and the silesia XML corpus):
======= ================ ==================
====================================
Method Compression Rate Decompression Rate Notes
======= ================ ==================
====================================
-fastlz 173 MB/sec 442 MB/sec Basically free since disk or
network
- will limit first; ~53% final size.
-libz 55 MB/sec 234 MB/sec Almost free, particularly
- decompression; ~37% final size.
-liblzma 3 MB/sec 50 MB/sec Expensive; ~27% final size.
+fastlz 452 MB/sec 913 MB/sec Effectively obsolete; prefer lz4.
+ Basically free since disk or
network
+ will limit first; ~26% final size.
+libz 54 MB/sec 536 MB/sec Effectively obsolete; prefer zstd.
+ Almost free, particularly
+ decompression; ~13% final size.
+liblzma 5 MB/sec 291 MB/sec Expensive; ~8% final size.
+lz4 727 MB/sec 3458 MB/sec Basically free since disk or
network
+ will limit first; 23% final size
Review Comment:
Small doc nits: the lz4 row reads `23% final size` (no leading `~`, no
trailing period) while every other row uses the `~NN%.` form. Also worth noting
the zstd numbers are level-3 specific (`CLFUS_ZSTD_LEVEL = 3`); a note like
"zstd at level 3" ties the figures to the code constant so they don't silently
rot if the level changes. (Unrelated, while you're here: the `compressed` row a
few lines up documents the flag as `uncompressible`, but the struct field is
`incompressible`.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
