Repository navigation
fix(monitoring): make Counter and Meter increments O(1) amortized - #53
Merged
Merged
Conversation
Counter kept one list entry per increment and scanned the whole list on every call. It now keeps entries in a priority queue ordered by expiry and only dequeues the expired head. Meter rebuilt an array of all bucket keys on every call; it now prunes at most once per bucket interval, without LINQ or per-call allocations. Measured in Release (net9.0): Counter with 200k live entries went from 91 us to 0.13 us per increment, a full 24 h Meter from 164 us to 0.26 us, and 8 threads on a full Meter from ~6k to ~5.8M increments per second.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Counter.IncrementandMeter.Incrementhad a cost that grew with the amount of retained data, so a busy counter or a longMeterwindow made every increment slower.Counterkept oneListentry per increment and ranRemoveAllover the whole list on every call (O(n) per increment, quadratic overall). Entries now live in aPriorityQueueordered by expiry, and only the expired head is dequeued. A priority queue is used instead of a plain queue becauseIncrement(by, duration)accepts a different duration per call, so insertion order is not expiry order.Meterrebuilt an array of all bucket keys with LINQ on every call (about 193 B allocated per increment, with up to 86,400 buckets for the default 24 h window). It now prunes at most once per bucket interval, without LINQ or per-call allocations.Public API, TTL/window semantics and thread safety are unchanged.
Counter.Currentand the state export still return the same values.Measurements
Release, net9.0, loopback, same probe against
mainand this branch:Counter, 200k live entriesMeter, full 24 h windowMeter, full 24 h window, 8 threadsTests
New
CounterMeterTestsinSisk.Monitoring.Tests: sum and expiry correctness, expiry out of insertion order,Meterwindow expiry, and a cost test (100k increments must finish within 1 s). With the old implementation the cost test fails (52 s); with this changeSisk.Monitoring.Testspasses 8/8.Notes
Meterwindow) because the types readDateTime.Nowdirectly and there is no injectable clock.