perf(queue): cut redis round-trips out of the worker hot path - #6
Merged
Merged
Conversation
Drives only the public API — `run()` / `work()` / `job.wait()` — against a real Redis, so every number includes superjson, the Lua round-trips, the worker wake loop and pub/sub delivery rather than a synthetic slice of them. Five scenarios: enqueue (serial and pipelined), steady-state drain at concurrency 1 and 50, a drain with steps, a drain across groups, and round-trip latency. Throughput on a shared box swings several-fold under load, so each scenario reports the best of three runs alongside its worst.
A CPU profile of a concurrency-50 drain showed the worker 69% idle: it was round-trip bound, not CPU bound. Every change here deletes a round-trip. `reserve` now claims a batch in one atomic call (bounded by RESERVE_BATCH_CAP, since Redis runs Lua single-threaded and a large `concurrency` would otherwise pause every client sharing the instance) and returns the delayed-job timer, the cron timer and the due-schedule list with it. That replaces four commands per wake — a schedule poll, a ms-to-next-schedule poll, one reserve per job, and a stalled scan — with one. Claim tokens are derived in-script from a single UUID, so a batch still gets globally unique tokens. A worker with every slot busy now parks on an in-process signal instead of BRPOP. It was paying a network hop to be told a slot it owns had freed, and that hop sat directly between one job finishing and the next being claimed. `reserve` also ships the job's step hash with the claim. The claim makes the worker the only writer of that hash, so the read is an exact snapshot and `step.do` resolves a memoized step with no round-trip at all — writes still persist before a step counts as done. `Queue.getStepData` goes with it. `complete` and `fail` publish the result record itself rather than a bare "1", so `wait()` no longer re-reads the key the notification just carried. A non-record payload still falls back to the stored record: during a rolling deploy a peer on the older build publishes "1", and parsing that as a result would hand the caller an empty string. The stalled-recovery scan gets a local throttle so a worker stops paying a round-trip per wake to be told a peer already holds the interval gate. Measured against the previous commit, adjacent runs on the same loaded machine: drain c=50 8986 -> 17427 ops/s, drain with 3 steps 5536 -> 8319, drain across 10 groups 8716 -> 14066, drain c=1 2543 -> 3303, round-trip latency 0.85 -> 0.46 ms. The enqueue path is untouched and scores identically, which is the control. Tests cover the invariants the batch and the local park could silently break: group and namespace caps under batch claiming, distinct tokens within a batch, close() racing an in-flight batch, a worker held off by a cap a dead peer owns, a saturated worker still firing cron, a retry replaying steps on a different worker, and four real OS processes draining a backlog exactly once.
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
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.
A CPU profile of a concurrency-50 drain showed the worker 69% idle — round-trip bound, not CPU bound. So this is not micro-optimisation; every change deletes a Redis round-trip from a hot path. Adds an e2e benchmark first so the numbers are measured rather than asserted.
Results
Adjacent runs on the same (loaded) machine, best of three:
The enqueue path is untouched, so it scores identically — that was the control. Any run where it moved was a run where the machine, not the code, had changed.
What changed
One
reserveper wake, claiming a batch. It used to take four commands to get round the loop: a schedule poll, a ms-to-next-schedule poll, onereserveper job claimed, and a stalled scan. Nowreserveclaims a batch atomically and returns both timers and the due-schedule list with it. The schedule polls were pure overhead for the common case of a workflow with no cron schedules at all.A saturated worker parks locally. It was doing a network round-trip to be told that a slot it owns had freed — and that hop sat directly between one job finishing and the next being claimed. It now waits on an in-process signal. When slots are free it still
BRPOPs, because then the interesting event genuinely is remote.Step memo ships with the claim. Every
step.dobegan with anHGETthat, on a first attempt, is a guaranteed miss. The claim makes the worker the sole writer of that hash, soHGETALLinside the reserve script is an exact snapshot and step reads now cost nothing. Writes still persist before a step counts as done.The
donepublish carries the result. It was a content-free doorbell: the waiter got woken, then had to go ask what happened.wait()goes from four blocking round-trips to two.Local throttle on the stalled scan, which Redis was rejecting via its gate anyway — the worker was paying a round-trip to find out.
Notes for review
RESERVE_BATCH_CAP = 64bounds the batch. Redis runs Lua single-threaded, so an unboundedconcurrencywould turn one wake into thousands of serialized commands and pause every client on the instance. Filling a larger concurrency just takes more passes.resolvePublished's non-record fallback is load-bearing, not defensive coding. During a rolling deploy a peer on the previous build publishes"1"; parsing that as a result would silently hand the caller an empty string.Queue.getStepDatais deleted. Not a breaking change —src/index.tsdoesn't exportQueue.keepFailed's retention score is millisecond-granular, so same-ms failures evict in uuid order rather than finish order. Always true; removing a round-trip fromwait()just made it reachable, and one existing test was relying on the latency. Test premise fixed, library behaviour unchanged.Tests
75 passing. New coverage targets what the batch and the local park could break silently: group and namespace caps under batch claiming, distinct claim tokens within one batch,
close()racing an in-flight batch, a worker held off by a cap a dead peer owns, a saturated worker still firing cron, a retry replaying steps on a different worker, the legacy publish shape, and four real OS processes draining a backlog exactly once.