Skip to content

fix(shared): make Queue.pull() O(1) to avoid quadratic backlog drains - #103

Merged
dinwwwh merged 4 commits into
mainfrom
claude/elegant-cray-9e780o
Sep 26, 2026
Merged

dinwwwh merged 4 commits into
mainfrom
claude/elegant-cray-9e780o

Conversation

@dinwwwh

@dinwwwh dinwwwh commented Sep 25, 2026

Copy link
Copy Markdown
Member

Summary

Optimized the Queue implementation to reduce memory overhead when pulling items by using a head pointer instead of repeatedly shifting the array. This prevents memory leaks from held references and amortizes the cost of array compaction.

Key Changes

  • Replaced Array.shift() with a head pointer approach to avoid O(n) operations on every pull
  • Items are marked as undefined after being pulled to allow garbage collection
  • Implemented lazy compaction that triggers when at least half the array has been consumed (and array size exceeds 1024 items), amortizing the O(n) splice operation to O(1) per pull
  • Updated abort() to reset the head pointer when clearing the queue
  • Added comprehensive test case verifying correct ordering across internal compaction, including handling of undefined values

Implementation Details

  • Added COMPACT_THRESHOLD constant (1024) to control when compaction becomes eligible
  • Compaction only occurs when head >= COMPACT_THRESHOLD && head * 2 >= items.length, ensuring the amortized cost remains acceptable
  • Special case: if all items have been pulled (head === items.length), simply reset array length instead of splicing
  • Type signature updated to (T | undefined)[] to reflect that pulled items are set to undefined

https://claude.ai/code/session_01LDugk98gQoDyPK2NCVvDuD

Queue.pull() used Array#shift(), which is O(n) once V8 stops using its
fast path for large arrays (~16k+ elements). Draining a large buffered
backlog, e.g. a peer streaming many octet-stream chunks before the
handler reads the body, was therefore quadratic and blocked the event
loop (200k items: ~3.3s before, ~21ms after).

Track a head index instead, clear pulled slots so items can be GC'd,
reset the array once fully drained, and compact with splice once at
least half of a large array is consumed (amortized O(1) per pull).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LDugk98gQoDyPK2NCVvDuD
A single fill-then-drain of 3000 items already hits both compactions
and the fully-drained reset. Drop the abort-after-partial-drain test,
which cannot fail differently from the existing abort test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LDugk98gQoDyPK2NCVvDuD
Resetting the buffer with `length = 0` on every full drain made V8 drop
and reallocate the backing store on each pull when the consumer keeps
up (the common 0 -> 1 -> 0 case). Fold the reset into the thresholded
compaction instead, so it runs at most once per 1024 pulls: alternating
push/pull goes from ~161ms to ~85ms per 1M ops, with backlog drains
unchanged.

Also name the threshold constant, trim the field comment, and make the
compaction test cover both the splice and the full-drain reset.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LDugk98gQoDyPK2NCVvDuD
The queue decides whether an item is buffered by index, not by value,
so undefined items must survive both the splice and the full-drain
reset. Mix undefined into the compaction test; the trailing push stays
a defined value so a stale cleared slot cannot pass as a real item.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LDugk98gQoDyPK2NCVvDuD
@pkg-pr-new

pkg-pr-new Bot commented Sep 25, 2026

Copy link
Copy Markdown
@standard-server/aws-lambda

npm i https://pkg.pr.new/@standard-server/aws-lambda@103

@standard-server/core

npm i https://pkg.pr.new/@standard-server/core@103

@standard-server/fastify

npm i https://pkg.pr.new/@standard-server/fastify@103

@standard-server/fetch

npm i https://pkg.pr.new/@standard-server/fetch@103

@standard-server/node

npm i https://pkg.pr.new/@standard-server/node@103

@standard-server/peer

npm i https://pkg.pr.new/@standard-server/peer@103

@standard-server/shared

npm i https://pkg.pr.new/@standard-server/shared@103

commit: 011d3b4

@codspeed

codspeed Bot commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 26 untouched benchmarks
⏩ 108 skipped benchmarks1


Comparing claude/elegant-cray-9e780o (011d3b4) with main (4488969)

Open in CodSpeed

Footnotes

  1. 108 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@codecov

codecov Bot commented Sep 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Head-pointer buffer in Queue — packages/shared/src/queue.ts drops Array.shift() in pull() for a head index over (T | undefined)[]; the pulled slot is overwritten with undefined to release the reference.
  • Lazy compaction — once head >= 1024 && head * 2 >= items.length, a fully drained buffer is reset via items.length = 0, otherwise items.splice(0, head), and head returns to 0.
  • abort() resets head — keeps the pointer consistent with the cleared buffer.
  • Compaction-ordering test — packages/shared/src/queue.test.ts pushes 2048 items (every third undefined) and pulls them all, hitting both the partial splice (pull #1024) and full-reset (pull #2048) branches.

I traced the 0 <= head <= items.length invariant through push/pull/close/abort and it holds; a pending pull can only exist when head === items.length, so compaction never races one. Private representation change only — no public API impact. Tests, tsc -b, and eslint all pass.

ℹ️ PR description overstates the memory benefit

The description says the change "prevents memory leaks from held references", but Array.prototype.shift() already removed and released each pulled element — there was no reference leak. The measurable win is CPU: pull() becomes amortized O(1) instead of O(n), removing the quadratic cost of draining a large backlog. Worth stating the rationale accurately since the old path is now in history.

Technical details
# PR rationale attributes a reference leak that didn't exist

## Affected sites
- PR body / commit `99d7286` ("fix(shared): make Queue.pull() O(1) ...") — describes the prior `shift()` path as leaking held references.
- `packages/shared/src/queue.ts:37-53` — the new head-pointer pull + compaction path.

## Required outcome
- Rationale should attribute the win to CPU (amortized O(1) `pull()`, no quadratic backlog drain), not to a reference leak.
- Optionally note the memory tradeoff: consumed slots are retained as `undefined` until compaction, so peak array length is bounded by roughly `threshold + backlog` and can be marginally higher than the old path for tiny queues.

## Open questions for the human (optional)
- Was "memory leak" meant to describe V8 backing-store capacity retention rather than element references? If so, the description could say so precisely.

Pullfrog  | View workflow run | Using DeepSeek Flash (default — pick a model for stronger reviews) | 𝕏

@dinwwwh dinwwwh changed the title Optimize Queue memory usage with lazy compaction fix(shared): make Queue.pull() O(1) to avoid quadratic backlog drains Sep 25, 2026
@dinwwwh
dinwwwh merged commit 901bb47 into main Sep 26, 2026
11 checks passed
dinwwwh added a commit to middleapi/orpc that referenced this pull request Sep 27, 2026
…ts (#2104)

Resuming an iterator subscription with `lastEventId` silently dropped
the oldest missed events whenever more than `maxBufferedEvents` (default
100) were missed. Adapters replay the whole backlog in one burst before
the consumer can read, so the buffer overflowed even for a fast
consumer, and the client's `lastEventId` then skipped past the gap for
good. Resumed subscribers now receive every missed event, while slow
consumers still drop the oldest ones.

### Fixes

- Memory, Redis, Upstash and Bun publishers deliver the full resume
backlog to `subscribe(event, { lastEventId })` iterators. Before,
resuming 150 missed events with the default limit lost the first 50.
- A consumer that falls more than `maxBufferedEvents` further behind
after resuming still drops the oldest events, so memory stays bounded by
the backlog plus the limit.
- Subscriptions without `lastEventId` behave exactly as before.

### Performance

- Draining a large buffer no longer copies the array on every read: a
50K-event resume drains in about 57 ms instead of about 2 s.

### Notes for reviewers

- Adapters must deliver the resume backlog before `subscribeListener`
resolves. This is now documented on the method, and the Memory and
BaseRedis adapters already do it. Cloudflare's Durable Object replays
over the WebSocket after subscribing, so its resumed events still count
toward the limit; left for a follow-up.
- The private `Queue` mirrors the O(1) `Queue.pull` fix in
middleapi/standard-server#103, which is unreleased. It can be replaced
once standard-server ships a queue that exposes its size and can drop
the oldest item.

### Testing

- New regression tests in the base publisher, memory and Redis suites
fail on `main` and pass here.
- Publisher and Bun Redis suites pass against a local Redis 8.0 (Upstash
skipped without credentials).
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.

2 participants