From 24783038747abda0180bc4751dc5e6ca238135c6 Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Thu, 24 Sep 2026 09:18:29 +0800 Subject: [PATCH 1/5] feat(keys): reuse expired metric slots and stop broadcasting on return Two things made a low-frequency series coming back expensive. It got a brand new slot, so key_count only ever grew -- the dump in apache/apisix#13658 had 747,970 dead index entries against 28 live series -- and it bumped delete_count, which sends every worker through a full sync of key_count slots on its request path. Measured on the shape of a gateway pod (10 workers, 512m dict, the three exporter.lua metrics, 140k label combinations, 200 req/s): one such return took the worker set from 14.9% to 30.2% CPU, and 50 of them over ten seconds put three workers at 98%, 93% and 88% of a core. ttl() is what makes reuse possible. get() reports a slot whose node is merely past its ttl and a slot whose node has been reclaimed alike, as nil, while ttl() returns a negative number for the first and "not found" for the second. A slot in the first state still belongs to its own key, which can take it back in place; only a slot in the second may go to another key. So a key takes its own slot back in place, and a key that needs one takes a number its worker has seen reclaimed -- no search, no shared cursor. Renewal reads the slot instead of the counters, which is one read instead of two and is also what keeps a stale index entry from extending another key's ttl. delete_count is no longer bumped, and the request path no longer reads it, so an external bump cannot make it scan either. Both kinds of write land below the other workers' self.last, where an incremental sync would not look again, so each publishes its slot number into a ring that the scrape follows; the ring is created up front and rewritten with fixed-width values, so publishing cannot fail on a full dict and leave the scrape walking every slot. ensure_key_count() keeps a slot taken back in place from sitting above key_count. After: 7.8% steady, 2.2% on a return, 4.5% under 50 bumps. Correctness checked in the scraping process against a walk of the dict, on 10 workers, at 300k and 1.5M slots: no duplicates and nothing missing. rfcs/0001-slot-reuse-and-bounded-reclaim.md has the design, the scenario matrix and the measurements. --- CHANGELOG.md | 7 + prometheus_keys.lua | 533 +++++++++++++++----- prometheus_test.lua | 297 +++++++++-- rfcs/0001-slot-reuse-and-bounded-reclaim.md | 303 +++++++++++ 4 files changed, 969 insertions(+), 171 deletions(-) create mode 100644 rfcs/0001-slot-reuse-and-bounded-reclaim.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 21f35ea..bb5bac1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,13 @@ of changes. `flush_expired()` call holds the dict lock for a whole backlog, and let callers take the reclamation over with the `auto_flush_expired` option and `prometheus:flush_expired()` (#23). +- Reuse the index slots of metrics that have expired instead of handing out a + new number every time, and stop broadcasting to the other workers when a + metric comes back: renewing a metric no longer reads the shared counters, so + one low-frequency series returning no longer sends every worker through a + full sync on its request path (apache/apisix#13658). The slot count now + tracks the number of live series instead of growing for ever under label + churn. See `rfcs/0001-slot-reuse-and-bounded-reclaim.md`. ## 1.0.0 diff --git a/prometheus_keys.lua b/prometheus_keys.lua index ee56683..5a22511 100644 --- a/prometheus_keys.lua +++ b/prometheus_keys.lua @@ -28,6 +28,29 @@ local FLUSH_EXPIRED_MAX_BATCHES = 30 -- Pause between batches, so the lock is not taken back to back. local FLUSH_EXPIRED_BATCH_DELAY = 1 +-- Slot numbers this worker keeps for reuse after seeing them reclaimed. They +-- go to the next keys that need one, so a registration costs the same single +-- write it always did: a number taken in the meantime just fails that write. +local FREE_SLOTS_KEEP = 1024 + +-- Reclaimed numbers tried before falling back to a fresh slot past key_count. +local FREE_SLOT_ATTEMPTS = 4 + +-- Reused slot numbers kept in the shared dict for a scrape to pick up. A +-- scrape further behind than this walks every slot instead. The entries are +-- created up front and always rewritten with a value of the same length, so +-- publishing one is an in-place write that cannot fail for want of memory -- +-- on a full dict a write that had to allocate would either evict a metric or +-- be dropped, and a dropped one costs the scrape a walk over every slot. +local REUSE_RING_MAX = 8192 +local REUSE_RING_MIN = 256 +local REUSE_RING_BYTES_PER_ENTRY = 65536 +local REUSE_ENTRY_FORMAT = "%012d:%09d" + +-- Attempts at raising key_count above a slot number. Concurrent raises can +-- only overshoot, so this is about retrying a lost race, not about progress. +local KEY_COUNT_RAISE_ATTEMPTS = 3 + -- check and remove expired keys local function remove_expired_keys(_, self) @@ -43,38 +66,76 @@ function KeyIndex.new(shared_dict, prefix, remove_expired_keys_interval, self.key_prefix = prefix .. "key_" self.delete_count = prefix .. "delete_count" self.key_count = prefix .. "key_count" + self.reuse_count = prefix .. "reuse_count" + self.reuse_slot = prefix .. "reuse_slot_" + self.reuse_ready = prefix .. "reuse_ring" + self.seen_reuse = 0 + self.free_slots = {} + self.free_n = 0 self.last = 0 self.deleted = 0 self.not_expired_index = 1 self.keys = {} self.index = {} + self.hidden = {} self.expire_keys = {} + -- One entry per 64 KiB of the dict, so the trail costs about 0.2% of it and + -- stays useful on the dicts big enough for a full scan to be expensive. + local capacity = self.dict.capacity and self.dict:capacity() or 0 + self.ring_size = math.max(REUSE_RING_MIN, + math.min(REUSE_RING_MAX, math.floor(capacity / REUSE_RING_BYTES_PER_ENTRY))) + self:init_reuse_ring() + ngx.timer.every(remove_expired_keys_interval or 600, remove_expired_keys, self) return self end + +-- Creates the trail entries once, so that publishing one later never has to +-- allocate. Whichever process gets here first does it; the others see the +-- marker and skip the writes. +function KeyIndex:init_reuse_ring() + local ok = self.dict:add(self.reuse_ready, 1) + if not ok then + return + end + + local blank = string.format(REUSE_ENTRY_FORMAT, 0, 0) + for i = 0, self.ring_size - 1 do + self.dict:add(self.reuse_slot .. i, blank) + end +end + -- check and remove expired keys function KeyIndex:remove_expired_keys() + -- Reclaiming first means the scan below sees the final state of every slot + -- and can give up the numbers that are really gone in the same round. + -- Callers that schedule flush_expired() themselves -- in a single process + -- rather than in every worker -- turn this off with auto_flush_expired. + if self.auto_flush_expired then + self:flush_expired() + end + for i, _ in pairs(self.expire_keys) do - -- Read i-th key. If it is nil or ttl is < 0, it means it was expired + -- A slot is in one of three states, and only ttl() tells them apart -- + -- get() reports the last two alike, as nil: + -- + -- (1) live: ttl > 0 + -- (2) past its ttl, node still in the dict: ttl < 0 + -- (3) node physically reclaimed: "not found" + -- + -- In state (2) the slot can still be taken back in place by its own key, + -- so its number is kept; only the key stops being listed, because its + -- value has expired. In state (3) the number is gone and may be reused by + -- another key, so every reference to it has to go. local ttl, err = self.dict:ttl(self.key_prefix .. i) - if not (ttl and ttl >= 0 or err and err ~= "not found") then - if self.keys[i] then - self.index[self.keys[i]] = nil - self.keys[i] = nil - end - self.expire_keys[i] = nil + if err == "not found" then + self:clear_slot(i) + elseif ttl and ttl < 0 then + self:hide_slot(i) end end - - -- The loop above only drops worker-local references, so the expired entries - -- still have to be reclaimed from the dict itself. Callers that schedule - -- flush_expired() themselves -- in a single process rather than in every - -- worker -- turn this off with auto_flush_expired. - if self.auto_flush_expired then - self:flush_expired() - end end @@ -135,8 +196,7 @@ function KeyIndex:sync_range(first, last) -- Read i-th key. If it is nil, it means it was deleted by some other thread. local key = self.dict:get(self.key_prefix .. i) if key then - self.keys[i] = key - self.index[key] = i + self:set_slot(i, key) -- if it is nil and ttl not is 0, set expire_keys map if not self.expire_keys[i] then @@ -146,17 +206,149 @@ function KeyIndex:sync_range(first, last) end end elseif self.keys[i] then - self.index[self.keys[i]] = nil - self.keys[i] = nil - self.expire_keys[i] = nil + -- The slot holds no live key, which is all a scrape needs to know, so it + -- is only hidden here -- one read per slot. Telling "past its ttl" from + -- "node reclaimed" costs a second read and decides whether the number + -- can be given up, so that belongs to remove_expired_keys(), which runs + -- on a timer rather than on every scrape. + self:hide_slot(i) end end self.last = last end + +-- self.keys (slot -> key) and self.index (key -> slot) are two views of one +-- mapping, and every local change goes through the three helpers below so the +-- views cannot drift apart. Leaving the previous occupant's index entry behind +-- would point a later add() at a slot that is no longer its own, and dropping +-- the index entry of a key that has since moved would hide a live key from +-- list(). +function KeyIndex:set_slot(i, key) + local old = self.keys[i] + if old and old ~= key and self.index[old] == i then + self.index[old] = nil + end + self.keys[i] = key + self.index[key] = i + self.hidden[i] = nil +end + + +-- State (2), past its ttl with the node still in the dict: the key stops being +-- listed, because its value has expired, but the slot number is kept so that +-- add() can take it back in place. +function KeyIndex:hide_slot(i) + if self.keys[i] then + self.hidden[i] = true + end +end + + +-- State (3), node physically reclaimed: the number may be handed to another +-- key from now on, so every reference to it goes. +function KeyIndex:clear_slot(i) + -- Its number can go to another key now. Keeping it here is what makes reuse + -- free to look for: the next key that needs a slot takes one of these and + -- writes it, exactly as it would write a fresh one. + if self.free_n < FREE_SLOTS_KEEP then + self.free_n = self.free_n + 1 + self.free_slots[self.free_n] = i + end + + local key = self.keys[i] + if key and self.index[key] == i then + self.index[key] = nil + end + self.keys[i] = nil + self.hidden[i] = nil + self.expire_keys[i] = nil +end + + +-- Raises key_count above a slot number. A slot above key_count is invisible to +-- list(), which walks 0..key_count; that happens when key_count -- an ordinary +-- dict entry -- is LRU-evicted and incr() re-creates it below the slots that +-- are already in use. +function KeyIndex:ensure_key_count(idx) + for _ = 1, KEY_COUNT_RAISE_ATTEMPTS do + local n = self.dict:get(self.key_count) or 0 + if n >= idx then + return + end + + -- Concurrent raises can only overshoot, which costs a few empty slots in + -- the next scan. Undershooting would hide the slot, so the result is + -- checked rather than assumed. + local new = self.dict:incr(self.key_count, idx - n, 0) + if not new or new >= idx then + return + end + end +end + + +-- Re-reads one slot. Unlike sync_range it leaves self.last alone: a reused +-- slot can be anywhere below it. +function KeyIndex:sync_slot(i) + local key = self.dict:get(self.key_prefix .. i) + if key then + self:set_slot(i, key) + if not self.expire_keys[i] then + local ttl = self.dict:ttl(self.key_prefix .. i) + if ttl and ttl ~= 0 then + self.expire_keys[i] = true + end + end + elseif self.keys[i] then + self:hide_slot(i) + end +end + + +-- Re-reads the slots written below self.last since the last scrape. The ring +-- holds ":", so an entry that has been overwritten since -- or that +-- a writer had not finished publishing -- is recognised, and the caller walks +-- every slot instead. Re-reading a slot twice is harmless; missing one is not. +function KeyIndex:follow_reuse(reuse) + for seq = self.seen_reuse + 1, reuse do + local entry = self.dict:get(self.reuse_slot .. seq % self.ring_size) + if not entry then + return false + end + + local at, idx = entry:match("^(%d+):(%d+)$") + if tonumber(at) ~= seq then + return false + end + + self:sync_slot(tonumber(idx)) + end + + return true +end + + -- Returns array of all keys. function KeyIndex:list() - self:sync() + -- The scrape is the only caller that needs the whole set of keys, and the + -- only one that must not miss any. Slot numbers are taken back in place and + -- reused between keys, so a slot below self.last can change without + -- key_count or delete_count moving, and an incremental sync would never read + -- it again. Every such write publishes its slot number, so the usual case is + -- re-reading just those; only a scrape that has fallen too far behind, or + -- one that finds the trail incomplete, walks every slot. + local reuse = self.dict:get(self.reuse_count) or 0 + if reuse == self.seen_reuse then + self:sync() + elseif reuse - self.seen_reuse <= self.ring_size and self:follow_reuse(reuse) then + self:sync() + else + self.deleted = self.dict:get(self.delete_count) or 0 + self:sync_range(0, self.dict:get(self.key_count) or 0) + end + self.seen_reuse = reuse + local copy = {} local i = 1 -- Emit a key only from the slot the index currently points at @@ -164,12 +356,9 @@ function KeyIndex:list() -- in two different slots (e.g. when an expired metric is re-added at a new -- slot before the old slot is reclaimed); listing the raw self.keys values -- would emit duplicate metrics. Consulting the index guarantees each key is - -- listed exactly once, at its canonical slot. Iterating self.keys (not - -- 0..self.last) keeps this O(live keys): self.last grows monotonically with - -- every add and is never reclaimed, so a slot range scan would walk every - -- dead slot ever created on long-lived, high-churn workers. + -- listed exactly once, at its canonical slot. for idx, key in pairs(self.keys) do - if self.index[key] == idx then + if not self.hidden[idx] and self.index[key] == idx then copy[i] = key i = i + 1 end @@ -177,6 +366,7 @@ function KeyIndex:list() return copy end + -- Atomically adds one or more keys to the index. -- -- Args: @@ -191,113 +381,210 @@ function KeyIndex:add(key_or_keys, err_msg_lru_eviction, exptime) end for _, key in pairs(keys) do - local retried = false - local repairs = 0 - local repair_forcible = false - while true do - local N = self:sync() - if self.index[key] ~= nil then - -- key already exists, if has exptime, set expire - local expired = false - if exptime then - local ok, err = self.dict:expire(self.key_prefix .. self.index[key], exptime) - if not ok then - if err == "not found" then - -- The slot already expired in the shared dict. Drop the stale - -- local state and bump delete_count so other workers do a full - -- sync and reclaim the slot; without this the old slot lingers in - -- their local self.keys while the metric is re-added at a new slot, - -- desynchronizing the index and causing duplicate metric emission. - -- The dict slot is already gone (expire returned "not found"), so - -- there is no slot to clear here. - local idx = self.index[key] - self.index[key] = nil - self.keys[idx] = nil - self.expire_keys[idx] = nil - self.deleted = self.deleted + 1 - local _, incr_err, forcible = self.dict:incr(self.delete_count, 1, 0) - if incr_err or forcible then - return incr_err or err_msg_lru_eviction - end - expired = true - else - -- Unexpected expire error: the slot may still be live, so leave it - -- as-is rather than re-adding it, which would create a duplicate. - ngx.log(ngx.ERR, "failed to renew expire for key '", key, "': ", - tostring(err)) - end - end - end - if not expired then - if repair_forcible then - -- the key was adopted from an occupied slot after repair - -- increments that forcibly displaced other entries; report the - -- eviction just like the new-slot success path does. - return (err_msg_lru_eviction .. "; key index: adopted key after " .. - "key_count repair: idx=" .. self.key_prefix .. self.index[key] .. - ", key=" .. key) - end - break - end + local err = self:add_key(key, err_msg_lru_eviction, exptime) + if err then + return err + end + end +end + + +-- Registers a single key. Returns nil on success, an error message otherwise. +function KeyIndex:add_key(key, err_msg_lru_eviction, exptime) + local idx = self.index[key] + if idx then + local occupant = self.dict:get(self.key_prefix .. idx) + + -- By far the common case: the slot is ours and live, so only its ttl has + -- to be pushed out, and none of the shared counters are even read. + -- Reading the slot first is also what makes reusing slot numbers safe: an + -- index entry left over from a slot that has since been handed to another + -- key does not match here, where renewing it blindly would extend that + -- other key's ttl while this key stays unregistered. + if occupant == key then + if not exptime then + return end - N = N+1 - local ok, err, forcible = self.dict:add(self.key_prefix .. N, key, exptime) + + local ok, err = self.dict:expire(self.key_prefix .. idx, exptime) if ok then - local _, _, forcible2 = self.dict:incr(self.key_count, 1, 0) - self.keys[N] = key - self.index[key] = N - if exptime and exptime > 0 then - self.expire_keys[N] = true - end - if forcible or forcible2 or repair_forcible then - return (err_msg_lru_eviction .. "; key index: add key: idx=" .. - self.key_prefix .. N .. ", key=" .. key) + if exptime > 0 then + self.expire_keys[idx] = true end - break - elseif err ~= "exists" then - return "Unexpected error adding a key: " .. err + return end - -- "exists": slot N is already occupied although key_count reported N-1. - -- Once per key this can be a benign race with another worker that has - -- created slot N but not incremented key_count yet, so retry and let - -- sync() pick the new slot up. If it repeats, key_count has fallen - -- behind the occupied slots: it is an ordinary shared-dict node, so on - -- a full dict it can be LRU-evicted (it is only refreshed when new keys - -- are registered, so it goes cold under steady traffic) and incr() then - -- re-creates it at 1, far below the surviving slots. Retrying the same - -- slot forever would spin the worker at 100% CPU with the shared-dict - -- lock held hot (apache/apisix#12275). Advance the counter past the - -- occupied slot instead: the next sync() adopts that slot's occupant - -- and progress resumes. - if retried then - local _, incr_err, forcible3 = self.dict:incr(self.key_count, 1, 0) - if incr_err then - -- hard failure (e.g. "no memory"): give up immediately, mirroring - -- the add() error path above, instead of burning the repair budget - -- on retries that cannot succeed. - return "Unexpected error advancing key_count: " .. incr_err - end - if forcible3 then - -- re-creating an evicted key_count displaced another entry; surface - -- it through the LRU-eviction warning on the success path, like - -- forcible/forcible2. - repair_forcible = true + if err ~= "not found" then + -- Unexpected expire error: the slot may still be live, so leave it + -- as-is rather than re-adding it, which would create a duplicate. + ngx.log(ngx.ERR, "failed to renew expire for key '", key, "': ", + tostring(err)) + return + end + -- "not found": the node was reclaimed between the two calls, so fall + -- through and take the slot number back below + occupant = nil + end + + if occupant == nil then + -- The slot holds no visible key: it is either past its ttl or already + -- reclaimed. Taking it back in place keeps this key on the slot number + -- the other workers already know, so nothing has to be broadcast, and + -- key_count does not grow every time a metric expires and comes back. + local ok, err, forcible = self.dict:add(self.key_prefix .. idx, key, exptime) + if ok or (err == "exists" and + self.dict:get(self.key_prefix .. idx) == key) then + -- either we took it back, or a peer took it back for us + self:claim_slot(idx, key, exptime) + if forcible then + return (err_msg_lru_eviction .. "; key index: re-claimed slot: idx=" .. + self.key_prefix .. idx .. ", key=" .. key) end - -- The cap counts attempts, not successes: it exists to guarantee the - -- loop terminates. - repairs = repairs + 1 - if repairs >= MAX_KEY_COUNT_REPAIRS then - return (err_msg_lru_eviction .. "; key index: key_count fell " .. - "behind occupied slots; advanced it by " .. repairs .. - " without finding a free slot, dropping key: " .. key) + return + end + end + + -- The slot belongs to another key now. Only this key's own reference is + -- dropped: self.keys[idx] describes the new occupant and stays. + if self.index[key] == idx then + self.index[key] = nil + end + end + + return self:alloc_slot(key, err_msg_lru_eviction, exptime) +end + + +-- Records a slot this worker has just written below the other workers' +-- self.last, and makes sure a full scan can reach it. +function KeyIndex:claim_slot(idx, key, exptime) + self:set_slot(idx, key) + if exptime and exptime > 0 then + self.expire_keys[idx] = true + end + self:ensure_key_count(idx) + + -- An incremental sync would never read this slot again, so the number is + -- published for the scrape to re-read. The workers never read this, so + -- nothing is forced to re-scan on a request. safe_set, so that publishing + -- never evicts a metric to make room for itself: on a full dict the entry is + -- simply not written, and the scrape falls back to walking every slot. + local seq = self.dict:incr(self.reuse_count, 1, 0) + if seq then + self.dict:safe_set(self.reuse_slot .. seq % self.ring_size, + string.format(REUSE_ENTRY_FORMAT, seq, idx)) + end +end + + +-- Gives a key a slot it does not have yet: one this worker has seen reclaimed, +-- if it has one, a fresh one past key_count otherwise. +-- +-- Without reuse key_count only ever grows: a metric whose labels are never +-- seen again leaves its number behind for good, and under label churn the +-- range a scrape walks grows without bound (apache/apisix#13658). +function KeyIndex:alloc_slot(key, err_msg_lru_eviction, exptime) + for _ = 1, FREE_SLOT_ATTEMPTS do + if self.free_n == 0 then + break + end + + local idx = self.free_slots[self.free_n] + self.free_slots[self.free_n] = nil + self.free_n = self.free_n - 1 + + -- a number taken since it was put aside simply fails here + local ok, _, forcible = self.dict:add(self.key_prefix .. idx, key, exptime) + if ok then + self:claim_slot(idx, key, exptime) + if forcible then + return (err_msg_lru_eviction .. "; key index: reused slot: idx=" .. + self.key_prefix .. idx .. ", key=" .. key) + end + return + end + end + + local retried = false + local repairs = 0 + local repair_forcible = false + while true do + local N = self:sync() + + -- another worker may have registered this key while we were looking + local existing = self.index[key] + if existing and self.dict:get(self.key_prefix .. existing) == key then + if exptime then + self.dict:expire(self.key_prefix .. existing, exptime) + if exptime > 0 then + self.expire_keys[existing] = true end end - retried = true + if repair_forcible then + return (err_msg_lru_eviction .. "; key index: adopted key after " .. + "key_count repair: idx=" .. self.key_prefix .. existing .. + ", key=" .. key) + end + return + end + + N = N + 1 + local ok, err, forcible = self.dict:add(self.key_prefix .. N, key, exptime) + if ok then + local _, _, forcible2 = self.dict:incr(self.key_count, 1, 0) + self:set_slot(N, key) + if exptime and exptime > 0 then + self.expire_keys[N] = true + end + if forcible or forcible2 or repair_forcible then + return (err_msg_lru_eviction .. "; key index: add key: idx=" .. + self.key_prefix .. N .. ", key=" .. key) + end + return + elseif err ~= "exists" then + return "Unexpected error adding a key: " .. err end + + -- "exists": slot N is already occupied although key_count reported N-1. + -- Once per key this can be a benign race with another worker that has + -- created slot N but not incremented key_count yet, so retry and let + -- sync() pick the new slot up. If it repeats, key_count has fallen + -- behind the occupied slots: it is an ordinary shared-dict node, so on + -- a full dict it can be LRU-evicted (it is only refreshed when new keys + -- are registered, so it goes cold under steady traffic) and incr() then + -- re-creates it at 1, far below the surviving slots. Retrying the same + -- slot forever would spin the worker at 100% CPU with the shared-dict + -- lock held hot (apache/apisix#12275). Advance the counter past the + -- occupied slot instead: the next sync() adopts that slot's occupant + -- and progress resumes. + if retried then + local _, incr_err, forcible3 = self.dict:incr(self.key_count, 1, 0) + if incr_err then + -- hard failure (e.g. "no memory"): give up immediately, mirroring + -- the add() error path above, instead of burning the repair budget + -- on retries that cannot succeed. + return "Unexpected error advancing key_count: " .. incr_err + end + if forcible3 then + -- re-creating an evicted key_count displaced another entry; surface + -- it through the LRU-eviction warning on the success path, like + -- forcible/forcible2. + repair_forcible = true + end + -- The cap counts attempts, not successes: it exists to guarantee the + -- loop terminates. + repairs = repairs + 1 + if repairs >= MAX_KEY_COUNT_REPAIRS then + return (err_msg_lru_eviction .. "; key index: key_count fell " .. + "behind occupied slots; advanced it by " .. repairs .. + " without finding a free slot, dropping key: " .. key) + end + end + retried = true end end + -- Removes a key based on its value. -- -- Args: @@ -305,9 +592,7 @@ end function KeyIndex:remove(key, err_msg_lru_eviction) local i = self.index[key] if i then - self.index[key] = nil - self.keys[i] = nil - self.expire_keys[i] = nil + self:clear_slot(i) self.dict:set(self.key_prefix .. i, nil) self.deleted = self.deleted + 1 diff --git a/prometheus_test.lua b/prometheus_test.lua index 0bfe05d..dc71051 100644 --- a/prometheus_test.lua +++ b/prometheus_test.lua @@ -19,13 +19,22 @@ function SimpleDict:set(k, v, exptime) end return true, nil, forcible end +function SimpleDict:capacity() + return 10 * 1024 * 1024 -- like a lua_shared_dict of 10m +end +function SimpleDict:safe_set(k, v, exptime) + -- like ngx.shared.DICT:safe_set: never evicts, so in this mock, which never + -- runs out of memory, it cannot fail + return self:set(k, v, exptime) +end function SimpleDict:add(k, v, exptime) local forcible = false if k == "willnotfitk" or v == "willnotfitv" then forcible = true end - self:get(k) -- prunes the key if it has expired, like ngx.shared.DICT does - if self.dict and self.dict[k] then + -- ngx.shared.DICT:add only refuses a node that is still live; one that is + -- past its exptime is reused in place, keeping the same key. + if self:get(k) ~= nil then return false, "exists", false -- match ngx.shared.DICT:add on present keys end self:set(k, v, exptime) @@ -51,18 +60,27 @@ function SimpleDict:get(k) return nil, "dict error" end if not self.dict then self.dict = {} end - if self.dict[k] and self.dict[k]["expired"] and self.dict[k]["expired"] < os.time() then self.dict[k] = nil end - local v = self.dict[k] or {} - return v["value"], nil -- value, err + -- An entry past its exptime reads as missing but is NOT freed here: like + -- ngx.shared.DICT, only flush_expired() (or a write reusing the node) frees + -- it. Pruning here would collapse the "past its ttl" and "node reclaimed" + -- states, which ttl() has to tell apart. + local e = self.dict[k] + if not e or (e["expired"] and e["expired"] < os.time()) then + return nil, nil + end + return e["value"], nil -- value, err end function SimpleDict:delete(k) self.dict[k] = nil end function SimpleDict:expire(k, exptime) + -- Like ngx.shared.DICT:expire: it looks the node up without checking the + -- exptime, so a node that is merely past it is resurrected in place, with + -- its value intact. Only a node that has been freed reports "not found". if not self.dict[k] then return nil, "not found" end - self.dict[k]["expired"] = os.time() + exptime + self.dict[k]["expired"] = exptime ~= 0 and (os.time() + exptime) or nil return true -- match ngx.shared.DICT:expire, which returns true on success end function SimpleDict:flush_expired(n) @@ -82,18 +100,19 @@ function SimpleDict:flush_expired(n) return flushed end function SimpleDict:ttl(k) - -- Like ngx.shared.DICT:ttl, an expired entry reads as "not found" but is NOT - -- freed here: only flush_expired (or a write reusing the node) reclaims it. - -- Do not prune, or the physical-reclamation assertions below become vacuous. + -- Like ngx.shared.DICT:ttl, which peeks at the node without checking the + -- exptime. It reports the three states a slot can be in: + -- live -> a positive number (0 when permanent) + -- past its exptime, node kept -> a negative number + -- node freed -> nil, "not found" local e = self.dict and self.dict[k] - if not e or (e["expired"] and e["expired"] < os.time()) then + if not e then return nil, "not found" end - if e["expired"] then - return e["expired"] - os.time() - else + if not e["expired"] then return 0 end + return e["expired"] - os.time() end local function sleep(n) @@ -832,56 +851,78 @@ end -- Regression test for apache/apisix#11934 (duplicate metrics). -- A key with an exptime is added and synced (self.last now tracks N). The key -- then expires in the underlying shared dict without going through remove(), so --- neither key_count nor delete_count changes and the next sync() is a no-op, --- leaving self.index still pointing at the now-vanished slot. Re-adding the key --- therefore takes the "expired" path in add() and allocates a new slot while --- the stale slot lingers in self.keys. Before the fix, delete_count was not --- bumped (other workers never re-synced) and list() iterated self.keys, so the --- same key was emitted twice -> duplicate metrics. -function TestKeyIndex:testExpiredReAddNoDuplicate() +-- neither key_count nor delete_count changes and an incremental sync is a +-- no-op, leaving self.index still pointing at the slot. Re-adding the key must +-- take that same slot back rather than allocate a second one: a key on two +-- slots is what produced the duplicate metrics, and a new slot per expiry is +-- what makes key_count grow without bound. +function TestKeyIndex:testExpiredReAddReclaimsSameSlot() + -- reclaiming is left to the caller here, so the slot stays in the "past its + -- ttl, node still in the dict" state that this test is about + self.key_index = require('prometheus_keys').new(self.dict, "_prefix_", 1, false) local err = self.key_index:add("expkey", "eviction_err", 1) luaunit.assertEquals(err, nil) self.key_index:sync() luaunit.assertEquals(self.dict:get("_prefix_key_count"), 1) luaunit.assertEquals(self.dict:get("_prefix_key_1"), "expkey") - luaunit.assertEquals(self.dict:get("_prefix_delete_count"), nil) luaunit.assertEquals(self.key_index.index["expkey"], 1) - -- A second worker sharing the same shared dict syncs the initial state, so it - -- now holds slot 1 in its local self.keys/index. This is the worker that the - -- delete_count bump must later force to re-sync and reclaim the stale slot. + -- A second worker sharing the same shared dict syncs the initial state, so + -- it now holds slot 1 in its local self.keys/index. local worker2 = require('prometheus_keys').new(self.dict, "_prefix_", 1) worker2:sync() luaunit.assertEquals(worker2.index["expkey"], 1) luaunit.assertEquals(#worker2:list(), 1) - -- Let the key expire in the underlying shared dict. key_count and - -- delete_count are untouched, so the next sync() inside add() is a no-op and - -- the local index keeps pointing at the (now gone) slot 1. sleep(2) + -- past its ttl, but the node is still there, so the slot can be taken back luaunit.assertEquals(self.dict:get("_prefix_key_1"), nil) + luaunit.assertTrue(self.dict:ttl("_prefix_key_1") < 0) + + -- a reclaim round stops listing the expired key without giving up its slot + -- (metric_data() skips it in the meantime: its value has expired too) + self.key_index:remove_expired_keys() + luaunit.assertEquals(#self.key_index:list(), 0) + luaunit.assertEquals(self.key_index.index["expkey"], 1) - -- Re-adding the now-expired key takes the expired branch and allocates slot 2. err = self.key_index:add("expkey", "eviction_err", 1) luaunit.assertEquals(err, nil) - luaunit.assertEquals(self.dict:get("_prefix_key_2"), "expkey") - -- delete_count must have been bumped on the expired re-add path so that - -- other workers do a full sync and drop the stale slot. - luaunit.assertEquals(self.dict:get("_prefix_delete_count"), 1) + -- same slot, no new one, and nothing broadcast to the other workers + luaunit.assertEquals(self.dict:get("_prefix_key_1"), "expkey") + luaunit.assertEquals(self.dict:get("_prefix_key_count"), 1) + luaunit.assertEquals(self.dict:get("_prefix_key_2"), nil) + luaunit.assertEquals(self.dict:get("_prefix_delete_count"), nil) - -- list() must report the key exactly once, not twice. local keys = self.key_index:list() luaunit.assertEquals(#keys, 1) luaunit.assertEquals(keys[1], "expkey") - -- The second worker must converge: its next sync() sees the bumped - -- delete_count, does a full sync, drops the stale slot 1 and picks up slot 2. - -- Without the delete_count bump it would keep slot 1 forever and list() the - -- key twice. + -- The second worker never had to re-sync: the key never left slot 1, so its + -- local state was correct the whole time and lists the key exactly once. + local keys2 = worker2:list() + luaunit.assertEquals(#keys2, 1) + luaunit.assertEquals(keys2[1], "expkey") +end + +-- Same, but after the node itself has been reclaimed: the slot number is still +-- the key's own, so it is taken back in place rather than allocated anew. +function TestKeyIndex:testReclaimedSlotIsTakenBackInPlace() + luaunit.assertEquals(self.key_index:add("expkey", "eviction_err", 1), nil) + local worker2 = require('prometheus_keys').new(self.dict, "_prefix_", 1) worker2:sync() - luaunit.assertEquals(worker2.keys[1], nil) - luaunit.assertEquals(worker2.index["expkey"], 2) + + sleep(2) + self.key_index:flush_expired() + luaunit.assertNil(self.dict.dict["_prefix_key_1"]) + + luaunit.assertEquals(self.key_index:add("expkey", "eviction_err", 1), nil) + luaunit.assertEquals(self.dict:get("_prefix_key_1"), "expkey") + luaunit.assertEquals(self.dict:get("_prefix_key_count"), 1) + luaunit.assertEquals(self.dict:get("_prefix_delete_count"), nil) + + -- The other worker must not miss it although the slot was rewritten below + -- its self.last and neither counter moved. local keys2 = worker2:list() luaunit.assertEquals(#keys2, 1) luaunit.assertEquals(keys2[1], "expkey") @@ -903,9 +944,11 @@ function TestKeyIndex:testRemoveExpiredKeysReclaimsSharedDict() sleep(2) - -- Both entries are logically gone but still physically present: every dict - -- API reports them as missing while they still hold their slab pages. - luaunit.assertEquals(self.dict:ttl("_prefix_key_1"), nil) + -- Both entries are logically gone but still physically present: get() + -- reports them as missing while they still hold their slab pages, and ttl() + -- is what tells that state apart from a node that is really gone. + luaunit.assertEquals(self.dict:get("_prefix_key_1"), nil) + luaunit.assertTrue(self.dict:ttl("_prefix_key_1") < 0) luaunit.assertNotNil(self.dict.dict["_prefix_key_1"]) luaunit.assertNotNil(self.dict.dict["expkey"]) @@ -918,8 +961,160 @@ function TestKeyIndex:testRemoveExpiredKeysReclaimsSharedDict() luaunit.assertNil(self.dict.dict["expkey"]) -- Entries that have not expired must be left alone. luaunit.assertNotNil(self.dict.dict["permanent"]) + + -- With the node gone the slot number is given up: a second round sees + -- "not found" and drops the local reference to it. + local _, err2 = self.dict:ttl("_prefix_key_1") + luaunit.assertEquals(err2, "not found") + self.key_index:remove_expired_keys() + luaunit.assertNil(self.key_index.index["expkey"]) +end + +-- A slot that is still live must survive a reclaim round untouched: its key +-- keeps being listed, and neither its slot number nor its local state moves. +function TestKeyIndex:testRemoveExpiredKeysKeepsLiveSlots() + luaunit.assertEquals(self.key_index:add("livekey", "eviction_err", 60), nil) + luaunit.assertEquals(self.key_index:add("permkey", "eviction_err"), nil) + + self.key_index:remove_expired_keys() + + local listed = {} + for _, k in ipairs(self.key_index:list()) do + listed[k] = true + end + luaunit.assertTrue(listed["livekey"]) + luaunit.assertTrue(listed["permkey"]) + luaunit.assertEquals(self.key_index.index["livekey"], 1) + luaunit.assertEquals(self.dict:get("_prefix_key_1"), "livekey") +end + + +-- Slot numbers whose entry is gone for good -- a metric whose labels will +-- never be seen again -- are reused by the next metric that needs one, instead +-- of key_count growing for ever (apache/apisix#13658). +function TestKeyIndex:testReclaimedSlotsAreReused() + for i = 1, 3 do + luaunit.assertEquals(self.key_index:add("churn" .. i, "eviction_err", 1), nil) + end + luaunit.assertEquals(self.dict:get("_prefix_key_count"), 3) + + sleep(2) + self.key_index:remove_expired_keys() + + for i = 4, 6 do + luaunit.assertEquals(self.key_index:add("churn" .. i, "eviction_err", 60), nil) + end + + -- three slots, reused, instead of six + luaunit.assertEquals(self.dict:get("_prefix_key_count"), 3) + + local listed = {} + for _, k in ipairs(self.key_index:list()) do + listed[k] = true + end + for i = 4, 6 do + luaunit.assertTrue(listed["churn" .. i]) + end + luaunit.assertEquals(#self.key_index:list(), 3) +end + + +-- A slot that is only past its ttl still has its node, and its own key can +-- take it back in place at any time, so another key must not be given it. +-- get() cannot tell this state from a reclaimed slot; ttl() can. +function TestKeyIndex:testSlotsOnlyPastTheirTtlAreNotReused() + self.key_index = require('prometheus_keys').new(self.dict, "_prefix_", 1, false) + luaunit.assertEquals(self.key_index:add("expkey", "eviction_err", 1), nil) + sleep(2) + self.key_index:remove_expired_keys() + luaunit.assertEquals(self.dict:get("_prefix_key_1"), nil) + + luaunit.assertEquals(self.key_index:add("newcomer", "eviction_err", 60), nil) + luaunit.assertEquals(self.dict:get("_prefix_key_2"), "newcomer") + luaunit.assertEquals(self.dict:get("_prefix_key_count"), 2) + + -- so the original key still comes back on its own slot + luaunit.assertEquals(self.key_index:add("expkey", "eviction_err", 60), nil) + luaunit.assertEquals(self.dict:get("_prefix_key_1"), "expkey") + luaunit.assertEquals(self.dict:get("_prefix_key_count"), 2) end + +-- Once a slot number has been reused by another key, a worker that still has +-- the old key in its index must not touch it: blindly renewing it would push +-- out the new occupant's ttl while leaving its own key unregistered. +function TestKeyIndex:testReusedSlotIsNotHijackedByAStaleIndex() + local worker2 = require('prometheus_keys').new(self.dict, "_prefix_", 1) + luaunit.assertEquals(self.key_index:add("gone", "eviction_err", 1), nil) + worker2:sync() + luaunit.assertEquals(worker2.index["gone"], 1) + + -- the slot is reclaimed and reused by another key, while worker2 is none + -- the wiser: neither key_count nor delete_count moves + sleep(2) + self.key_index:remove_expired_keys() + luaunit.assertEquals(self.key_index:add("newcomer", "eviction_err", 60), nil) + luaunit.assertEquals(self.dict:get("_prefix_key_1"), "newcomer") + luaunit.assertEquals(self.dict:get("_prefix_delete_count"), nil) + + -- worker2 registers its key again: it must land somewhere else, and the + -- newcomer must keep both its slot and its own ttl + luaunit.assertEquals(worker2:add("gone", "eviction_err", 60), nil) + luaunit.assertEquals(self.dict:get("_prefix_key_1"), "newcomer") + luaunit.assertNotEquals(worker2.index["gone"], 1) + luaunit.assertEquals(self.dict:get("_prefix_key_" .. worker2.index["gone"]), "gone") + + -- and a scrape reports each of them exactly once + local scraper = require('prometheus_keys').new(self.dict, "_prefix_", 1) + local listed, n = {}, 0 + for _, k in ipairs(scraper:list()) do + listed[k] = (listed[k] or 0) + 1 + n = n + 1 + end + luaunit.assertEquals(n, 2) + luaunit.assertEquals(listed["gone"], 1) + luaunit.assertEquals(listed["newcomer"], 1) +end + + +-- Label churn must not make key_count grow without bound: every round retires +-- one series and registers a new one, and the retired number comes back. +function TestKeyIndex:testKeyCountStaysBoundedUnderChurn() + for round = 1, 5 do + luaunit.assertEquals(self.key_index:add("series" .. round, "eviction_err", 1), nil) + sleep(2) + self.key_index:remove_expired_keys() + end + + -- one slot, reused five times over, instead of five + luaunit.assertEquals(self.dict:get("_prefix_key_count"), 1) +end + + +-- A slot taken back in place can sit above key_count, once key_count has been +-- LRU-evicted and re-created below the slots already in use. A full scan walks +-- 0..key_count, so the counter has to be raised or the key becomes invisible +-- to every worker that has no local record of it. +function TestKeyIndex:testReclaimedSlotAboveKeyCountIsMadeVisible() + for i = 1, 3 do + luaunit.assertEquals(self.key_index:add("key" .. i, "eviction_err", 1), nil) + end + sleep(2) + self.key_index:flush_expired() + + -- key_count is an ordinary entry: on a full dict it can be evicted, and + -- incr() then re-creates it far below the surviving slots + self.dict:delete("_prefix_key_count") + + luaunit.assertEquals(self.key_index:add("key3", "eviction_err", 60), nil) + luaunit.assertEquals(self.dict:get("_prefix_key_3"), "key3") + luaunit.assertTrue((self.dict:get("_prefix_key_count") or 0) >= 3) + + local scraper = require('prometheus_keys').new(self.dict, "_prefix_", 1) + luaunit.assertEquals(scraper:list(), {"key3"}) +end + + -- flush_expired() holds the dict lock for its whole scan of the LRU queue, so -- the reclamation is issued in bounded batches. A backlog larger than one batch -- must still be reclaimed in full, by looping. @@ -967,7 +1162,10 @@ function TestKeyIndex:testAutoFlushExpiredDisabled() sleep(2) key_index:remove_expired_keys() - luaunit.assertNil(key_index.index["expkey"]) + -- the key stops being listed, but its slot number is kept: the node is only + -- past its ttl, so add() can still take that slot back in place + luaunit.assertEquals(#key_index:list(), 0) + luaunit.assertEquals(key_index.index["expkey"], 1) -- both entries are still physically present, unlike with the default luaunit.assertNotNil(self.dict.dict["_noflush_key_1"]) luaunit.assertNotNil(self.dict.dict["expkey"]) @@ -1199,8 +1397,13 @@ function TestPrometheus:testKeyTimeout() self.p.key_index:sync() luaunit.assertEquals(self.dict:get("metric_exp"), nil) luaunit.assertEquals(self.dict:get("__ngx_prom__key_" .. i), nil) - luaunit.assertEquals(self.p.key_index.index["metric_exp"], nil) - luaunit.assertEquals(self.p.key_index.keys[i], nil) + -- The expired metric stops being listed, but its slot is only hidden: the + -- node is merely past its ttl, so the metric can come back on the same slot. + luaunit.assertEquals(self.p.key_index.hidden[i], true) + luaunit.assertEquals(self.p.key_index.index["metric_exp"], i) + for _, k in ipairs(self.p.key_index:list()) do + luaunit.assertNotEquals(k, "metric_exp") + end self.gauge_exp:inc(1) luaunit.assertEquals(self.dict:get("gauge_exp"), 1) @@ -1212,8 +1415,8 @@ function TestPrometheus:testKeyTimeout() self.p.key_index:sync() luaunit.assertEquals(self.dict:get("gauge_exp"), nil) luaunit.assertEquals(self.dict:get("__ngx_prom__key_" .. i), nil) - luaunit.assertEquals(self.p.key_index.index["gauge_exp"], nil) - luaunit.assertEquals(self.p.key_index.keys[i], nil) + luaunit.assertEquals(self.p.key_index.index["gauge_exp"], i) + luaunit.assertEquals(self.p.key_index.hidden[i], true) self.gauge_exp_2:set(1) self.p.key_index:sync() diff --git a/rfcs/0001-slot-reuse-and-bounded-reclaim.md b/rfcs/0001-slot-reuse-and-bounded-reclaim.md new file mode 100644 index 0000000..3555cb8 --- /dev/null +++ b/rfcs/0001-slot-reuse-and-bounded-reclaim.md @@ -0,0 +1,303 @@ +# RFC 0001: bounded reclamation and slot reuse in KeyIndex + +Status: proposed +Tracking: apache/apisix#13658, apache/apisix#11934, apache/apisix#12275 + +## 1. Problem + +`KeyIndex` gives every metric name a numbered slot in the shared dict +(`__ngx_prom__key_N`) so that a scrape can enumerate the metrics without +`get_keys()`. `key_count` is the highest number handed out, and a full sync +walks `0..key_count`. + +Three problems come out of that, all of them only when metrics are registered +with an `exptime`. + +### 1.1 One low-frequency series coming back pegs every worker + +When a metric comes back after its slot is gone, `add()` bumps `delete_count` +(added in #14 so that peers stop listing a stale slot). Every worker's next +`sync()` then walks `0..key_count` -- and `sync()` runs on the request path, +once per observation of a metric with an `exptime`. + +Measured on the shape of a 3.9.x gateway pod (§6.1): with 140k slots and 200 +req/s, a single such return doubles the CPU of the worker set for the next +window, and 50 of them over 10s put three workers at 98%, 93% and 88% of a +core. That is the `top` picture behind the reports. + +### 1.2 Slot numbers are never reused + +A metric whose labels are never seen again leaves its number behind for good, +and a metric that expires and comes back gets a *new* one. `key_count` only +grows, and everything proportional to it grows with it. The dump in +apache/apisix#13658 had 747,970 dead index entries against 28 live series. + +### 1.3 The reclamation holds the dict lock for a whole backlog + +`remove_expired_keys()` calls `flush_expired()` with no bound, in every worker. +That call holds the dict mutex until it returns and walks the whole LRU queue, +so an hour's worth of expired entries is reclaimed in one uninterrupted hold, +and every worker repeats the walk (#23 in this repo). + +## 2. What the shared dict actually offers + +A slot is in one of three states, and `get()` reports the last two alike: + +| state | `get()` | `ttl()` | `expire()` | `add()` | +|---|---|---|---|---| +| (1) live | the key | `> 0` | renews | `"exists"` | +| (2) past its ttl, node still in the dict | `nil` | `< 0` | resurrects it, value intact | replaces in place | +| (3) node physically reclaimed | `nil` | `nil, "not found"` | `"not found"` | creates it | + +Verified on OpenResty 1.29.2.4 (`resty --shdict`) and in the source: `ttl()` +goes through `ngx_http_lua_shdict_peek()`, which neither checks the expiry nor +touches the LRU position, while `get()` goes through +`ngx_http_lua_shdict_lookup()`, which returns `NGX_DONE` for an expired node. + +This is the basis of the design: **`get()` answers "is this key visible", +`ttl()` answers "does this node still exist"**. The second question is what +decides whether a slot number still belongs to its key. + +## 3. Design + +### 3.1 The renewal path checks ownership instead of syncing + +`add()` used to start with `sync()` -- two reads of the shared counters -- and +then renew the slot `self.index[key]` points at. It now reads the slot itself: + +```lua +if self.dict:get(self.key_prefix .. idx) == key then + self.dict:expire(self.key_prefix .. idx, exptime) -- done +end +``` + +One read instead of two, and none of the shared counters are touched, so +nothing another worker does can force this path into a full sync. It is also +what makes slot reuse safe: an index entry left over from a slot that has since +been handed to another key does not match here, where renewing it blindly would +extend a foreign key's ttl and leave this key unregistered. + +`delete_count` is no longer bumped when a metric comes back (1.1), and +`remove()` -- which APISIX never calls on prometheus metrics -- remains the only +writer of it. + +### 3.2 A key takes its own slot back in place + +If the slot holds no visible key -- state (2) or (3) -- the key takes the number +back with `add()`, verifying the occupant if that comes back `"exists"` (a peer +may have taken it back first). The key keeps the number every worker already +knows, so nothing has to be broadcast and `key_count` does not grow when a +metric expires and comes back. + +### 3.3 Reclaimed numbers are reused by other keys + +`clear_slot()` runs when a worker sees a slot in state (3); it puts the number +in a bounded per-worker list. A key that needs a slot takes one from there and +writes it exactly as it would write a fresh one -- a number taken in the +meantime simply fails that write and is dropped. Reuse therefore costs no +search: no shared cursor, no scan, no extra read. + +### 3.4 `key_count` is raised above a slot taken in place + +A slot above `key_count` is invisible to a full scan. That happens when +`key_count` -- an ordinary dict entry -- is LRU-evicted and `incr()` re-creates +it below the slots in use. `ensure_key_count(idx)` raises it, re-checking the +result because a lost race can only overshoot. + +### 3.5 The scrape follows a trail of reused slots + +Slots are written below the other workers' `self.last`, where an incremental +sync would never read them again. Every such write publishes `":"` +into a ring in the dict and bumps `reuse_count`; `list()` is the only reader. +When the counter has moved, the scrape re-reads just those slots; only a scrape +that has fallen further behind than the ring, or that finds the trail +incomplete, walks every slot. + +The ring entries are created up front and always rewritten with a value of the +same length, so publishing is an in-place write that cannot fail for want of +memory. This matters: while publishing used `safe_set` on a not-yet-existing +entry, a full dict dropped the write, every scrape fell back to walking 400k +slots, and the tail latency of the whole gateway went with it (§6.3). The ring +holds one entry per 64 KiB of dict capacity, between 256 and 8192. + +### 3.6 The scan is one read per slot + +A slot that holds no live key is *hidden*: dropped from the listing, slot number +kept. Telling state (2) from state (3) costs a second read and decides whether +the number can be given up, so it is done by `remove_expired_keys()`, on a +timer, not on every scrape. + +### 3.7 The local views cannot drift apart + +`self.keys` (slot -> key) and `self.index` (key -> slot) are maintained only +through `set_slot` / `hide_slot` / `clear_slot`. Leaving a previous occupant's +index entry behind would point a later `add()` at a slot that is no longer its +own; dropping the index entry of a key that has since moved would hide a live +key from `list()`. + +## 4. Invariants + +1. **Ownership.** A worker only renews a slot whose current value is its own key + (3.1). Nothing else can extend a foreign key's ttl. +2. **Identity.** A number is reused by another key only after its node is gone + (3.3), and only through `add()`, which fails on a live node. +3. **Visibility.** Every live slot is within `0..key_count` (3.4), and a slot + written below `self.last` is either on the trail the scrape follows or causes + a full walk (3.5). +4. **Uniqueness.** `list()` emits a key only from the slot its own index points + at, so a key that transiently sits on two slots is listed once. +5. **Locality.** `self.keys` and `self.index` are mutual inverses (3.7). + +## 5. Scenario tests + +### 5.1 Unit (`prometheus_test.lua`) + +The `SimpleDict` mock now models the three states of §2: `get()` no longer +prunes an expired node, `ttl()` returns a negative number for state (2) and +`"not found"` for state (3), `expire()` resurrects a state-(2) node, and `add()` +replaces one in place. Tests that assumed the old semantics were rewritten +rather than patched. + +| test | what it pins down | +|---|---| +| `testExpiredReAddReclaimsSameSlot` | a metric that comes back takes its own slot; `key_count` and `delete_count` do not move; a second worker that never re-synced still lists it once | +| `testReclaimedSlotIsTakenBackInPlace` | same, after the node itself was reclaimed; a second worker sees it although the write was below its `self.last` | +| `testReclaimedSlotsAreReused` | three retired numbers are reused by three new metrics; `key_count` stays at 3 | +| `testSlotsOnlyPastTheirTtlAreNotReused` | a state-(2) slot is not given to another key, and its own key still gets it back | +| `testReusedSlotIsNotHijackedByAStaleIndex` | a worker holding a stale index does not renew the new occupant, registers its own key elsewhere, and a scrape lists both exactly once | +| `testKeyCountStaysBoundedUnderChurn` | five rounds of retire-and-register reuse one slot | +| `testReclaimedSlotAboveKeyCountIsMadeVisible` | with `key_count` evicted, a slot taken back above it is still listed by a fresh worker | +| `testRemoveExpiredKeysKeepsLiveSlots` | a reclaim round leaves live slots alone | +| `testFlushExpiredRunsInBatches` | a 20,000-entry backlog is reclaimed in bounded batches (each call is asserted to ask for 10,000) | +| `testAutoFlushExpiredDisabled` | with the option off, a reclaim round only drops local references | + +53 of 54 tests pass; `TestPrometheus.testPrintfTable` fails on `main` as well, +under LuaJIT, and is unrelated. + +### 5.2 Multi-worker correctness + +OpenResty 1.29.2.4, `worker_processes 10` plus the privileged agent, which +scrapes and (on this branch) reclaims. Every check compares what the scraping +process lists against ground truth taken from the dict itself -- a walk of +`1..key_count` -- inside that same process, so nothing is lost in transport. + +| dict / series | variant | live slots | listed | duplicates | missing | two slots, same key | +|---|---|---|---|---|---|---| +| 100m / 300k | v1.0.0 | 300,001 | 300,001 | 0 | 0 | 0 | +| 100m / 300k | this branch | 300,001 | 300,001 | 0 | 0 | 0 | +| 100m / 300k, after 40s of churn | v1.0.0 | 402,778 | 405,399 | 0 | 0 | 0 | +| 100m / 300k, after 40s of churn | this branch | 402,418 | 402,592 | 0 | 0 | 76 | +| 500m / 1.5M, after 60s of churn | v1.0.0 | 1,500,001 | 1,500,001 | 0 | 0 | 0 | +| 500m / 1.5M, after 60s of churn | this branch | 1,500,001 | 1,500,001 | 0 | 0 | 0 | + +The 76 slots holding the same key are the transient this design allows: two +workers can each end up with a slot for the same key when one of them had +already dropped its reference. `list()` emits the key once (invariant 4), and +the extra slot is reclaimed when the metric next expires. It is 0.02% of the +slots in that run, and the earlier row shows the counterpart: on v1.0.0 it is +the *expired* keys that linger in the listing (2,621 against 250 here). + +Churn also shows what the reuse is for. Twelve rounds of 50 fresh series, each +round expiring before the next, on 10 workers: + +| | v1.0.0 | this branch | +|---|---|---| +| `key_count` after 12 rounds | 383 -> 934 (+50 per round) | 254, flat | +| consistency check each round | pass | pass | + +## 6. Performance report + +20-core x86-64, OpenResty 1.29.2.4, load generator on the same host. Each +microbenchmark figure is the median of 5 runs issued over one keepalive +connection, so they land on the same worker, with the scrape and the reclamation +paused for the duration. + +### 6.1 The CPU spike of §1.1 + +10 workers, a 512m dict, the three metrics of `apisix/plugins/prometheus/exporter.lua` +with their label sets and the default latency buckets, 140k label combinations +accumulated, then 200 req/s of exactly what `exporter.http_log()` does +(`status:inc` + three `latency:observe` + two `bandwidth:inc`). CPU is the sum +over the workers, from `/proc//stat`, over 10s windows. + +| window | v1.0.0 | this branch | +|---|---|---| +| steady 200 req/s | 14.9% | **7.8%** | +| one low-frequency series comes back | **30.2%** (`delete_count` +1) | **2.2%** (+0) | +| 50 of them over 10s | **287.7%**, busiest workers 98.6 / 93.3 / 87.9% | **4.5%**, busiest 4.0% | + +The low-frequency series is registered before the 140k are filled in, so its +slot is an old one: on v1.0.0 an incremental sync happens to clear a stale index +entry for a *recent* slot, and only an old slot reaches the branch that bumps +`delete_count`. That matches a pod that has been running for days. + +The third row is externally triggered (`delete_count` is bumped directly), and +this branch does not react at all: its request path never reads the shared +counters, so a bump cannot make it scan. + +### 6.2 Microbenchmarks + +| | 100m / 300k | | 500m / 1.5M | | +|---|---|---|---|---| +| | v1.0.0 | branch | v1.0.0 | branch | +| renewing a known series | 0.42-0.56 us/op | 0.54-0.70 us/op | 2.04 us/op | 2.14 us/op | +| registering a new series | 3.45-3.75 us/op | 3.50-4.50 us/op | 3.60 us/op | 3.50 us/op | +| `list()` | 9-13 ms | 11-16 ms | 38 ms | 49-55 ms | +| `flush_expired(10000)` | 13-15 ms | 13-16 ms | 30-31 ms | 56-61 ms | + +Both variants vary by about a factor of two between runs at these dict sizes, so +the ranges overlap and none of these differences are established. The §6.1 +measurement, where the two differ by 60x, is the one that is. + +### 6.3 Request latency + +`wrk2`, 40s, plus new series arriving as ordinary requests, spread over the +workers. + +| | v1.0.0 | this branch | +|---|---|---| +| 100m / 300k, 5k req/s + 100 new series/s: p50 | 0.97ms | 0.96ms | +| p90 | 504ms | 470ms | +| p99 | 1.62s | 1.58s | +| 100m / 300k, 20k req/s + 400 new series/s: p90 | 739ms | 756ms | +| p99 | 1.96s | 2.11s | +| 500m / 1.5M, 2k req/s + 40 new series/s: p50 | 3.93s | 3.41s | +| achieved rps | 75 | 76 | + +The tails are large in both variants and for the same reason: rendering the +exposition for 300k series takes ~440ms and for 1.5M series ~3.6s, every +`refresh_interval`, and that runs against the same dict the workers write to. +At 1.5M series neither variant can serve 2k req/s on this box. What matters +here is that the two are indistinguishable. + +An earlier revision was *not* indistinguishable: it walked every slot on every +scrape and, on a full dict, could not publish its trail, so the scrape fell back +to that walk permanently. It measured p90 500ms against 123ms for v1.0.0 in the +20k req/s run. Both causes were fixed (3.5, 3.6). + +### 6.4 Reclamation lock hold (`resty --shdict`, single process) + +| dict contents | call | lock hold | +|---|---|---| +| 750k entries, 749.7k expired | `flush_expired()` | 70ms | +| same | `flush_expired(10000)` | ~1ms per call, 26 calls | +| 301k entries, 1k expired | either | ~3ms (walks the whole queue) | + +~0.09us per entry reclaimed, ~0.01us per live node walked. The walk is the +floor: in the steady state, where the backlog is smaller than a batch, every +call still walks the queue once. + +## 7. Risks and follow-ups + +- **A scrape that loses the trail walks every slot**, which is 0.4s at 400k + slots and seconds at 1.5M. The ring is pre-created so that publishing cannot + fail, and its size scales with the dict, but a scrape that falls more than + `ring_size` reuses behind still pays for one walk. +- **Two workers can hold a slot each for the same key.** Bounded by the number + of workers, listed once, reclaimed on the next expiry; 0.02% of slots in the + churn run above. +- **Reuse depends on reclamation.** A number becomes reusable only once its node + is gone, so the reclamation interval sets how quickly numbers come back. 10s + to 60s is a reasonable range. +- **The scrape itself is the bottleneck at 1.5M series** in both variants, and + this RFC does not address it. From 14d256b53d2fa7205a5648703530dc6fde14e9d9 Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Thu, 24 Sep 2026 09:50:38 +0800 Subject: [PATCH 2/5] docs(rfc): add the value-level consistency checks Comparing the scrape against a walk of the slots only proves the two agree about slots. It cannot catch a key whose slot was taken by another one: it stays unregistered, its value is still in the dict, and it is missing from the exposition while every slot-level count matches. Two checks inside the scraping process close that: every live value in the dict is rendered exactly once, and a known number of increments adds up exactly. Run after a batch of slots has been retired and reclaimed, so the counted series take those numbers: 15 of 20 landed on recycled numbers, values exact, nothing rendered twice, nothing missing. --- rfcs/0001-slot-reuse-and-bounded-reclaim.md | 35 +++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/rfcs/0001-slot-reuse-and-bounded-reclaim.md b/rfcs/0001-slot-reuse-and-bounded-reclaim.md index 3555cb8..7bb21e3 100644 --- a/rfcs/0001-slot-reuse-and-bounded-reclaim.md +++ b/rfcs/0001-slot-reuse-and-bounded-reclaim.md @@ -205,6 +205,41 @@ round expiring before the next, on 10 workers: | `key_count` after 12 rounds | 383 -> 934 (+50 per round) | 254, flat | | consistency check each round | pass | pass | +### 5.3 What the slot-level check cannot see + +Comparing what the scrape lists against a walk of `1..key_count` only proves +the two agree about the *slots*. It cannot catch the failure this design has to +rule out: a key whose slot was taken by someone else stays unregistered, its +value is still in the dict, and it is missing from the exposition while every +slot-level count still matches. + +So two further checks run inside the scraping process, on 10 workers: + +- **every live value is rendered exactly once** -- the ground truth is the set + of dict keys that are not index bookkeeping and still read non-nil, compared + against the series in the rendered exposition; +- **counter values survive slot reuse** -- a known number of increments is + driven through all the workers, and each series' value must be exactly that. + +To make the reuse actually happen for the counted series, the run retires a +batch of slots first (20s of churn, then the expiry and one reclaim round), and +only then registers them, so they take the numbers just given up: + +| | v1.0.0 | this branch | +|---|---|---| +| entries reclaimed by the round | 0 (hourly timer) | 65,396 | +| slots added by the 20 counted series | 20 | **5** (15 landed on recycled numbers) | +| counter values exactly as driven | yes | **yes** | +| live values in the dict / series rendered | 200,020 / 200,021 | 200,020 / 200,021 | +| rendered twice | 0 | **0** | +| live value missing from the output | 0 | **0** | +| slot-level: listed / live slots / duplicates / missing | 200,021 / 200,021 / 0 / 0 | 200,021 / 200,021 / 0 / 0 | + +The one series rendered without a value is the same on both variants: it expired +between the enumeration and the render, which is a race in the check, not in the +library. The same two checks also pass at 300k series with churn running +throughout (1,500 new series/s). + ## 6. Performance report 20-core x86-64, OpenResty 1.29.2.4, load generator on the same host. Each From 34628b4d1f859ef98c1b62514420541f19dcf64e Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Thu, 24 Sep 2026 10:20:57 +0800 Subject: [PATCH 3/5] test(keys): cover the scenarios the first round missed Five of them were only reasoned about: a scrape further behind than the trail can hold, a trail entry evicted, two workers racing for the same reclaimed number, remove() freeing a number that is then reused, and a metric with no exptime at all, where none of this may engage. The sixth found something. The numbers a previous generation of workers gave up -- what a reload or a restart leaves behind -- are in nobody's expire_keys, so nothing ever reused them and key_count kept the old high water mark. A reclaim round now also walks a slice of the range for numbers whose node is gone, a tenth of it per round at one read per slot, which took the growth over 20s of churn on an inherited dict from +14.6k to +7.2k. Also verified, on 10 workers: an in-place upgrade from 1.0.0 over the same shm, and a rollback to 1.0.0 over a dict this code wrote, both correct from the first scrape; a dict kept at 0 free space, where both versions lose most of the exposition and neither is better; and the metric shapes -- a histogram writes 18 keys per observation, renewal 6.65us/op against 7.05, registration 49us/op against 43. --- prometheus_keys.lua | 68 +++++++++- prometheus_test.lua | 142 +++++++++++++++++++- rfcs/0001-slot-reuse-and-bounded-reclaim.md | 85 ++++++++++++ 3 files changed, 293 insertions(+), 2 deletions(-) diff --git a/prometheus_keys.lua b/prometheus_keys.lua index 5a22511..0d7930d 100644 --- a/prometheus_keys.lua +++ b/prometheus_keys.lua @@ -31,7 +31,14 @@ local FLUSH_EXPIRED_BATCH_DELAY = 1 -- Slot numbers this worker keeps for reuse after seeing them reclaimed. They -- go to the next keys that need one, so a registration costs the same single -- write it always did: a number taken in the meantime just fails that write. -local FREE_SLOTS_KEEP = 1024 +local FREE_SLOTS_KEEP = 8192 + +-- Slots examined per reclaim round for numbers that died while this worker was +-- not looking: before it started, or in a slot it never held. A tenth of the +-- range per round covers all of it in ten, and each slot is one read. +local RECLAIM_SCAN_DIVISOR = 10 +local RECLAIM_SCAN_MIN = 1000 +local RECLAIM_SCAN_MAX = 50000 -- Reclaimed numbers tried before falling back to a fresh slot past key_count. local FREE_SLOT_ATTEMPTS = 4 @@ -72,6 +79,7 @@ function KeyIndex.new(shared_dict, prefix, remove_expired_keys_interval, self.seen_reuse = 0 self.free_slots = {} self.free_n = 0 + self.scan_cursor = 1 self.last = 0 self.deleted = 0 self.not_expired_index = 1 @@ -136,6 +144,48 @@ function KeyIndex:remove_expired_keys() self:hide_slot(i) end end + + self:scan_for_free_slots() +end + + +-- The loop above only reaches the slots this worker registered itself. A number +-- that died before it started -- what a reload or a restart leaves behind -- or +-- one that died in a slot it never held is in nobody's expire_keys here, and +-- without this pass it would never be reused: key_count would keep the high +-- water mark of the previous generation for ever. +function KeyIndex:scan_for_free_slots() + local last = self.dict:get(self.key_count) or 0 + if last < 1 or self.free_n >= FREE_SLOTS_KEEP then + return + end + + local window = math.max(RECLAIM_SCAN_MIN, + math.min(RECLAIM_SCAN_MAX, math.floor(last / RECLAIM_SCAN_DIVISOR))) + local i = self.scan_cursor + for _ = 1, window do + if i > last then + i = 1 + end + + if not self.keys[i] and self.dict:get(self.key_prefix .. i) == nil then + -- only a node that is gone frees its number; one that is merely past its + -- ttl still belongs to the key that can take it back + local _, err = self.dict:ttl(self.key_prefix .. i) + if err == "not found" then + self.free_n = self.free_n + 1 + self.free_slots[self.free_n] = i + if self.free_n >= FREE_SLOTS_KEEP then + i = i + 1 + break + end + end + end + + i = i + 1 + end + + self.scan_cursor = i end @@ -192,6 +242,12 @@ end -- Iterates keys from first to last, adds new items and removes deleted items. function KeyIndex:sync_range(first, last) + -- A walk of the whole range is also the only chance a worker gets to notice + -- the slots that died before it started -- after a reload, or a restart, the + -- numbers a previous generation gave up are in nobody's expire_keys, and + -- without this they would never be reused. + local whole_range = first == 0 + for i = first, last do -- Read i-th key. If it is nil, it means it was deleted by some other thread. local key = self.dict:get(self.key_prefix .. i) @@ -205,6 +261,16 @@ function KeyIndex:sync_range(first, last) self.expire_keys[i] = true end end + elseif whole_range and i > 0 and not self.keys[i] + and self.free_n < FREE_SLOTS_KEEP then + -- a slot this worker has no record of: only its node being gone makes the + -- number reusable, and only ttl() can tell that from a node that is + -- merely past its ttl and still belongs to its own key + local _, err = self.dict:ttl(self.key_prefix .. i) + if err == "not found" then + self.free_n = self.free_n + 1 + self.free_slots[self.free_n] = i + end elseif self.keys[i] then -- The slot holds no live key, which is all a scrape needs to know, so it -- is only hidden here -- one read per slot. Telling "past its ttl" from diff --git a/prometheus_test.lua b/prometheus_test.lua index dc71051..cb2a434 100644 --- a/prometheus_test.lua +++ b/prometheus_test.lua @@ -19,7 +19,7 @@ function SimpleDict:set(k, v, exptime) end return true, nil, forcible end -function SimpleDict:capacity() +function SimpleDict.capacity() return 10 * 1024 * 1024 -- like a lua_shared_dict of 10m end function SimpleDict:safe_set(k, v, exptime) @@ -1115,6 +1115,146 @@ function TestKeyIndex:testReclaimedSlotAboveKeyCountIsMadeVisible() end +-- G5: a scrape that has fallen further behind than the trail can hold, or that +-- finds an entry of it gone, must fall back to walking every slot rather than +-- trust an incremental sync -- the slots it would miss are live. +function TestKeyIndex:testScrapeFallsBackWhenTheTrailIsTooShort() + luaunit.assertEquals(self.key_index:add("gone", "eviction_err", 1), nil) + local scraper = require('prometheus_keys').new(self.dict, "_prefix_", 1) + luaunit.assertEquals(#scraper:list(), 1) + + sleep(2) + self.key_index:remove_expired_keys() + luaunit.assertEquals(self.key_index:add("newcomer", "eviction_err", 60), nil) + luaunit.assertEquals(self.dict:get("_prefix_key_1"), "newcomer") + + -- the scrape is made to look further behind than the ring can hold + scraper.seen_reuse = -scraper.ring_size - 10 + luaunit.assertEquals(scraper:list(), {"newcomer"}) +end + + +function TestKeyIndex:testScrapeFallsBackWhenATrailEntryIsGone() + luaunit.assertEquals(self.key_index:add("gone", "eviction_err", 1), nil) + local scraper = require('prometheus_keys').new(self.dict, "_prefix_", 1) + scraper:list() + + sleep(2) + self.key_index:remove_expired_keys() + luaunit.assertEquals(self.key_index:add("newcomer", "eviction_err", 60), nil) + + -- the entry that would point at the reused slot is evicted + local seq = self.dict:get("_prefix_reuse_count") + self.dict:delete("_prefix_reuse_slot_" .. seq % scraper.ring_size) + + luaunit.assertEquals(scraper:list(), {"newcomer"}) +end + + +-- G6: two workers can put the same reclaimed number aside. Whoever writes it +-- first keeps it; the other must notice and take a different one, so the key +-- does not end up sharing a slot. +function TestKeyIndex:testTwoWorkersRacingForOneReclaimedSlot() + local w1 = require('prometheus_keys').new(self.dict, "_prefix_", 1) + local w2 = require('prometheus_keys').new(self.dict, "_prefix_", 1) + luaunit.assertEquals(w1:add("gone", "eviction_err", 1), nil) + w2:sync() + + sleep(2) + w1:remove_expired_keys() + w2:remove_expired_keys() + -- both now hold slot 1 as reclaimed + luaunit.assertEquals(w1.free_slots[w1.free_n], 1) + luaunit.assertEquals(w2.free_slots[w2.free_n], 1) + + luaunit.assertEquals(w1:add("first", "eviction_err", 60), nil) + luaunit.assertEquals(w2:add("second", "eviction_err", 60), nil) + + luaunit.assertEquals(self.dict:get("_prefix_key_1"), "first") + luaunit.assertNotEquals(w2.index["second"], 1) + luaunit.assertEquals(self.dict:get("_prefix_key_" .. w2.index["second"]), "second") + + local scraper = require('prometheus_keys').new(self.dict, "_prefix_", 1) + local listed = {} + for _, k in ipairs(scraper:list()) do + listed[k] = (listed[k] or 0) + 1 + end + luaunit.assertEquals(listed, {first = 1, second = 1}) +end + + +-- G7: remove() gives the number up for reuse, and a key that is registered +-- again afterwards must not end up sharing a slot with whoever took it. +function TestKeyIndex:testRemovedSlotIsReusedWithoutMixingKeys() + luaunit.assertEquals(self.key_index:add("dropme", "eviction_err", 60), nil) + luaunit.assertEquals(self.key_index:add("stay", "eviction_err", 60), nil) + luaunit.assertEquals(self.key_index:remove("dropme", "eviction_err"), nil) + luaunit.assertNil(self.key_index.index["dropme"]) + + -- the freed number goes to the next key that needs one + luaunit.assertEquals(self.key_index:add("newcomer", "eviction_err", 60), nil) + luaunit.assertEquals(self.key_index.index["newcomer"], 1) + luaunit.assertEquals(self.dict:get("_prefix_key_count"), 2) + + -- and the removed key, registered again, takes a slot of its own + luaunit.assertEquals(self.key_index:add("dropme", "eviction_err", 60), nil) + luaunit.assertNotEquals(self.key_index.index["dropme"], 1) + luaunit.assertEquals(self.dict:get("_prefix_key_1"), "newcomer") + + local scraper = require('prometheus_keys').new(self.dict, "_prefix_", 1) + local listed = {} + for _, k in ipairs(scraper:list()) do + listed[k] = (listed[k] or 0) + 1 + end + luaunit.assertEquals(listed, {stay = 1, newcomer = 1, dropme = 1}) +end + + +-- G8: with no exptime -- the default in APISIX -- nothing ever expires, so none +-- of the reuse machinery may come into play: no slot is ever given up, nothing +-- is published, and the shared counters stay where they are. +function TestKeyIndex:testPermanentMetricsNeverReuseSlots() + for i = 1, 3 do + luaunit.assertEquals(self.key_index:add("perm" .. i, "eviction_err"), nil) + end + self.key_index:remove_expired_keys() + for i = 1, 3 do + luaunit.assertEquals(self.key_index:add("perm" .. i, "eviction_err"), nil) + end + + luaunit.assertEquals(self.dict:get("_prefix_key_count"), 3) + luaunit.assertEquals(self.dict:get("_prefix_reuse_count"), nil) + luaunit.assertEquals(self.dict:get("_prefix_delete_count"), nil) + luaunit.assertEquals(self.key_index.free_n, 0) + luaunit.assertEquals(#self.key_index:list(), 3) +end + + +-- G1: the numbers a previous generation of workers gave up -- what an upgrade +-- or a restart leaves behind -- are in nobody's expire_keys. A worker that +-- walks the whole range must pick them up, or the inherited backlog is never +-- reused and the scan range never comes back down. +function TestKeyIndex:testSlotsInheritedFromAnEarlierGenerationAreReused() + for i = 1, 3 do + luaunit.assertEquals(self.key_index:add("old" .. i, "eviction_err", 1), nil) + end + sleep(2) + self.key_index:flush_expired() + + -- a worker that starts now has no record of any of them + local fresh = require('prometheus_keys').new(self.dict, "_prefix_", 1) + luaunit.assertEquals(fresh.free_n, 0) + fresh:sync() -- its first sync walks the whole range + luaunit.assertEquals(fresh.free_n, 3) + + for i = 1, 3 do + luaunit.assertEquals(fresh:add("new" .. i, "eviction_err", 60), nil) + end + luaunit.assertEquals(self.dict:get("_prefix_key_count"), 3) + luaunit.assertEquals(#fresh:list(), 3) +end + + -- flush_expired() holds the dict lock for its whole scan of the LRU queue, so -- the reclamation is issued in bounded batches. A backlog larger than one batch -- must still be reclaimed in full, by looping. diff --git a/rfcs/0001-slot-reuse-and-bounded-reclaim.md b/rfcs/0001-slot-reuse-and-bounded-reclaim.md index 7bb21e3..38e15a0 100644 --- a/rfcs/0001-slot-reuse-and-bounded-reclaim.md +++ b/rfcs/0001-slot-reuse-and-bounded-reclaim.md @@ -240,6 +240,75 @@ between the enumeration and the render, which is a race in the check, not in the library. The same two checks also pass at 300k series with churn running throughout (1,500 new series/s). +### 5.4 Scenario coverage + +Every row is a scenario this design has to survive, what it must guarantee, and +where the evidence is. "unit" is `prometheus_test.lua`; the rest run on +OpenResty with 10 workers and the privileged agent. + +| scenario | must hold | evidence | +|---|---|---| +| steady state, 300k and 1.5M series | output = live set, no duplicates | multi-worker, §5.2 | +| a series expires | it leaves the output | unit + multi-worker | +| it comes back, node still there (state 2) | same slot, nothing broadcast | unit | +| it comes back, node reclaimed (state 3) | same slot, peers do not miss it | unit | +| a dead label's number goes to a new series | correct value, listed once | §5.3, 15 of 20 landed on recycled numbers | +| a slot only past its ttl | not given to another key | unit | +| a stale index entry after reuse | must not renew the new occupant | unit | +| two workers racing for one reclaimed number | one wins, the other takes another | unit | +| `remove()` frees a number that is then reused | keys do not get mixed up | unit | +| no `exptime` at all (the APISIX default) | none of the reuse machinery engages | unit + §6.2 | +| `key_count` LRU-evicted | a slot above it is still listed | unit | +| the trail is shorter than the scrape's lag | fall back to walking every slot | unit | +| a trail entry is evicted | same fallback | unit | +| slots inherited from an earlier generation | reused, not stranded | unit + §5.5 | +| in-place upgrade from 1.0.0, shm kept | correct from the first scrape | §5.5 | +| rollback to 1.0.0 over a dict this code wrote | 1.0.0 stays correct | §5.5 | +| a dict kept at 0 free space | *neither* version is correct here -- see §5.6 | §5.6 | +| counter values across all of the above | exactly what was driven | §5.3 | + +### 5.5 Upgrade and rollback + +A reload keeps the shm zone, so swapping the library under a running instance is +what an in-place upgrade looks like: the new code inherits a dict that 1.0.0 +wrote -- 39k dead slots, `key_count` well above the live count, no trail -- and +then 1.0.0 inherits one this code wrote, with reused slots and a trail it knows +nothing about. + +| stage | `key_count` | live slots | duplicates | live values missing | first scrape | +|---|---|---|---|---|---| +| on 1.0.0, before | 239,366 | 200,001 | 0 | 0 | -- | +| after the upgrade | 239,366 | 200,001 | 0 | 0 | 272ms | +| after 20s of churn on the inherited state | 246,568 | 200,001 | 0 | 0 | -- | +| after rolling back to 1.0.0 | 246,568 | 200,001 | 0 | 0 | 257ms | + +The first run of this exposed a gap rather than a bug: the numbers a previous +generation of workers gave up are in nobody's `expire_keys`, so they were never +reused and `key_count` kept the old high-water mark. A reclaim round now also +walks a slice of the range looking for numbers whose node is gone (a tenth per +round, one read per slot), which brought the growth over the same 20s of churn +from +14.6k to +7.2k. The residual is the working set of simultaneously live +churn series, which has to have numbers. + +### 5.6 A dict with no free space + +Kept at 0 free space by churn beyond its capacity, with `key_count` evicted as +well, both versions produce a broken exposition: of ~405k live values, 381k +(1.0.0) and 301k (this branch) were missing from the output, and `key_count` +itself was evicted and restarted. Slot reuse does not fix this and does not make +it worse -- it is what the dict does when it is too small for the cardinality, +and it is the state that apache/apisix#13658's bloat drives a gateway into. + +Under a churn rate the dict *can* absorb (3,000 new series/s for 60s on 100m, +starting from 150k live series), both stay correct, all 500 continuously-hit +series keep their values, and the difference is the numbering: + +| | v1.0.0 | this branch | +|---|---|---| +| `key_count` after 60s | 150,501 -> 298,420 | 150,501 -> **260,468** | +| growth in the last 15s | +12.6k/5s, flat | **+4.5k/5s, falling** | +| live values missing from the output | 0 | 0 | + ## 6. Performance report 20-core x86-64, OpenResty 1.29.2.4, load generator on the same host. Each @@ -272,6 +341,22 @@ counters, so a bump cannot make it scan. ### 6.2 Microbenchmarks +Metric shapes, at 200k live series on 100m, with the scrape and the reclamation +paused: + +| path | v1.0.0 | this branch | +|---|---|---| +| counter renewal (1 dict write per request) | 0.91 us/op | **0.60 us/op** | +| histogram renewal (18 keys per observation) | 7.05 us/op | **6.65 us/op** | +| counter registration | 5.0 us/op | 6.3 us/op | +| histogram registration | 43 us/op | 49 us/op | + +Registration is the one path that pays for reuse: about one extra dict +operation per key, for `ensure_key_count` and for publishing the number on the +trail. It happens once per series per worker, against the renewal path that runs +on every request. + + | | 100m / 300k | | 500m / 1.5M | | |---|---|---|---|---| | | v1.0.0 | branch | v1.0.0 | branch | From c9199326351c8d494d3cd9e822deba293f001657 Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Thu, 24 Sep 2026 11:19:35 +0800 Subject: [PATCH 4/5] fix(keys): size the reuse trail for the churn it has to keep up with Fifteen minutes of 1,500 new series/s put the scrape at 340-450ms against 220-243ms on 1.0.0. The trail held one entry per 64 KiB of dict, which is 1,600 entries on a 100m dict, while that rate is 3,000 reuses between two scrapes -- so the scrape fell further behind than the trail on every round and walked all 300k slots each time, which is correct but is the one thing the trail exists to avoid. One entry per 16 KiB, floor 1024, cap 8192: 0.9% of the dict, and the scrape comes back to 228-237ms. key_count over the same run: 1.0.0 grows to 1,318,746 and is still linear, this branch settles at 303,422. --- prometheus_keys.lua | 9 +++++---- rfcs/0001-slot-reuse-and-bounded-reclaim.md | 21 +++++++++++++++++++++ 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/prometheus_keys.lua b/prometheus_keys.lua index 0d7930d..825f56a 100644 --- a/prometheus_keys.lua +++ b/prometheus_keys.lua @@ -49,9 +49,12 @@ local FREE_SLOT_ATTEMPTS = 4 -- publishing one is an in-place write that cannot fail for want of memory -- -- on a full dict a write that had to allocate would either evict a metric or -- be dropped, and a dropped one costs the scrape a walk over every slot. +-- One entry per 16 KiB of the dict, so the trail costs about 0.9% of it and +-- covers a few thousand reuses between two scrapes -- past that the scrape +-- walks every slot, which is correct but is what the trail exists to avoid. local REUSE_RING_MAX = 8192 -local REUSE_RING_MIN = 256 -local REUSE_RING_BYTES_PER_ENTRY = 65536 +local REUSE_RING_MIN = 1024 +local REUSE_RING_BYTES_PER_ENTRY = 16384 local REUSE_ENTRY_FORMAT = "%012d:%09d" -- Attempts at raising key_count above a slot number. Concurrent raises can @@ -88,8 +91,6 @@ function KeyIndex.new(shared_dict, prefix, remove_expired_keys_interval, self.hidden = {} self.expire_keys = {} - -- One entry per 64 KiB of the dict, so the trail costs about 0.2% of it and - -- stays useful on the dicts big enough for a full scan to be expensive. local capacity = self.dict.capacity and self.dict:capacity() or 0 self.ring_size = math.max(REUSE_RING_MIN, math.min(REUSE_RING_MAX, math.floor(capacity / REUSE_RING_BYTES_PER_ENTRY))) diff --git a/rfcs/0001-slot-reuse-and-bounded-reclaim.md b/rfcs/0001-slot-reuse-and-bounded-reclaim.md index 38e15a0..f4c7bcb 100644 --- a/rfcs/0001-slot-reuse-and-bounded-reclaim.md +++ b/rfcs/0001-slot-reuse-and-bounded-reclaim.md @@ -395,6 +395,27 @@ scrape and, on a full dict, could not publish its trail, so the scrape fell back to that walk permanently. It measured p90 500ms against 123ms for v1.0.0 in the 20k req/s run. Both causes were fixed (3.5, 3.6). +### 6.4 Fifteen minutes of churn + +1,500 new series/s on a 100m dict, 10 workers, starting from 150k long-lived +series: + +| | v1.0.0 | this branch | +|---|---|---| +| `key_count` at 1 min | 222,520 | 195,734 | +| at 5 min | 522,930 | 235,288 | +| at 15 min | **1,318,746** | **303,422** | +| growth in the last minute | +81k, linear | **+5k, falling** | +| dict free space | stable | stable | +| scrape | 220-243ms | 228-237ms | +| duplicates / live values missing at the end | 0 / 0 | 0 / 0 | + +The first run of this had the scrape at 340-450ms on the branch, because the +trail was sized at one entry per 64 KiB -- 1,600 entries on a 100m dict -- while +1,500 new series/s over a 2s scrape interval is 3,000 reuses, so every scrape +fell back to walking every slot. One entry per 16 KiB (§3.5) covers that rate and +brings the scrape back in line; the fallback remains for anything faster. + ### 6.4 Reclamation lock hold (`resty --shdict`, single process) | dict contents | call | lock hold | From 202e3fb5f0838c1c13f3e3369dd506e34315d4e4 Mon Sep 17 00:00:00 2001 From: AlinsRan Date: Thu, 24 Sep 2026 11:31:56 +0800 Subject: [PATCH 5/5] docs: move the design out of the code repo The design, the scenario matrix and the measurements live in api7/rfcs#290, where they can be reviewed as a proposal instead of shipping with the library. --- CHANGELOG.md | 2 +- rfcs/0001-slot-reuse-and-bounded-reclaim.md | 444 -------------------- 2 files changed, 1 insertion(+), 445 deletions(-) delete mode 100644 rfcs/0001-slot-reuse-and-bounded-reclaim.md diff --git a/CHANGELOG.md b/CHANGELOG.md index bb5bac1..ee81a91 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,7 @@ of changes. one low-frequency series returning no longer sends every worker through a full sync on its request path (apache/apisix#13658). The slot count now tracks the number of live series instead of growing for ever under label - churn. See `rfcs/0001-slot-reuse-and-bounded-reclaim.md`. + churn. Design and measurements: api7/rfcs#290. ## 1.0.0 diff --git a/rfcs/0001-slot-reuse-and-bounded-reclaim.md b/rfcs/0001-slot-reuse-and-bounded-reclaim.md deleted file mode 100644 index f4c7bcb..0000000 --- a/rfcs/0001-slot-reuse-and-bounded-reclaim.md +++ /dev/null @@ -1,444 +0,0 @@ -# RFC 0001: bounded reclamation and slot reuse in KeyIndex - -Status: proposed -Tracking: apache/apisix#13658, apache/apisix#11934, apache/apisix#12275 - -## 1. Problem - -`KeyIndex` gives every metric name a numbered slot in the shared dict -(`__ngx_prom__key_N`) so that a scrape can enumerate the metrics without -`get_keys()`. `key_count` is the highest number handed out, and a full sync -walks `0..key_count`. - -Three problems come out of that, all of them only when metrics are registered -with an `exptime`. - -### 1.1 One low-frequency series coming back pegs every worker - -When a metric comes back after its slot is gone, `add()` bumps `delete_count` -(added in #14 so that peers stop listing a stale slot). Every worker's next -`sync()` then walks `0..key_count` -- and `sync()` runs on the request path, -once per observation of a metric with an `exptime`. - -Measured on the shape of a 3.9.x gateway pod (§6.1): with 140k slots and 200 -req/s, a single such return doubles the CPU of the worker set for the next -window, and 50 of them over 10s put three workers at 98%, 93% and 88% of a -core. That is the `top` picture behind the reports. - -### 1.2 Slot numbers are never reused - -A metric whose labels are never seen again leaves its number behind for good, -and a metric that expires and comes back gets a *new* one. `key_count` only -grows, and everything proportional to it grows with it. The dump in -apache/apisix#13658 had 747,970 dead index entries against 28 live series. - -### 1.3 The reclamation holds the dict lock for a whole backlog - -`remove_expired_keys()` calls `flush_expired()` with no bound, in every worker. -That call holds the dict mutex until it returns and walks the whole LRU queue, -so an hour's worth of expired entries is reclaimed in one uninterrupted hold, -and every worker repeats the walk (#23 in this repo). - -## 2. What the shared dict actually offers - -A slot is in one of three states, and `get()` reports the last two alike: - -| state | `get()` | `ttl()` | `expire()` | `add()` | -|---|---|---|---|---| -| (1) live | the key | `> 0` | renews | `"exists"` | -| (2) past its ttl, node still in the dict | `nil` | `< 0` | resurrects it, value intact | replaces in place | -| (3) node physically reclaimed | `nil` | `nil, "not found"` | `"not found"` | creates it | - -Verified on OpenResty 1.29.2.4 (`resty --shdict`) and in the source: `ttl()` -goes through `ngx_http_lua_shdict_peek()`, which neither checks the expiry nor -touches the LRU position, while `get()` goes through -`ngx_http_lua_shdict_lookup()`, which returns `NGX_DONE` for an expired node. - -This is the basis of the design: **`get()` answers "is this key visible", -`ttl()` answers "does this node still exist"**. The second question is what -decides whether a slot number still belongs to its key. - -## 3. Design - -### 3.1 The renewal path checks ownership instead of syncing - -`add()` used to start with `sync()` -- two reads of the shared counters -- and -then renew the slot `self.index[key]` points at. It now reads the slot itself: - -```lua -if self.dict:get(self.key_prefix .. idx) == key then - self.dict:expire(self.key_prefix .. idx, exptime) -- done -end -``` - -One read instead of two, and none of the shared counters are touched, so -nothing another worker does can force this path into a full sync. It is also -what makes slot reuse safe: an index entry left over from a slot that has since -been handed to another key does not match here, where renewing it blindly would -extend a foreign key's ttl and leave this key unregistered. - -`delete_count` is no longer bumped when a metric comes back (1.1), and -`remove()` -- which APISIX never calls on prometheus metrics -- remains the only -writer of it. - -### 3.2 A key takes its own slot back in place - -If the slot holds no visible key -- state (2) or (3) -- the key takes the number -back with `add()`, verifying the occupant if that comes back `"exists"` (a peer -may have taken it back first). The key keeps the number every worker already -knows, so nothing has to be broadcast and `key_count` does not grow when a -metric expires and comes back. - -### 3.3 Reclaimed numbers are reused by other keys - -`clear_slot()` runs when a worker sees a slot in state (3); it puts the number -in a bounded per-worker list. A key that needs a slot takes one from there and -writes it exactly as it would write a fresh one -- a number taken in the -meantime simply fails that write and is dropped. Reuse therefore costs no -search: no shared cursor, no scan, no extra read. - -### 3.4 `key_count` is raised above a slot taken in place - -A slot above `key_count` is invisible to a full scan. That happens when -`key_count` -- an ordinary dict entry -- is LRU-evicted and `incr()` re-creates -it below the slots in use. `ensure_key_count(idx)` raises it, re-checking the -result because a lost race can only overshoot. - -### 3.5 The scrape follows a trail of reused slots - -Slots are written below the other workers' `self.last`, where an incremental -sync would never read them again. Every such write publishes `":"` -into a ring in the dict and bumps `reuse_count`; `list()` is the only reader. -When the counter has moved, the scrape re-reads just those slots; only a scrape -that has fallen further behind than the ring, or that finds the trail -incomplete, walks every slot. - -The ring entries are created up front and always rewritten with a value of the -same length, so publishing is an in-place write that cannot fail for want of -memory. This matters: while publishing used `safe_set` on a not-yet-existing -entry, a full dict dropped the write, every scrape fell back to walking 400k -slots, and the tail latency of the whole gateway went with it (§6.3). The ring -holds one entry per 64 KiB of dict capacity, between 256 and 8192. - -### 3.6 The scan is one read per slot - -A slot that holds no live key is *hidden*: dropped from the listing, slot number -kept. Telling state (2) from state (3) costs a second read and decides whether -the number can be given up, so it is done by `remove_expired_keys()`, on a -timer, not on every scrape. - -### 3.7 The local views cannot drift apart - -`self.keys` (slot -> key) and `self.index` (key -> slot) are maintained only -through `set_slot` / `hide_slot` / `clear_slot`. Leaving a previous occupant's -index entry behind would point a later `add()` at a slot that is no longer its -own; dropping the index entry of a key that has since moved would hide a live -key from `list()`. - -## 4. Invariants - -1. **Ownership.** A worker only renews a slot whose current value is its own key - (3.1). Nothing else can extend a foreign key's ttl. -2. **Identity.** A number is reused by another key only after its node is gone - (3.3), and only through `add()`, which fails on a live node. -3. **Visibility.** Every live slot is within `0..key_count` (3.4), and a slot - written below `self.last` is either on the trail the scrape follows or causes - a full walk (3.5). -4. **Uniqueness.** `list()` emits a key only from the slot its own index points - at, so a key that transiently sits on two slots is listed once. -5. **Locality.** `self.keys` and `self.index` are mutual inverses (3.7). - -## 5. Scenario tests - -### 5.1 Unit (`prometheus_test.lua`) - -The `SimpleDict` mock now models the three states of §2: `get()` no longer -prunes an expired node, `ttl()` returns a negative number for state (2) and -`"not found"` for state (3), `expire()` resurrects a state-(2) node, and `add()` -replaces one in place. Tests that assumed the old semantics were rewritten -rather than patched. - -| test | what it pins down | -|---|---| -| `testExpiredReAddReclaimsSameSlot` | a metric that comes back takes its own slot; `key_count` and `delete_count` do not move; a second worker that never re-synced still lists it once | -| `testReclaimedSlotIsTakenBackInPlace` | same, after the node itself was reclaimed; a second worker sees it although the write was below its `self.last` | -| `testReclaimedSlotsAreReused` | three retired numbers are reused by three new metrics; `key_count` stays at 3 | -| `testSlotsOnlyPastTheirTtlAreNotReused` | a state-(2) slot is not given to another key, and its own key still gets it back | -| `testReusedSlotIsNotHijackedByAStaleIndex` | a worker holding a stale index does not renew the new occupant, registers its own key elsewhere, and a scrape lists both exactly once | -| `testKeyCountStaysBoundedUnderChurn` | five rounds of retire-and-register reuse one slot | -| `testReclaimedSlotAboveKeyCountIsMadeVisible` | with `key_count` evicted, a slot taken back above it is still listed by a fresh worker | -| `testRemoveExpiredKeysKeepsLiveSlots` | a reclaim round leaves live slots alone | -| `testFlushExpiredRunsInBatches` | a 20,000-entry backlog is reclaimed in bounded batches (each call is asserted to ask for 10,000) | -| `testAutoFlushExpiredDisabled` | with the option off, a reclaim round only drops local references | - -53 of 54 tests pass; `TestPrometheus.testPrintfTable` fails on `main` as well, -under LuaJIT, and is unrelated. - -### 5.2 Multi-worker correctness - -OpenResty 1.29.2.4, `worker_processes 10` plus the privileged agent, which -scrapes and (on this branch) reclaims. Every check compares what the scraping -process lists against ground truth taken from the dict itself -- a walk of -`1..key_count` -- inside that same process, so nothing is lost in transport. - -| dict / series | variant | live slots | listed | duplicates | missing | two slots, same key | -|---|---|---|---|---|---|---| -| 100m / 300k | v1.0.0 | 300,001 | 300,001 | 0 | 0 | 0 | -| 100m / 300k | this branch | 300,001 | 300,001 | 0 | 0 | 0 | -| 100m / 300k, after 40s of churn | v1.0.0 | 402,778 | 405,399 | 0 | 0 | 0 | -| 100m / 300k, after 40s of churn | this branch | 402,418 | 402,592 | 0 | 0 | 76 | -| 500m / 1.5M, after 60s of churn | v1.0.0 | 1,500,001 | 1,500,001 | 0 | 0 | 0 | -| 500m / 1.5M, after 60s of churn | this branch | 1,500,001 | 1,500,001 | 0 | 0 | 0 | - -The 76 slots holding the same key are the transient this design allows: two -workers can each end up with a slot for the same key when one of them had -already dropped its reference. `list()` emits the key once (invariant 4), and -the extra slot is reclaimed when the metric next expires. It is 0.02% of the -slots in that run, and the earlier row shows the counterpart: on v1.0.0 it is -the *expired* keys that linger in the listing (2,621 against 250 here). - -Churn also shows what the reuse is for. Twelve rounds of 50 fresh series, each -round expiring before the next, on 10 workers: - -| | v1.0.0 | this branch | -|---|---|---| -| `key_count` after 12 rounds | 383 -> 934 (+50 per round) | 254, flat | -| consistency check each round | pass | pass | - -### 5.3 What the slot-level check cannot see - -Comparing what the scrape lists against a walk of `1..key_count` only proves -the two agree about the *slots*. It cannot catch the failure this design has to -rule out: a key whose slot was taken by someone else stays unregistered, its -value is still in the dict, and it is missing from the exposition while every -slot-level count still matches. - -So two further checks run inside the scraping process, on 10 workers: - -- **every live value is rendered exactly once** -- the ground truth is the set - of dict keys that are not index bookkeeping and still read non-nil, compared - against the series in the rendered exposition; -- **counter values survive slot reuse** -- a known number of increments is - driven through all the workers, and each series' value must be exactly that. - -To make the reuse actually happen for the counted series, the run retires a -batch of slots first (20s of churn, then the expiry and one reclaim round), and -only then registers them, so they take the numbers just given up: - -| | v1.0.0 | this branch | -|---|---|---| -| entries reclaimed by the round | 0 (hourly timer) | 65,396 | -| slots added by the 20 counted series | 20 | **5** (15 landed on recycled numbers) | -| counter values exactly as driven | yes | **yes** | -| live values in the dict / series rendered | 200,020 / 200,021 | 200,020 / 200,021 | -| rendered twice | 0 | **0** | -| live value missing from the output | 0 | **0** | -| slot-level: listed / live slots / duplicates / missing | 200,021 / 200,021 / 0 / 0 | 200,021 / 200,021 / 0 / 0 | - -The one series rendered without a value is the same on both variants: it expired -between the enumeration and the render, which is a race in the check, not in the -library. The same two checks also pass at 300k series with churn running -throughout (1,500 new series/s). - -### 5.4 Scenario coverage - -Every row is a scenario this design has to survive, what it must guarantee, and -where the evidence is. "unit" is `prometheus_test.lua`; the rest run on -OpenResty with 10 workers and the privileged agent. - -| scenario | must hold | evidence | -|---|---|---| -| steady state, 300k and 1.5M series | output = live set, no duplicates | multi-worker, §5.2 | -| a series expires | it leaves the output | unit + multi-worker | -| it comes back, node still there (state 2) | same slot, nothing broadcast | unit | -| it comes back, node reclaimed (state 3) | same slot, peers do not miss it | unit | -| a dead label's number goes to a new series | correct value, listed once | §5.3, 15 of 20 landed on recycled numbers | -| a slot only past its ttl | not given to another key | unit | -| a stale index entry after reuse | must not renew the new occupant | unit | -| two workers racing for one reclaimed number | one wins, the other takes another | unit | -| `remove()` frees a number that is then reused | keys do not get mixed up | unit | -| no `exptime` at all (the APISIX default) | none of the reuse machinery engages | unit + §6.2 | -| `key_count` LRU-evicted | a slot above it is still listed | unit | -| the trail is shorter than the scrape's lag | fall back to walking every slot | unit | -| a trail entry is evicted | same fallback | unit | -| slots inherited from an earlier generation | reused, not stranded | unit + §5.5 | -| in-place upgrade from 1.0.0, shm kept | correct from the first scrape | §5.5 | -| rollback to 1.0.0 over a dict this code wrote | 1.0.0 stays correct | §5.5 | -| a dict kept at 0 free space | *neither* version is correct here -- see §5.6 | §5.6 | -| counter values across all of the above | exactly what was driven | §5.3 | - -### 5.5 Upgrade and rollback - -A reload keeps the shm zone, so swapping the library under a running instance is -what an in-place upgrade looks like: the new code inherits a dict that 1.0.0 -wrote -- 39k dead slots, `key_count` well above the live count, no trail -- and -then 1.0.0 inherits one this code wrote, with reused slots and a trail it knows -nothing about. - -| stage | `key_count` | live slots | duplicates | live values missing | first scrape | -|---|---|---|---|---|---| -| on 1.0.0, before | 239,366 | 200,001 | 0 | 0 | -- | -| after the upgrade | 239,366 | 200,001 | 0 | 0 | 272ms | -| after 20s of churn on the inherited state | 246,568 | 200,001 | 0 | 0 | -- | -| after rolling back to 1.0.0 | 246,568 | 200,001 | 0 | 0 | 257ms | - -The first run of this exposed a gap rather than a bug: the numbers a previous -generation of workers gave up are in nobody's `expire_keys`, so they were never -reused and `key_count` kept the old high-water mark. A reclaim round now also -walks a slice of the range looking for numbers whose node is gone (a tenth per -round, one read per slot), which brought the growth over the same 20s of churn -from +14.6k to +7.2k. The residual is the working set of simultaneously live -churn series, which has to have numbers. - -### 5.6 A dict with no free space - -Kept at 0 free space by churn beyond its capacity, with `key_count` evicted as -well, both versions produce a broken exposition: of ~405k live values, 381k -(1.0.0) and 301k (this branch) were missing from the output, and `key_count` -itself was evicted and restarted. Slot reuse does not fix this and does not make -it worse -- it is what the dict does when it is too small for the cardinality, -and it is the state that apache/apisix#13658's bloat drives a gateway into. - -Under a churn rate the dict *can* absorb (3,000 new series/s for 60s on 100m, -starting from 150k live series), both stay correct, all 500 continuously-hit -series keep their values, and the difference is the numbering: - -| | v1.0.0 | this branch | -|---|---|---| -| `key_count` after 60s | 150,501 -> 298,420 | 150,501 -> **260,468** | -| growth in the last 15s | +12.6k/5s, flat | **+4.5k/5s, falling** | -| live values missing from the output | 0 | 0 | - -## 6. Performance report - -20-core x86-64, OpenResty 1.29.2.4, load generator on the same host. Each -microbenchmark figure is the median of 5 runs issued over one keepalive -connection, so they land on the same worker, with the scrape and the reclamation -paused for the duration. - -### 6.1 The CPU spike of §1.1 - -10 workers, a 512m dict, the three metrics of `apisix/plugins/prometheus/exporter.lua` -with their label sets and the default latency buckets, 140k label combinations -accumulated, then 200 req/s of exactly what `exporter.http_log()` does -(`status:inc` + three `latency:observe` + two `bandwidth:inc`). CPU is the sum -over the workers, from `/proc//stat`, over 10s windows. - -| window | v1.0.0 | this branch | -|---|---|---| -| steady 200 req/s | 14.9% | **7.8%** | -| one low-frequency series comes back | **30.2%** (`delete_count` +1) | **2.2%** (+0) | -| 50 of them over 10s | **287.7%**, busiest workers 98.6 / 93.3 / 87.9% | **4.5%**, busiest 4.0% | - -The low-frequency series is registered before the 140k are filled in, so its -slot is an old one: on v1.0.0 an incremental sync happens to clear a stale index -entry for a *recent* slot, and only an old slot reaches the branch that bumps -`delete_count`. That matches a pod that has been running for days. - -The third row is externally triggered (`delete_count` is bumped directly), and -this branch does not react at all: its request path never reads the shared -counters, so a bump cannot make it scan. - -### 6.2 Microbenchmarks - -Metric shapes, at 200k live series on 100m, with the scrape and the reclamation -paused: - -| path | v1.0.0 | this branch | -|---|---|---| -| counter renewal (1 dict write per request) | 0.91 us/op | **0.60 us/op** | -| histogram renewal (18 keys per observation) | 7.05 us/op | **6.65 us/op** | -| counter registration | 5.0 us/op | 6.3 us/op | -| histogram registration | 43 us/op | 49 us/op | - -Registration is the one path that pays for reuse: about one extra dict -operation per key, for `ensure_key_count` and for publishing the number on the -trail. It happens once per series per worker, against the renewal path that runs -on every request. - - -| | 100m / 300k | | 500m / 1.5M | | -|---|---|---|---|---| -| | v1.0.0 | branch | v1.0.0 | branch | -| renewing a known series | 0.42-0.56 us/op | 0.54-0.70 us/op | 2.04 us/op | 2.14 us/op | -| registering a new series | 3.45-3.75 us/op | 3.50-4.50 us/op | 3.60 us/op | 3.50 us/op | -| `list()` | 9-13 ms | 11-16 ms | 38 ms | 49-55 ms | -| `flush_expired(10000)` | 13-15 ms | 13-16 ms | 30-31 ms | 56-61 ms | - -Both variants vary by about a factor of two between runs at these dict sizes, so -the ranges overlap and none of these differences are established. The §6.1 -measurement, where the two differ by 60x, is the one that is. - -### 6.3 Request latency - -`wrk2`, 40s, plus new series arriving as ordinary requests, spread over the -workers. - -| | v1.0.0 | this branch | -|---|---|---| -| 100m / 300k, 5k req/s + 100 new series/s: p50 | 0.97ms | 0.96ms | -| p90 | 504ms | 470ms | -| p99 | 1.62s | 1.58s | -| 100m / 300k, 20k req/s + 400 new series/s: p90 | 739ms | 756ms | -| p99 | 1.96s | 2.11s | -| 500m / 1.5M, 2k req/s + 40 new series/s: p50 | 3.93s | 3.41s | -| achieved rps | 75 | 76 | - -The tails are large in both variants and for the same reason: rendering the -exposition for 300k series takes ~440ms and for 1.5M series ~3.6s, every -`refresh_interval`, and that runs against the same dict the workers write to. -At 1.5M series neither variant can serve 2k req/s on this box. What matters -here is that the two are indistinguishable. - -An earlier revision was *not* indistinguishable: it walked every slot on every -scrape and, on a full dict, could not publish its trail, so the scrape fell back -to that walk permanently. It measured p90 500ms against 123ms for v1.0.0 in the -20k req/s run. Both causes were fixed (3.5, 3.6). - -### 6.4 Fifteen minutes of churn - -1,500 new series/s on a 100m dict, 10 workers, starting from 150k long-lived -series: - -| | v1.0.0 | this branch | -|---|---|---| -| `key_count` at 1 min | 222,520 | 195,734 | -| at 5 min | 522,930 | 235,288 | -| at 15 min | **1,318,746** | **303,422** | -| growth in the last minute | +81k, linear | **+5k, falling** | -| dict free space | stable | stable | -| scrape | 220-243ms | 228-237ms | -| duplicates / live values missing at the end | 0 / 0 | 0 / 0 | - -The first run of this had the scrape at 340-450ms on the branch, because the -trail was sized at one entry per 64 KiB -- 1,600 entries on a 100m dict -- while -1,500 new series/s over a 2s scrape interval is 3,000 reuses, so every scrape -fell back to walking every slot. One entry per 16 KiB (§3.5) covers that rate and -brings the scrape back in line; the fallback remains for anything faster. - -### 6.4 Reclamation lock hold (`resty --shdict`, single process) - -| dict contents | call | lock hold | -|---|---|---| -| 750k entries, 749.7k expired | `flush_expired()` | 70ms | -| same | `flush_expired(10000)` | ~1ms per call, 26 calls | -| 301k entries, 1k expired | either | ~3ms (walks the whole queue) | - -~0.09us per entry reclaimed, ~0.01us per live node walked. The walk is the -floor: in the steady state, where the backlog is smaller than a batch, every -call still walks the queue once. - -## 7. Risks and follow-ups - -- **A scrape that loses the trail walks every slot**, which is 0.4s at 400k - slots and seconds at 1.5M. The ring is pre-created so that publishing cannot - fail, and its size scales with the dict, but a scrape that falls more than - `ring_size` reuses behind still pays for one walk. -- **Two workers can hold a slot each for the same key.** Bounded by the number - of workers, listed once, reclaimed on the next expiry; 0.02% of slots in the - churn run above. -- **Reuse depends on reclamation.** A number becomes reusable only once its node - is gone, so the reclamation interval sets how quickly numbers come back. 10s - to 60s is a reasonable range. -- **The scrape itself is the bottleneck at 1.5M series** in both variants, and - this RFC does not address it.