Copilot commented on code in PR #13893:
URL: https://github.com/apache/trafficserver/pull/13893#discussion_r4352810653
##########
src/iocore/cache/RamCacheCLFUS.cc:
##########
@@ -243,6 +244,20 @@ RamCacheCLFUS::init(int64_t abytes, StripeSM *astripe)
}
}
+void
+RamCacheCLFUS::_decay_epoch()
+{
+ forl_LL(RamCacheCLFUSEntry, e, this->_lru[0])
+ {
+ e->hits >>= 1;
+ }
+ forl_LL(RamCacheCLFUSEntry, e, this->_lru[1])
+ {
+ e->hits >>= 1;
+ }
+ this->_average_value *= 0.5;
+}
Review Comment:
Calling `_decay_epoch()` from `put()` does a full traversal of both LRU
lists. On large RAM caches this can introduce periodic latency spikes (the
first `put()` after 30s pays O(N)). Consider an incremental/lazy decay design
(e.g., epoch counter with per-entry last_epoch, or time-sliced work budget per
`put()`/`get()`) to spread the cost while preserving aging behavior.
##########
src/iocore/cache/RamCacheCLFUS.cc:
##########
@@ -775,10 +791,18 @@ RamCacheCLFUS::put(CryptoHash *key, IOBufferData *data,
uint32_t len, bool copy,
if (!this->_max_bytes) {
return 0;
}
+ ink_hrtime now = ink_get_hrtime();
+ if (now - this->_last_decay_time > HRTIME_SECONDS(30)) {
+ if (this->_last_decay_time != 0) {
+ this->_decay_epoch();
+ }
+ this->_last_decay_time = now;
+ }
Review Comment:
Calling `_decay_epoch()` from `put()` does a full traversal of both LRU
lists. On large RAM caches this can introduce periodic latency spikes (the
first `put()` after 30s pays O(N)). Consider an incremental/lazy decay design
(e.g., epoch counter with per-entry last_epoch, or time-sliced work budget per
`put()`/`get()`) to spread the cost while preserving aging behavior.
##########
src/iocore/cache/unit_tests/test_RamCacheCLFUS.cc:
##########
@@ -398,3 +428,183 @@ TEST_CASE("CLFUS value metric is a floating-point
density", "[cache][ramcache][c
static_assert(v_hot > v_one, "CLFUS value metric does not increase with
hits");
static_assert(v_small > v_large, "CLFUS value metric does not decrease with
size");
}
+
+namespace
+{
+
+CryptoHash
+make_key(uint64_t id)
+{
+ CryptoHash key;
+ key.u64[0] = 0xc0ffee00 + id;
+ key.u64[1] = 0xdeadbeef + id;
+ key.u32[3] = static_cast<uint32_t>(0x12345678 + id);
Review Comment:
`CryptoHash` commonly exposes multiple overlapping views (e.g., `u64[]` and
`u32[]`) of the same underlying bytes. Writing `u32[3]` after `u64[1]` can
partially overwrite the bytes you just set (and can be endianness/layout
sensitive), making the test key construction less deterministic/portable.
Prefer initializing exactly one view (all `u64[]`, or a byte array) or
zero-initialize then fill a single representation.
##########
src/iocore/cache/RamCacheCLFUS.cc:
##########
@@ -834,7 +858,7 @@ RamCacheCLFUS::put(CryptoHash *key, IOBufferData *data,
uint32_t len, bool copy,
}
Que(RamCacheCLFUSEntry, lru_link) victims;
RamCacheCLFUSEntry *victim = nullptr;
- int requeue_count = requeue_limit;
+ int requeue_count =
std::min(static_cast<int64_t>(requeue_limit), this->_objects);
Review Comment:
`std::min(...)` here yields an `int64_t` but is assigned to `int`. Depending
on compiler flags (e.g., `-Wconversion/-Werror`) this can warn or truncate when
`_objects` exceeds `INT_MAX`. Use a consistent type (e.g., make `requeue_count`
`int64_t` or explicitly cast after bounding) to avoid narrowing/truncation.
##########
src/iocore/cache/RamCacheCLFUS.cc:
##########
@@ -943,7 +977,7 @@ RamCacheCLFUS::put(CryptoHash *key, IOBufferData *data,
uint32_t len, bool copy,
e = THREAD_ALLOC(ramCacheCLFUSEntryAllocator, this_ethread());
e->key = *key;
e->auxkey = auxkey;
- e->hits = 1;
+ e->hits = 2;
e->size = data->block_size();
e->flags = 0;
this->_bucket[i].push(e);
Review Comment:
This changes the initial hit count for all newly allocated entries from 1 to
2. The PR description calls out initializing new *history* entries with `hits =
2`, but this allocation site appears to be shared by multiple insertion paths.
If this can also allocate resident (_lru[0]) entries, it will unintentionally
bias admission/retention. Consider setting the initial hits based on the
destination queue (history vs resident), or keep resident entries at 1 and only
bump history entries.
--
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]
