Skip to content

Use the existing RingBuffer for keeper executions - #817

Open
shaurya703 wants to merge 1 commit into
SO4-Markets:mainfrom
shaurya703:perf/keeper-executions-ringbuffer
Open

Use the existing RingBuffer for keeper executions#817
shaurya703 wants to merge 1 commit into
SO4-Markets:mainfrom
shaurya703:perf/keeper-executions-ringbuffer

Conversation

@shaurya703

Copy link
Copy Markdown

Fixes #808.

last_executions is now the RingBuffer already used by state.failures, so the two bounded-history lists in this file use one implementation instead of two — and eviction is pop_front rather than Vec::remove(0) shifting every remaining element.

Cap stays 100, the admin endpoint still returns the most recent 50. Behaviour unchanged.

Three things the swap needed, none of them obvious from the issue

  • Default is now hand-written. RingBuffer::default() is FAILURE_RING_CAPACITY — the failures list's 256. Keeping the derived Default would have silently grown this list from 100 to 256 with nothing to notice. There is a test that fails if it ever goes back.
  • RingBuffer gained a Serialize impl, because KeeperStatus derives Serialize and this field is part of it. Oldest-first, matching the Vec, so anything serializing a struct that holds one sees the same array as before. (KeeperStatus is not serialized anywhere today — KeeperStatusResponse is, and builds its own Vec — but the derive is there and I did not want to remove it as a side effect of a performance change.)
  • The admin handler iterates and clones instead of consuming: RingBuffer exposes iter(), not into_iter().

Tests

Two, both in state.rs:

  • The eviction bound itself — push capacity + 25, assert exactly 100 remain, oldest evicted, newest retained, order preserved. Nothing covered this before, so a capacity change would have gone unnoticed.
  • A guard on the Default trap above.

Verified non-vacuous: switching Default back to RingBuffer::default() fails both (292 passed; 2 failed).

Checks

cargo test — 16 suites green, 294 lib tests. cargo fmt --check clean. cargo clippy --all-targets -- -D warnings clean.

Fixes SO4-Markets#808.

last_executions was a Vec trimmed with remove(0), which shifts every
remaining element on each eviction, while the equivalent bounded-history
list next to it (state.failures) already uses RingBuffer and its
VecDeque-backed pop_front. Two implementations of the same idea in one
file, one of them O(n).

The cap stays 100 and the admin endpoint still returns the most recent 50,
so behaviour is unchanged.

Three things the swap needed:

- Default is now hand-written. RingBuffer::default() is
  FAILURE_RING_CAPACITY, which is the failures list's 256 — a derived
  Default would have silently grown this list to 256.
- RingBuffer gained a Serialize impl, because KeeperStatus derives
  Serialize and the field is part of it. Oldest first, so anything
  serializing a struct holding one sees the array it saw before.
- The admin handler iterates and clones instead of consuming, since
  RingBuffer exposes iter() rather than into_iter().

Two tests: the eviction bound itself, which nothing covered before, and one
that fails if Default ever goes back to the 256-capacity derive.

294 lib tests pass, 16 suites green, fmt and clippy -D warnings clean.
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.

KeeperStatus.last_executions uses Vec::remove(0) (O(n)) instead of the RingBuffer already used for the equivalent failures list

1 participant