brbzull0 commented on code in PR #13785:
URL: https://github.com/apache/trafficserver/pull/13785#discussion_r4182145523
##########
src/iocore/cache/RamCacheCLFUS.h:
##########
@@ -85,6 +85,14 @@ class RamCacheCLFUS : public RamCache
void compress_entries(EThread *thread, int do_at_most = INT_MAX);
+ static constexpr uint32_t entry_overhead = 256; // per-entry overhead to
consider when computing cache value/size
Review Comment:
suggestion: (non-blocking) Both of these land in the `public:` section, so
they join the class's public surface for every includer, solely so the unit
test can reach the helper. The file already has a mechanism for exactly that:
`friend struct RamCacheCLFUSTestAccess` at line 116, carrying the comment
"Nothing in the product uses this", with the struct defined in the test at
`unit_tests/test_RamCacheCLFUS.cc:43`.
Declaring both members `private` and adding a forwarder to
`RamCacheCLFUSTestAccess` would leave the public surface unchanged. One
mechanical consequence: the file-scope `cache_value` at `RamCacheCLFUS.cc:123`
would lose access and would have to move into the class as a private static —
all seven of its call sites (`:310`, `:835`, `:872`, `:873`, `:886`, `:891`,
`:892`) are already inside `get()` and `put()`, so that move needs no call-site
edits.
##########
src/iocore/cache/unit_tests/test_RamCacheCLFUS.cc:
##########
@@ -375,3 +375,20 @@ TEST_CASE("CLFUS compression backends compiled in",
"[cache][ramcache][compress]
#endif
CHECK(compression_cases().size() >= 3);
}
+
+// Guards against PR #11733-style regressions of the CLFUS value metric: the
value density
+// must be computed in floating point. Integer division truncates (hits + 1) /
(size + overhead)
+// to 0 for normal object sizes, zeroing the metric and silently collapsing
CLFUS to FIFO (no
+// promote-on-hit, no clock second chance, no value-based ghost re-admission).
+TEST_CASE("CLFUS value metric is a floating-point density",
"[cache][ramcache][clfus]")
+{
+ constexpr double v_one = RamCacheCLFUS::cache_value_hits_size(1u, 16384u);
// a typical 16 KiB object, seen once
+ constexpr double v_hot = RamCacheCLFUS::cache_value_hits_size(100u,
16384u); // same size, many more hits
+ constexpr double v_small = RamCacheCLFUS::cache_value_hits_size(10u, 1024u);
// smaller object, equal hits
+ constexpr double v_large = RamCacheCLFUS::cache_value_hits_size(10u, 16384u);
+
+ // A non-zero fraction: the integer-division regression makes this exactly
0.0.
+ STATIC_REQUIRE(v_one > 0.0);
Review Comment:
suggestion: (non-blocking) Keeping the three `static_assert` messages would
preserve the diagnostics. The vendored Catch2 3.9 stringizes the expression as
the message (`lib/Catch2/src/catch2/catch_test_macros.hpp:171`), so when the
second one fires the compiler now reports `static_assert failed: "v_hot >
v_one"` instead of the `"CLFUS value metric does not increase with hits"` it
reported before. The block comment above covers the first assert only.
`STATIC_REQUIRE` is also new to the tree — these three lines are its only
uses — whereas a plain `static_assert` inside a `TEST_CASE` is already used in
`test_Unmarshal_Compat.cc`, `test_ConfigReloadTask.cc:146` and
`test_QUICConnectionId.cc:32`. Using `static_assert` with the original messages
keeps both the diagnostics and that idiom; the `SUCCEED()` half of
`STATIC_REQUIRE` only adds a cosmetic passing assertion.
--
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]
