B2: switch gates, epoch clock liveness, sample attribution, gradual window cap - #16
Merged
Merged
Conversation
MinHitRateImprovement compares the candidate's hit rate with the active
policy's for the epoch just measured. hitRate reports 0 for an arm with no
requests, so an active policy that saw no traffic read as one serving
nothing, and a candidate with a single hit cleared any threshold against it.
The gate meant to stop switches on thin evidence admitted one made on no
evidence at all about the policy being replaced.
With the gate configured, a switch is now held when either arm's epoch is
empty. The candidate edge already held on arithmetic -- an empty candidate
scores 0 and loses -- and is made explicit so it stays true whatever hitRate
returns for an empty epoch.
TestSwitchStability_ZeroTrafficOnActiveBlocksSwitch failed against this
commit's parent ("an active policy with no traffic cannot be out-performed")
and passes here. ZeroTrafficOnCandidateBlocksSwitch passed before and after;
it pins the edge so the fix cannot invert it. All nine TestSwitchStability
tests pass, and the root package passes under -race -short.
countRequest ran an epoch when Add(1) returned exactly the limit, then
subtracted the limit. Its comment claimed exactly one caller per epoch sees
the count equal the limit. That holds only if nobody else increments between
one caller's comparison and its subtraction. When they do, the count passes
the limit unobserved, nothing ever subtracts again, and every later Get sees
a count above the limit and returns: request-driven epochs stop for good. At
EpochRequests 1 a single concurrent Get opens the window.
TestEpochRequests_ClockSurvivesContention runs 16 goroutines x 2000 Gets at
EpochRequests 1, then 10 uncontended Gets, each of which must end an epoch.
Against this commit's parent it failed 1 run in 10 without the race detector
and 5 runs in 5 with it, the detector widening the window. A failing run:
2 selections before, 2 after 10 uncontended Gets
-- two epochs out of 32,000 requests, then none.
The counter now only ever increases, and an epoch runs on the call whose
increment returns a multiple of the limit. Add hands every caller a distinct
value, so each multiple is observed by exactly one caller and there is no
threshold to step past. Requests arriving mid-crossing still count toward
the next epoch.
After the change the test passes 20 runs in 20 under -race and 50 in 50
without it; the epoch tests pass 3 times under -race, and the root package
passes under -race -short.
…sumed it
Nothing in golang-lru promises an order for Keys(), and code here depends on
one. demoteLocked rewrites a demoted policy to zero values by walking Keys();
for an LRU that re-establishes the learned recency order only because Keys()
runs oldest to newest. docs/policies.md states that 2Q and ARC return
opposite groupings, which is why AdaptedCache.Resize replays every entry.
Three canaries now assert exact sequences after one access that moves an
entry, so a changed order fails rather than matching insertion order by
coincidence:
TestKeysOrder_LRUIsOldestToNewest a,b,c then Get(a) -> b,c,a
TestKeysOrder_TwoQueueIsFrequentThenRecent a,b,c then Get(b) -> b,a,c
TestKeysOrder_ARCIsRecentThenFrequent a,b,c then Get(b) -> a,c,b
(policies/arc)
The expected sequences were derived from golang-lru's source -- 2Q's Keys
appends its recent list to its frequent one, ARC's appends T2 to T1 -- and
match what upstream returns. Mutation-checked: with a temporary CacheWrapper
Keys that reverses the slice, all three fail; each failed at its first
assertion, the insertion order, so the post-access assertion was not
separately exercised by that mutation. With it removed, wrapper.go matches
HEAD and all three pass.
AdaptedCache.Keys was documented as "oldest first", which is false for both
caches it serves; it now says what they return. enforceCapacityLocked
removes keys[0] after a failed rebuild; its comment now says that is some
entry rather than the oldest, and why that is acceptable. demoteLocked's
comment names the canary its recency argument rests on.
gofmt, go vet and golangci-lint are clean in the root, policies and
policies/arc modules, and all three pass go test -race -short.
While a MigrationGradual window is open every Get takes the write lock, so
the window serialises reads for as long as it lasts. Nothing closed it except
promoting or removing every pending key, Purge, the next switch, or the next
epoch boundary; on a long epoch against a workload that stops touching the
pending keys, that is most of the epoch.
MigrationMaxRequests caps the window at a number of Get calls. When the Get
reaching the cap has been served, the window closes and the source is
demoted: keys not yet promoted are abandoned, and a later Get for one is a
miss, the same outcome as MigrationCold. Only Get counts, because Get is what
takes the write lock; Add drains one pending key per call and shortens the
window on its own. The count is reset whenever window state is cleared, so
every window starts from zero, and it needs no atomic -- every Get inside a
window already holds the write lock.
Zero sets no cap and keeps the previous behaviour exactly: the window closes
at the next epoch boundary. A negative value is rejected with
ErrInvalidMigrationMaxRequests.
Against the field and validation without the counting,
TestMigrationGradual_MaxRequestsClosesWindow ("the Get reaching the cap must
close the window") and MaxRequestsAbandonsPendingKeys failed; with it both
pass. ZeroMaxRequestsSetsNoCap -- 1000 Gets, window still open -- and
RejectsNegativeMigrationMaxRequests passed before and after. Every gradual
and migration test passes 3 times under -race, the root package passes under
-race -short, and go vet and golangci-lint are clean.
docs/configuration.md documents the field and what the cap costs, and its
strategy table now says outright that a gradual window serialises reads.
TestGradualMigration_NeverServesAZeroFromTheSource guards the fix for a gradual window that served a shadow zero as a hit, and it checked only Get. Peek and Contains are the other ways a caller asks whether a key is present, and nothing stopped a change from letting them reach the same placeholders. The test now checks both while the window is still open, before the Gets that would promote the real values. The discriminating half is the keys nobody stored: the only copies of those anywhere are zero placeholders written by the fan-out, so Peek and Contains must report them absent. Checking only the real keys would not have discriminated -- a placeholder for a real key appears in the source only on a Get for that key, and that same Get promotes it. Mutation-checked. With the source guard removed from fanOutReadLocked and Peek and Contains falling back to the source while a window is open, the new assertions fail for the four placeholders the capacity-4 source still holds (Peek and Contains on fresh8..fresh11), alongside the existing Get assertions. With both files restored, which git diff confirms, the test passes 5 runs in 5 under -race.
policies.NewTTL expires entries against time.Now, both when an entry is written and when it is read. Its hit rate therefore depends on how fast traffic arrives, not only on the order of requests, and a replay of the same trace gives a different answer on a faster or slower machine once the TTL is comparable to the run. The policy table described it only as "expiry as well as recency". It now says the expiry is wall-clock and that a replay is reproducible only while the TTL is far longer than the run -- which is the condition the evidence suite relies on, building the arm with a one-hour TTL.
The active arm's evidence is counted on the cache, in activeSampledHits and activeSampledMisses, and read-and-reset when an epoch collects. Nothing cleared it on a switch. Since #14 the epoch releases the lock between collecting and applying, and every Get arriving while the bandit decides is served by the policy still active and counted into those counters. When the bandit then switched, the next epoch reported all of it as the evidence of the policy that had just become active -- requests it never served. The window was as long as the bandit took to decide; before #14 it existed only as the gap between one Get's lookup and its counter update. switchLocked now clears both counters, as demotion already clears the outgoing policy's own. Samples from that gap are dropped rather than moved: they describe a policy in a role it no longer holds. A residue is documented rather than fixed. On the read-lock path the counter update runs after the lock is released, so a Get already past its lookup when the switch lands still credits the new policy; that is bounded by the Gets in flight at that instant, not by the bandit's duration. TestEpoch_OutgoingSamplesAreNotCreditedToIncomingPolicy parks the first epoch in the bandit, serves 5 Gets from LRU, lets the bandit switch to LFU, and runs a second epoch with no request reaching LFU. Against this commit's parent it failed 3 runs in 3: "epoch two reported 5 hits and 0 misses" for LFU. It now passes 5 runs in 5 under -race; the epoch, stability, advice, shadow and migration tests pass 3 times under -race, the root package passes under -race -short, and golangci-lint is clean.
Every evidence table was a whole-run average, which hides where a switch's
cost falls. TestSwitchWarmupCost scripts one switch, from LRU to LFU at
request 100,000 of the suite's zipf workload (200,000 requests, cache 500),
and reports hit rate in windows measured from the switch. The bandit is
scripted so every strategy switches at the same request; two runs produced
identical tables.
0-1000 1000-5000 5000-20000 20000-100000
LFU all along (no warm-up) 77.00% 74.28% 74.59% 73.90%
LRU, never switched 70.50% 67.90% 67.58% 66.82%
cold 59.50% 69.45% 72.93% 73.52%
warm 70.50% 70.03% 72.91% 73.55%
gradual 70.40% 70.12% 72.88% 73.52%
gradual, capped at 10 Gets 59.90% 69.47% 72.93% 73.52%
gradual, capped at 100 Gets 63.90% 69.58% 72.99% 73.53%
gradual, capped at 1000 Gets 70.40% 70.12% 72.88% 73.52%
Cold's first 1,000 requests are 11.0 points below not switching and 17.5
below an LFU that never warmed up; warm and gradual show no dip. No strategy
reaches the always-LFU rate even 20,000-100,000 requests later (73.52-73.55%
against 73.90%).
The first run had only the 1,000-Get cap, and that row matched uncapped
gradual exactly -- so it said nothing about what the cap costs. Rows for 10
and 100 were added to find where it bites: 10 behaves like cold, 100 costs
6.5 points in the first 1,000 requests, and the window empties on its own
somewhere between 100 and 1,000 Gets, because under read-through traffic
every miss is an Add and every Add drains a pending key.
The stage spec asked for these numbers in docs/design.md. They went into
docs/evidence.md instead, where every other measured claim lives, with a
summary and a link in configuration.md next to the setting they describe.
All internal links in docs/ and README.md resolve; go vet and golangci-lint
are clean in bench.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stage B2 of the cleanup plan: the technical fixes left after the bandit moved
out of the write lock (#14). One commit per subtask, each with its test.
Two items the stage listed as godoc caveats turned out to be bugs, both
with reproducing tests. They are the ones worth reading first.
The two bugs
14a7af2The request-driven epoch clock could stop for good.countRequestran an epoch whenAdd(1)returned exactly the limit, thensubtracted the limit. If other callers incremented between one caller's
comparison and its subtraction, the count passed the limit unobserved and
nothing ever subtracted again. At
EpochRequests: 1, one concurrentGetopens the window.
TestEpochRequests_ClockSurvivesContentionfailed 1 run in10 without
-raceand 5 in 5 with it; a failing run read "2 selectionsbefore, 2 after 10 uncontended Gets" — two epochs out of 32,000 requests,
then none. An epoch now runs on each multiple of the limit; the counter only
increases. After: 20/20 under
-race, 50/50 without.c73b023The outgoing policy's samples were credited to the incoming one.The active arm's evidence lives in cache-level counters that nothing cleared
on a switch. Since #14 the epoch releases the lock while the bandit decides,
so every
Getserved by the old policy in that window was reported nextepoch as the new policy's evidence. #14 widened this from one
Get's gap tothe bandit's full duration.
TestEpoch_OutgoingSamplesAreNotCreditedToIncomingPolicyfailed 3/3 with "epoch two reported 5 hits and 0 misses" for a policy that
had served nothing.
switchLockednow clears the counters; the residue — aGetalready past its lookup when the switch lands — is documented andbounded by in-flight
Gets.Everything else
031a9e3MinHitRateImprovementholds the switch when either arm saw no traffic (an empty active epoch read as 0% and lost to any candidate)febf104Keys()order for LRU, 2Q and ARC;AdaptedCache.Keysgodoc no longer claims "oldest first"Keys()fails all three62ff099Settings.MigrationMaxRequestscaps a gradual window inGetcalls; zero keeps the old behaviour9d1531fPeekandContainsPeek/Containsreading the source fails it447b50e5f8ef4eTestSwitchWarmupCost: hit rate in windows after a scripted switch, per strategyMeasured in
5f8ef4e(zipf, 200k requests, cache 500, LRU to LFU at request100,000): cold's first 1,000 requests are 11.0 points below not switching;
warm and gradual show no dip; no strategy reaches the always-LFU rate even
80,000 requests later (73.52–73.55% vs 73.90%). A 10-
Getcap behaves likecold, 100 costs 6.5 points, and 1,000 is never reached here.
Deviations from the stage spec
for one commit per subtask with its test, which rules out a test-only first
commit. Each behaviour fix states in its message what failed against its
parent.
MigrationMaxRequestsdefaults to zero = no cap. The spec asked for a"reasonably conservative" default and, in the reviewer checklist, for the
zero value to keep current behaviour. Resolved for the checklist by the
maintainer before the stage started.
Getcounts toward the cap, sinceGetis what takes the write lock.docs/evidence.md, notdocs/design.md,because every other measured claim lives there.
configuration.mdcarries asummary and a link.
Not in this change
PeekandContainsread only the active policy, so during a gradual windowthey report a pending key absent while
Getwould return it. No zero leaks —the extended regression test pins that — but the three disagree. Making them
consult the source is a public behaviour change and is left for a decision.
Verification
Class G. Per the stage:
stability.go. Given the two bugs, alsoepoch.go(countRequest) andshadow.go(switchLocked).