From c5e2b63ce1874ea6a5e0bd9ffc7cc6b7c90e87b3 Mon Sep 17 00:00:00 2001 From: Sam Shaplygin Date: Thu, 27 Aug 2026 22:58:11 +0200 Subject: [PATCH] docs: give each explanation one home, compress the core comments Four explanations were repeated between the core's comments and docs/, and each repetition was a place they could drift apart. They now live in one place each, and the comments state the invariant rather than re-arguing it. Moved out of the code, in full, not summarised away: the measured cost of not filling a shadow on a miss -> docs/design.md what demotion costs SIEVE and S3-FIFO specifically -> docs/policies.md MinEpochRequests counts sampled, not real, requests -> docs/configuration.md MinShadowCapacity raises the effective rate -> docs/configuration.md The last two were in the godoc and nowhere else, so a reader of the configuration guide could set either one wrong: at a 0.05 sample rate a MinEpochRequests of 100 is reached after roughly 2000 real requests, not 100. docs/policies.md also gains the three rules an arm has to honour, which until now were recorded only in a working file outside the repository: never return a zero value with true, keep Keys() and Values() aligned, and read size 0 as empty rather than unlimited. Each has been broken by a real implementation -- the first is why policies.NewTTL is written over a plain LRU rather than wrapping expirable. Comment density across the six core files, which the stage set out to bring under 25%: cache.go 109/361 = 30% -> 100/352 = 28% epoch.go 65/207 = 31% -> 64/206 = 31% shadow.go 114/215 = 53% -> 56/157 = 35% sampling.go 53/129 = 41% -> 41/117 = 35% migration.go 47/137 = 34% -> 43/133 = 32% settings.go 91/196 = 46% -> 61/166 = 36% total 479/1245 = 38% -> 365/1131 = 32% It stops at 32%, not 25%. What remains is invariants, lock preconditions and godoc on exported API; reaching the number would mean deleting documentation whose absence lets someone break correctness by editing nearby -- the switchLocked ordering rule, the three roles during a gradual window, why Peek and not Contains. The target and the rule that no removed comment may describe a load-bearing invariant conflict on these files, and the invariant wins. The README needed no work: it is 56 lines against a target of 90. --- cache.go | 51 +++++++--------- docs/configuration.md | 10 +++- docs/design.md | 11 +++- docs/policies.md | 33 ++++++++++ epoch.go | 35 ++++++----- migration.go | 14 ++--- sampling.go | 60 ++++++++----------- settings.go | 78 ++++++++---------------- shadow.go | 136 ++++++++++++------------------------------ 9 files changed, 180 insertions(+), 248 deletions(-) diff --git a/cache.go b/cache.go index 5797f48..c0b67c8 100644 --- a/cache.go +++ b/cache.go @@ -73,12 +73,11 @@ type AdaptiveCache[K comparable, V any] struct { // after each report, so an answer about the traffic has to be accumulated // somewhere. // - // It is cleared for both policies involved in a switch. Pooling a policy's - // active tenure with its shadow tenure would mix two different measurement - // regimes - full capacity over all traffic against miniature capacity over - // a sample - and, worse, would leave the just-demoted policy's long good - // history outweighing the promoted one's short history, so Advice would - // recommend reverting a switch the cache had just made correctly. + // It is cleared for both policies in a switch. Pooling a policy's active + // tenure with its shadow tenure mixes full capacity over all traffic with + // a miniature over a sample, and leaves the demoted policy's long history + // outweighing the promoted one's short one -- so Advice would recommend + // reverting a switch the cache had just made correctly. tenureStats map[PolicyType]PolicyStats // reportingEpochs counts only the epochs that actually measured something. @@ -125,10 +124,9 @@ func (c *AdaptiveCache[K, V]) recordActiveSample(sampled, hit bool) { // Get returns the value stored for key by the active policy, feeding the same // lookup to every shadow policy that samples the key. // -// When Settings.EpochRequests is set, the call that completes an epoch runs it -// here, after every lock this method took has been released - runEpoch needs -// the write lock, and a Get still holding the read lock would deadlock against -// it. +// With Settings.EpochRequests set, the call completing an epoch runs it here, +// after every lock this method took is released: runEpoch needs the write lock +// and would deadlock against a Get still holding the read lock. func (c *AdaptiveCache[K, V]) Get(key K) (V, bool) { value, found := c.get(key) c.countRequest() @@ -153,12 +151,10 @@ func (c *AdaptiveCache[K, V]) get(key K) (V, bool) { } c.mu.RUnlock() - // Gradual migration window: resolve the whole lookup under the write lock, - // promoting an eligible key into the active policy BEFORE its Get is - // counted. The active policy then records a hit for a request the cache - // serves; promoting after the Get would leave a spurious miss in the - // active arm's stats for a served request, skewing both Stats() and the - // bandit's posterior toward the demoted policy. + // Gradual window: resolve the lookup under the write lock, promoting an + // eligible key BEFORE its Get is counted. Promoting after would record a + // miss for a request the cache served, skewing Stats() and the bandit's + // posterior toward the demoted policy. c.mu.Lock() defer c.mu.Unlock() @@ -187,16 +183,11 @@ func (c *AdaptiveCache[K, V]) Add(key K, value V) bool { continue } - // Only a key the shadow does not already hold. A shadow's value is - // always the zero value, so re-adding a key it has carries no - // information - but it is not free: for a policy whose eviction - // state is a counter or a single bit, a write counts as an access. - // SIEVE would mark every freshly filled key as visited, defeating - // exactly the one-hit-wonder filtering it is carried for, and - // S3-FIFO's counter would run ahead of the algorithm. - // - // Peek rather than Contains or Get, because it must not disturb - // that state either. + // Only a key the shadow does not hold: its value is always zero, so + // re-adding carries no information but does count as an access for + // a policy whose eviction state is a counter or a bit. SIEVE would + // mark every filled key visited, defeating the one-hit-wonder + // filtering it is carried for. Peek, so the check disturbs nothing. if _, held := policy.Peek(key); held { continue } @@ -275,10 +266,10 @@ func (c *AdaptiveCache[K, V]) Purge() { // miniature capacity that corresponds to size rather than to size itself, so // they stay faithful simulations of a cache of the requested capacity. // -// The sample rate itself is fixed for the life of the cache: changing it would -// change which keys are sampled, invalidating every shadow's accumulated state. -// The miniature capacity therefore follows the rate directly here, without the -// MinShadowCapacity floor that construction applies - see scaledCapacity. +// The sample rate is fixed for the life of the cache -- changing it would +// change which keys are sampled and invalidate every shadow's state -- so the +// miniature capacity follows the rate directly, without the MinShadowCapacity +// floor construction applies. See scaledCapacity. func (c *AdaptiveCache[K, V]) Resize(size int) int { c.mu.Lock() defer c.mu.Unlock() diff --git a/docs/configuration.md b/docs/configuration.md index c5fb35b..218b8a9 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -90,7 +90,10 @@ is close to free. That is what makes carrying nine arms practical. Reproduce with `go test -run '^$' -bench . -benchtime=300ms .` -Sampling is off by default. Very small caches disable it automatically, since a +Sampling is off by default. `MinShadowCapacity` (256 unless you set it) is the +floor on a miniature: when the rate would shrink a shadow below it, the +*effective rate* is raised rather than the capacity alone, and on a cache small +enough that the floor exceeds its nominal size, sampling disables itself. A miniature of a handful of entries measures noise rather than a policy. Sampling does not distort which policy wins — that was measured directly, see @@ -111,6 +114,11 @@ migration. Three settings damp that, all inactive at their zero value: } ``` +`MinEpochRequests` counts the requests **the bandit sees**, which under +`ShadowSampleRate` are sampled requests: at a rate of 0.05 a threshold of 100 +is reached after roughly 2000 real ones. Set it against the sampled stream, not +against your traffic. + ## Tuning, measured The epoch duration is the setting that matters most, and the failure mode is diff --git a/docs/design.md b/docs/design.md index 88779e8..ae6c3fd 100644 --- a/docs/design.md +++ b/docs/design.md @@ -59,9 +59,14 @@ On each request: anything: a read-through caller only calls `Add` when the *active* policy missed, so without it a shadow could never acquire a key the incumbent was already serving, and the better the incumbent performed the less its rivals - were allowed to learn. Shadows hold keys and eviction bookkeeping, never - data, which is why N policies do not cost N times the memory — and why no - caller can ever be handed a shadow's zero. + were allowed to learn. The drift that causes is not a small bias: measured + on a cyclic workload behind a 94%-hit incumbent, arms that truly serve 0.00% + reported over 90%, because a starved shadow's contents go static and a + static cache covering most of a small keyspace looks excellent. Its sign + depends on which arm is incumbent, so it does not cancel — `Advice()` + recommended switching from the best arm to the worst. Shadows hold keys and + eviction bookkeeping, never data, which is why N policies do not cost N + times the memory — and why no caller can ever be handed a shadow's zero. Then once per epoch: diff --git a/docs/policies.md b/docs/policies.md index 47da8ff..38fdcab 100644 --- a/docs/policies.md +++ b/docs/policies.md @@ -208,6 +208,21 @@ fan-out skips a key the shadow already holds, so the counters no longer run ahead on shadow duty. It still applies to a caller whose own traffic rewrites live keys. +**Demotion disturbs their eviction state more than it disturbs the others'.** +When a policy stops being active it is rewritten to zero values in `Keys()` +order, which for a recency policy re-establishes the same order and for a +frequency policy adds one access to every surviving key, leaving the relative +order alone. Neither holds here. SIEVE treats a write as setting the visited +bit, and that bit is its whole eviction criterion, so rewriting every key sets +it on every key and erases the ordering rather than preserving it. S3-FIFO's +counter saturates at three, so a key already at the cap gains nothing while a +key at zero gains one, compressing the ordering instead of shifting it +uniformly. The effect is a bias in the demoted policy's first shadow epochs +rather than a standing loss — the queues are untouched and ordinary traffic +rewrites the bits soon after — but a policy whose eviction state is a single +saturating bit per entry should not be demoted this way without measuring what +it costs. + Two smaller notes. The adapter always builds with a TTL of zero, which is load-bearing: a non-zero TTL starts a background goroutine that would invoke the eviction callback from a goroutine the adapter never entered, and the @@ -243,3 +258,21 @@ Note that `Resize` on an adapted cache rebuilds it, discarding whatever adaptation the algorithm had learned. `AdaptiveCache` resizes shadow policies when its own capacity changes, so adapted policies are heavier arms to carry than natively resizable ones. + +Three rules an arm has to honour, each of which a real implementation has +broken here: + +- **Never return a zero value with `true`.** A `Get` or `Peek` that reports a + hit for an entry it no longer holds hands the caller a value nobody stored. + This library's central invariant is that a shadow's zero is never observable, + and one arm returning `(zeroValue, true)` for an expired-but-unreaped entry + defeats it from below. `hashicorp/golang-lru/v2/expirable` does exactly that, + which is why `policies.NewTTL` is written over a plain LRU with lazy expiry + rather than wrapping it. +- **`Keys()` and `Values()` must line up.** A `Values()` padded to full length + with trailing zeros does not correspond to `Keys()`, and warm migration + copies through both. +- **Size 0 means empty, not unlimited.** Shadows are resized automatically, so + an arm reading 0 as "no limit" turns a bounded miniature into an unbounded + cache. It must also not start a goroutine it gives you no way to stop: a + reaper per cache with no `Close` leaks the goroutine and the cache with it. diff --git a/epoch.go b/epoch.go index 55878ae..e437a75 100644 --- a/epoch.go +++ b/epoch.go @@ -25,15 +25,13 @@ func (c *AdaptiveCache[K, V]) runAdaptiveSelect() { } // countRequest advances the request-driven epoch clock and runs the epoch on -// the call that completes it. +// the call that completes it. Caller must hold no lock: runEpoch takes the +// write lock. // -// It must be called with no lock held: runEpoch takes the write lock. -// -// Exactly one caller per epoch observes the count equal to the limit, so -// exactly one epoch runs however many goroutines are in Get at once. The limit -// is then subtracted rather than the counter reset, so requests that arrived -// during the crossing are still counted towards the next epoch instead of -// being dropped. +// Exactly one caller per epoch sees the count equal the limit, so exactly one +// epoch runs however many goroutines are in Get. The limit is subtracted +// rather than the counter reset, so requests arriving mid-crossing still +// count towards the next epoch. func (c *AdaptiveCache[K, V]) countRequest() { limit := c.settings.EpochRequests if limit <= 0 { @@ -108,16 +106,17 @@ func (c *AdaptiveCache[K, V]) tryChangePolicy() PolicyType { return c.selectPolicyLocked() } -// selectPolicyLocked reports every policy's stats to the bandit — the active -// policy included, so its posterior does not go stale — and returns the -// bandit's chosen policy for the next epoch. When -// EvictPartialCapacityFilling is false and the active policy is not yet full, -// it returns early without reporting or resetting anything; counters then -// accumulate until the next reporting epoch. On a reporting epoch counters -// are reset after delivery; the active policy's counts are folded into -// globalStats first so Stats() stays cumulative and no active-tenure counts -// leak into a policy's first shadow epoch after demotion. It must be called -// while the write lock is held. +// selectPolicyLocked reports every policy's stats to the bandit -- the active +// policy included, so its posterior does not go stale -- and returns the arm +// chosen for the next epoch. +// +// With EvictPartialCapacityFilling false and the active policy not yet full it +// returns early, reporting and resetting nothing; counters accumulate until +// the next reporting epoch. Otherwise counters reset after delivery, the +// active policy's folded into globalStats first so Stats() stays cumulative +// and no active-tenure count leaks into a first shadow epoch after demotion. +// +// Caller must hold the write lock. func (c *AdaptiveCache[K, V]) selectPolicyLocked() PolicyType { currentPolicy := c.activePolicy diff --git a/migration.go b/migration.go index 36523ca..9458255 100644 --- a/migration.go +++ b/migration.go @@ -113,15 +113,11 @@ func (c *AdaptiveCache[K, V]) drainOneKey() { // (promotion here or an earlier Remove). It must be called while the write // lock is held during a gradual migration window. // -// A note on why the source is trustworthy here. It is not the active policy, -// so anything walking c.policies and skipping only activePolicy would treat it -// as a shadow and fill it with zero values - and the Peek below cannot tell -// such a zero from a real value still pending, so it would promote the zero -// and serve it to a caller as a hit. fanOutReadLocked therefore skips the -// source while a window is open. "Not active" is not the same as "is a -// shadow": for the duration of a gradual window there are three roles, not -// two, and anything iterating the policies has to say what it means to do to -// this one. +// The Peek below cannot tell a zero written by a shadow fill from a real value +// still pending, so fanOutReadLocked skips the source while a window is open. +// "Not active" is not "is a shadow": during a gradual window there are three +// roles, and anything iterating c.policies has to say what it does to this +// one. func (c *AdaptiveCache[K, V]) promoteLocked(key K) { // Skip keys the caller has since written directly, or already promoted. if _, ok := c.migrationRealKeys[key]; ok { diff --git a/sampling.go b/sampling.go index c168b09..9692eda 100644 --- a/sampling.go +++ b/sampling.go @@ -11,25 +11,19 @@ import ( const maxUint64AsFloat = float64(1 << 64) // keySampler decides whether a key belongs to the deterministic subset of the -// keyspace that shadow policies track. Sampling lets a shadow estimate its hit -// rate from a small fraction of traffic instead of mirroring every operation. +// keyspace that shadow policies track. // -// The decision is a pure function of the key and the seed, so a given key is -// either always sampled or never sampled for the lifetime of the sampler. That -// matters twice over: a sampled shadow sees a coherent access pattern for the -// keys it does track (rather than a random scatter that would destroy any -// notion of reuse), and every shadow sharing one sampler measures the same -// sub-workload, which is what makes their hit rates comparable to each other. +// The decision is a pure function of key and seed, so a key is either always +// sampled or never sampled. That matters twice: a shadow sees a coherent +// access pattern for the keys it tracks rather than a random scatter with no +// reuse, and every shadow sharing one sampler measures the same sub-workload, +// which is what makes their hit rates comparable. // -// The seed is drawn per cache rather than fixed, so the sampled subset differs -// between processes and cannot be predicted or targeted by a caller. +// The seed is per cache, so the subset cannot be predicted or targeted. // -// Sampled counts are never scaled back up to full-traffic magnitude before -// reaching the bandit. Scaling would restore magnitude while inventing -// confidence, handing a Beta posterior twenty times the evidence that was -// actually collected. Instead every arm, the active policy included, is -// measured over this same sampled substream, so the arms carry equal and -// honest evidence and remain directly comparable. +// Sampled counts are never scaled back up before reaching the bandit: that +// would restore magnitude while inventing confidence. Every arm, the active +// policy included, is measured over this same substream instead. type keySampler[K comparable] struct { seed maphash.Seed // threshold is the exclusive upper bound on a key's hash for it to be in @@ -70,18 +64,14 @@ func (s *keySampler[K]) sampled(key K) bool { } // scaledCapacity returns the miniature capacity corresponding to sampling rate -// of a cache of the given size, holding the identity capacity/size == rate. +// of a cache of the given size, holding capacity/size == rate. // -// Unlike shadowCapacity it applies no floor. The floor exists to stop a cache -// from being built with a miniature too small to measure, and it works by -// raising the sample rate to match. After construction the rate can no longer -// move, so applying the floor alone would leave shadows running at a capacity -// larger than their share of the traffic - and a shadow of capacity C fed an -// r-sampled stream simulates a cache of C/r. Every shadow would then simulate a -// larger cache than the active policy actually is and report a better hit rate -// for that reason alone, which is a systematic bias against whichever policy is -// active. A miniature that is merely small is noisy; one that is inconsistent -// with its rate is wrong, so the identity wins. +// Unlike shadowCapacity it applies no floor. A shadow of capacity C fed an +// r-sampled stream simulates a cache of C/r, so raising the capacity without +// raising the rate would have every shadow simulate a larger cache than the +// active policy is and report a better hit rate for that reason alone. A +// miniature that is merely small is noisy; one inconsistent with its rate is +// wrong. func scaledCapacity(size int, rate float64) int { if size <= 0 || rate >= 1 { return size @@ -98,16 +88,14 @@ func scaledCapacity(size int, rate float64) int { return capacity } -// shadowCapacity returns the capacity a shadow policy should run at to -// simulate a full-size cache of nominalCap over the sampled substream, and the -// effective rate that capacity corresponds to. +// shadowCapacity returns the capacity a shadow should run at to simulate a +// full-size cache of nominalCap over the sampled substream, and the effective +// rate that capacity corresponds to. // -// A cache of capacity rate*N fed an rate-sampled stream approximates a cache -// of capacity N fed the full stream, so the capacity has to shrink with the -// rate for the estimate to mean anything. A floor guards the degenerate end: -// a five-entry miniature measures noise, so when rate*nominalCap falls below -// minCapacity the rate itself is raised (not just the capacity) to keep the -// simulation identity intact, up to the point where sampling disables itself. +// The capacity shrinks with the rate for the estimate to mean anything. When +// rate*nominalCap falls below minCapacity the rate itself is raised, not just +// the capacity, keeping the identity intact -- up to the point where sampling +// disables itself. func shadowCapacity(nominalCap int, rate float64, minCapacity int) (capacity int, effectiveRate float64) { if nominalCap <= 0 || rate >= 1 { return nominalCap, 1 diff --git a/settings.go b/settings.go index f7f3e01..227bff8 100644 --- a/settings.go +++ b/settings.go @@ -14,25 +14,13 @@ type Settings struct { // applies both, and whichever comes first ends the epoch. EpochDuration time.Duration - // EpochRequests ends an epoch every N Get calls instead of on a clock. - // - // Wall-clock epochs make a cache's behaviour depend on how fast the - // machine runs it: replaying one trace twice re-evaluates a different - // number of times, so the hit rate moves between runs and cannot be - // compared with anything. Counting requests removes the clock from the - // measurement entirely - the same trace produces the same epochs, the - // same switches and the same hit rate on any machine, which is what a - // benchmark or a regression test needs. - // - // Get is the unit because Get is what produces evidence: hits and misses - // are recorded there and nowhere else, so this counts exactly the - // requests the bandit is shown. A workload that only writes never ends an - // epoch, which is correct - there is nothing to compare policies on. - // - // The epoch runs on the goroutine that happens to make the Nth Get, so - // that one call pays for the switch and any migration it triggers. In - // production prefer EpochDuration, which keeps that work on the - // background goroutine. Zero (the default) disables request counting. + // EpochRequests ends an epoch every N Get calls instead of on a clock, + // which is what makes a replay reproducible on any machine. Get is the + // unit because hits and misses are recorded there and nowhere else, so a + // write-only workload never ends an epoch. The epoch runs on the + // goroutine making the Nth Get, so that call pays for any switch it + // triggers; production should prefer EpochDuration. Zero disables it. + // See docs/benchmarking.md. EpochRequests int64 // EvictPartialCapacityFilling allows policy switching even when the cache // is not yet full. @@ -55,49 +43,31 @@ type Settings struct { SwitchCooldownEpochs int64 // MinEpochRequests is the number of requests (hits plus misses) both the - // active policy and the candidate must have observed in the measured - // epoch before a switch is allowed, so the cache does not react to a - // handful of samples. Zero (the default) imposes no minimum. - // - // The requests counted are the ones the bandit sees, which under - // ShadowSampleRate means sampled requests: at a rate of 0.05 a threshold - // of 100 is reached after roughly 2000 real requests. + // active policy and the candidate must have observed in the measured epoch + // before a switch is allowed. These are the requests the bandit sees, so + // under ShadowSampleRate they are sampled ones. Zero imposes no minimum. MinEpochRequests int64 // ShadowSampleRate is the fraction of the keyspace, in (0,1], that shadow - // policies track. Shadows exist only to estimate a hit rate, and a hit - // rate can be estimated from a sample: at 0.05 a shadow skips 95% of the - // operations it would otherwise mirror, which is where the bulk of the - // adaptive layer's overhead goes. - // - // Shadows shrink with the rate so each remains a faithful miniature of a - // full-size cache, and every shadow samples the same keys so their hit - // rates stay comparable. The active policy still serves every key; only - // the measurement is sampled, and it is sampled for the active policy too - // so that all arms carry equally weighted evidence. - // - // Zero (the default) means 1: shadows mirror every key, which is the - // behaviour of earlier versions. + // policies track, and where most of the adaptive layer's overhead goes. + // Shadows shrink with the rate to stay faithful miniatures, and every + // shadow samples the same keys so their hit rates stay comparable. The + // active policy still serves every key; only its measurement is sampled, + // so all arms carry equally weighted evidence. Zero means 1, no sampling. + // See docs/configuration.md. ShadowSampleRate float64 // ObserveOnly runs the cache as a measurement instrument: every policy is - // still measured each epoch and reported to the bandit, but the active - // policy never changes and no migration ever happens. - // - // This is the zero-risk way to adopt the library. The cache behaves - // exactly like the single policy you gave it first, while Advice() answers - // the question that is otherwise expensive to ask: would a different - // eviction policy serve this traffic better, and by how much. Once the - // answer is in, either switch to that policy directly or turn this off and - // let the bandit do it. + // measured and reported each epoch, but the active policy never changes + // and no migration happens, so the cache behaves exactly like the policy + // it was built with. Advice() reports what the others would have served. + // See docs/advisor-mode.md. ObserveOnly bool - // MinShadowCapacity is the floor on a shadow's miniature capacity. A - // miniature of a handful of entries measures noise rather than a policy, - // so when the sample rate would shrink a shadow below this floor the - // effective rate is raised instead, up to the point where sampling - // disables itself entirely. Zero (the default) applies - // DefaultMinShadowCapacity. + // MinShadowCapacity is the floor on a shadow's miniature capacity. When + // the sample rate would shrink a shadow below it the effective rate is + // raised instead, up to the point where sampling disables itself. Zero + // applies DefaultMinShadowCapacity. MinShadowCapacity int } diff --git a/shadow.go b/shadow.go index b00d66b..0bfbd4d 100644 --- a/shadow.go +++ b/shadow.go @@ -1,42 +1,22 @@ package ascache -// fanOutReadLocked feeds one lookup to every shadow policy, which is what -// makes a shadow's measurement mean anything. +// fanOutReadLocked feeds one lookup to every shadow policy. A shadow that +// misses fills itself with the zero value, which is what makes its hit rate +// describe the policy rather than the incumbent's miss stream -- see +// docs/design.md, which records how far that measurement drifts without it. // -// A shadow that misses is filled, exactly as the caller would fill a -// read-through cache that missed. That fill is the whole point: without it a -// shadow can only ever acquire a key on a request the ACTIVE policy also -// missed, because a read-through caller calls Add only then - so the better -// the incumbent performs, the less the shadows are allowed to learn, and their -// hit rates stop describing the policies at all. -// -// The distortion that causes is not a small bias. Measured on a cyclic -// workload with a 94%-hit incumbent, shadows holding policies that truly serve -// 0.00% reported over 90%: starved of inserts, their contents go static, and a -// static cache covering most of a small keyspace looks excellent. The sign and -// size depend on which arm is incumbent, so it does not cancel in the -// comparison - it inverts it, and Advice() recommended switching away from the -// best arm to the worst. -// -// Shadows store the zero value, never the caller's, so filling one costs a key -// and its eviction bookkeeping and no more. -// -// It must be called while at least the read lock is held. Each policy is -// independently synchronised, so mutating one here is safe: the shadow Get -// above already mutates recency and frequency state the same way. +// Caller must hold at least the read lock. Each policy is independently +// synchronised, so mutating one here is safe. func (c *AdaptiveCache[K, V]) fanOutReadLocked(key K) { for _, policy := range c.policies { if policy.GetType() == c.activePolicy { continue } - // The source of an open gradual migration is not a shadow yet. It is - // the only holder of every value not promoted so far, and promoteLocked - // reads those values back out with Peek. Filling it with a zero here - // would put a value nobody stored where a real one is still pending, - // and Peek cannot tell the two apart - so the zero would be promoted - // into the active policy and served to a caller as a hit. It is fed - // like any other shadow again once the window closes and it is demoted. + // The source of an open gradual window is not a shadow: it still holds + // every value not yet promoted, and promoteLocked reads those back with + // Peek, which cannot tell a real pending value from a zero written + // here. Filling it would promote that zero and serve it as a hit. if c.migrating && policy.GetType() == c.migrateFrom { continue } @@ -49,38 +29,17 @@ func (c *AdaptiveCache[K, V]) fanOutReadLocked(key K) { } // demoteLocked puts a policy that has just stopped being active onto shadow -// duty: it releases the policy's hold on real values and shrinks it to the -// miniature capacity it simulates at. -// -// Values are dropped rather than kept because a demoted policy no longer -// serves anyone. Its keys still matter - they are the eviction bookkeeping -// that makes its hit-rate estimate meaningful - so entries are rewritten to -// the zero value instead of being purged. Rewriting in Keys() order preserves -// the ordering the policy maintains: for a recency policy the oldest-to-newest -// walk re-establishes the same recency order, and for a frequency policy every -// surviving key gains exactly one access, which leaves the relative ordering -// untouched. -// -// Keys outside the sample are removed outright, so what remains is the -// substream every other shadow is measuring. +// duty: entries are rewritten to the zero value, keys outside the sample are +// removed, and the policy shrinks to its miniature capacity. Keys survive +// because they are the eviction bookkeeping its hit-rate estimate rests on. // -// The ordering claim above does not hold for every policy, and the exception -// is worth knowing. It assumes a write is either an ordering event (recency) -// or a counted access (frequency). For the FIFO-queue policies it is neither -// of those things cleanly: SIEVE treats a write as setting the entry's visited -// bit, and that bit is its whole eviction criterion, so rewriting every key -// sets it on every key and erases the ordering rather than preserving it; -// S3-FIFO's counter saturates at three, so a key already at the cap gains -// nothing while a key at zero gains one, compressing the ordering instead of -// shifting it uniformly. The effect is a bias in the demoted policy's first -// shadow epochs, not a standing loss - the queues themselves are untouched and -// the bits are rewritten by ordinary traffic soon after - but a policy whose -// eviction state is a single saturating bit per entry should not be demoted -// this way without measuring what it costs. +// The rewrite walks Keys() so a recency policy re-establishes the same order +// and a frequency policy gains one access on every surviving key, leaving the +// relative order intact. That reasoning does not hold for the FIFO-queue +// policies; docs/policies.md records what demotion costs them. // -// It must be called while the write lock is held, and only after the new state -// has been published, so a reader holding a stale view cannot observe a value -// being dropped and mistake the zero for real data. +// Caller must hold the write lock, and must have published the new state +// first, so no reader can observe a value being dropped. func (c *AdaptiveCache[K, V]) demoteLocked(policyType PolicyType) { policy, ok := c.policies[policyType] if !ok { @@ -100,17 +59,14 @@ func (c *AdaptiveCache[K, V]) demoteLocked(policyType PolicyType) { policy.Resize(capacity) } - // Whatever this policy measured in its previous role was measured at a - // different capacity, and over all traffic rather than the sample. Carrying - // those counts into its first shadow epoch would misreport it to the bandit. + // The previous role measured a different capacity over different traffic. policy.ResetStats() } -// promoteLockedCapacity restores a policy to its full nominal capacity as it -// takes over active duty. The caller purges it afterwards - a policy arriving -// from shadow duty holds only zero values - so no real data is resized away. -// -// It must be called while the write lock is held. +// promoteLockedCapacity restores a policy to its nominal capacity as it takes +// over active duty. The caller purges it afterwards -- a policy arriving from +// shadow duty holds only zero values -- so no real data is resized away. +// Caller must hold the write lock. func (c *AdaptiveCache[K, V]) promoteLockedCapacity(policyType PolicyType) { policy, ok := c.policies[policyType] if !ok { @@ -122,32 +78,21 @@ func (c *AdaptiveCache[K, V]) promoteLockedCapacity(policyType PolicyType) { } } -// switchLocked applies a policy change end to end: it restores the incoming -// policy to full capacity, migrates data according to the configured strategy, -// makes it active, and puts the outgoing policy onto shadow duty. +// switchLocked applies a policy change end to end: restore the incoming +// policy's capacity, migrate data into it, make it active, put the outgoing +// policy onto shadow duty. // -// The order of those steps is load-bearing, and the rule generalises: +// The order is load-bearing, and the rule generalises: // // Every mutation of a policy must happen while that policy is not the // active one. // -// So the incoming policy is resized and migrated into before it is made -// active, and the outgoing policy has its values dropped only after it has -// stopped being active. Reversing either half would let a caller observe a -// policy mid-rewrite - most damagingly, read a dropped value and take the zero -// for real data. The rule is what keeps that impossible, and it is what any -// future move to lock-free reads would rest on: a reader can only ever hold a -// policy that is not being mutated. -// -// The capacity is restored before migrateData runs for the same reason it is -// restored at all: a warm migration must copy into a full-size policy rather +// Reverse either half and a caller can read a policy mid-rewrite, most +// damagingly taking a dropped value's zero for real data. Capacity is restored +// before migrateData so a warm migration copies into a full-size policy rather // than a miniature that would evict most of what it is handed. // -// Demotion of the outgoing policy is deferred when a gradual window opens, -// because that window serves promotions out of the outgoing policy's real -// values; closeMigrationLocked performs it once the window closes. -// -// It must be called while the write lock is held. +// Caller must hold the write lock. func (c *AdaptiveCache[K, V]) switchLocked(from, to PolicyType) { // Abandon any window still open from a previous switch, demoting its // source now that nothing will promote out of it again. @@ -157,8 +102,7 @@ func (c *AdaptiveCache[K, V]) switchLocked(from, to PolicyType) { c.migrateData(from, to) c.activePolicy = to - // Both policies just changed role, so what they measured in the previous - // one no longer describes them. Advice compares them from here. + // Both changed role, so neither's previous measurements describe it now. delete(c.tenureStats, from) delete(c.tenureStats, to) @@ -167,11 +111,9 @@ func (c *AdaptiveCache[K, V]) switchLocked(from, to PolicyType) { } } -// closeMigrationLocked ends a gradual migration window and puts the source -// policy onto shadow duty, the demotion that was deferred while the window -// still needed the source's real values. -// -// It must be called while the write lock is held. +// closeMigrationLocked ends a gradual migration window and demotes the source, +// which switchLocked deferred while the window still needed its real values. +// Caller must hold the write lock. func (c *AdaptiveCache[K, V]) closeMigrationLocked() { source, wasMigrating := c.migrateFrom, c.migrating c.clearMigrationState() @@ -189,9 +131,9 @@ func (c *AdaptiveCache[K, V]) initShadowDutyLocked(rate float64, minCapacity int c.nominalCap = make(map[PolicyType]int, len(c.policies)) c.shadowCap = make(map[PolicyType]int, len(c.policies)) - // The sample must be identical for every shadow or their hit rates are not - // comparable, so one effective rate is derived from the smallest policy: - // that is the capacity most at risk of shrinking into noise. + // One rate for every shadow, or their hit rates are not comparable. It is + // derived from the smallest policy, the one most at risk of shrinking into + // noise. minNominal := 0 for policyType, policy := range c.policies { capacity := policy.Cap()