Skip to content

Make KeyIndex:add() O(1) so new workers don't scan the whole index on reload - #181

Open
ksvintsov wants to merge 2 commits into
knyar:mainfrom
ksvintsov:o1-key-index-add
Open

ksvintsov wants to merge 2 commits into
knyar:mainfrom
ksvintsov:o1-key-index-add

Conversation

@ksvintsov

Copy link
Copy Markdown

Problem

A fresh worker's KeyIndex starts with last = 0, so its first add() (in Prometheus.init, for
the error metric) and its first timer sync() both run sync_range(0, N): one dict:get per stored
key. On nginx -s reload, every new worker does this at the same moment, all on the same dictionary
lock. New workers don't accept connections until init_worker returns, and old workers stall on the
lock in their counter flushes.

This is #107. With 96 workers and ~35k keys we saw request stalls over 5 s (TTFB) and upstream
timeouts during every reload. With this change, max TTFB during reload is under 1 s.

#178 serialized the first sync with a lock. That avoids the contention, but each worker still scans
the whole index at startup. This PR removes the scan.

Change

  • Each key gets a reverse entry <prefix>rkey_<key> -> slot. add() of a key that is already
    stored costs two dict:gets (the reverse entry, plus the slot to check it), no matter how many
    keys are stored.
  • New keys get a slot the same way as before, then publish the reverse entry with dict:add. If two
    workers add the same key at once, the loser drops its slot and uses the winner's. list() drops
    duplicates as a safety net.
  • A reverse entry whose slot was evicted or reused is replaced with a new slot.
  • The periodic timer calls sync_deletes(), which is O(1) unless delete_count changed. add()
    doesn't need a synced index any more; only deletes have to reach delete_callback. list()
    (collection) still syncs as before.
  • A fresh worker starts with deleted set to the dictionary's current delete_count, since it has
    no lookup tables to invalidate for earlier deletes.
  • Upgrades by reload: the dictionary survives a reload, so keys stored by an older version have
    no reverse entries. Prometheus.init calls build_reverse(). One worker at a time (a lock with
    a TTL) adds the missing reverse entries, scanning only keys added since the previous call
    (rkey_upto). Workers that skip it may create a duplicate slot for an old key, which list() hides.
  • Separate commit: when key_count is lower than an occupied slot (e.g. after flush_all() while a
    worker was adding a key, see prometheuslib.init is blocked in the init_worker_by_lua_block phase. #162), the insert loop retried the same slot forever. It now steps
    past occupied slots and only ever raises key_count.

Cost: one extra dictionary entry per key.

Tests

  • SimpleDict:add now returns false, "exists" for an existing key, like ngx.shared.DICT:add.
    The existing tests pass with that.
  • New TestKeyIndex tests:
    • add() on a fresh index doesn't scan the index (counts dict:get calls);
    • build_reverse(), including the incremental scan and skipping while another worker holds the lock;
    • concurrent insert of the same key;
    • list() dropping duplicates;
    • stale reverse entry;
    • key_count lower than occupied slots (this test hangs on the current code);
    • sync_deletes().

Tested under load on a 96-worker node with ~35k stored keys, by reloading nginx.

Keep a reverse key -> slot entry, so a fresh worker no longer syncs the whole
index on its first add() and timer tick. On reload all workers did that at once,
stalling requests for seconds with many keys (knyar#107).
… slots

E.g. after flush_all() while another worker was adding a key (knyar#162).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant