perf(ordered-collection): back SnapshotableQueue with DoublyLinkedList

[anthony-murphy]

Description

Replaces the array backing SnapshotableArray (the base class for SnapshotableQueue) with DoublyLinkedList<T>. SnapshotableArray no longer extends Array — it is now a plain class.

• add(value) → list.push(value).
• remove() in ConsensusQueue → list.shift()!.data (O(1) instead of Array.shift()'s O(n)).
• asArray() materializes from the list (preserves snapshot wire format byte-for-byte).
• size delegates to list.length.
• loadFrom pushes each value in order (preserves ordering).

Why

SnapshotableQueue.remove() is on the hot path for ConsensusOrderedCollection.acquire. Queue depth grows with backlog of unacquired items; in a job-queue scenario every acquire paid O(n) for the array shift. The DoublyLinkedList makes it O(1) without changing the snapshot format.

Notes

Snapshot wire format is byte-identical (still a raw T[] on disk via asArray()). SnapshotableArray is not exported, and no in-repo callers used Array prototype methods on it. Pure perf prep — no reSubmitSquashed, no squash logic, no tests touched, no api-report changes.

[github-actions]

Hi! Thank you for opening this PR. Want me to review it?

Based on the diff (61 lines, 3 files), I've queued these reviewers:

• Correctness — logic errors, race conditions, lifecycle issues
• Security — vulnerabilities, secret exposure, injection
• API Compatibility — breaking changes, release tags, type design
• Performance — algorithmic regressions, memory leaks
• Testing — coverage gaps, hollow tests

How this works

• Adjust the reviewer set by ticking/unticking boxes above. Reviewer toggles alone don't trigger anything.

• Tick Start review below to dispatch the review fleet.

• After review finishes, tick Start review again to request another run — it auto-resets after each dispatch.

• This comment updates as new commits land; your reviewer selections are preserved.

• Start review

[anthony-murphy]

Deep Review: The class name SnapshotableArray was chosen in #1395 (vladsud, 2020-03-05) specifically because it was an Array subclass. This PR removes the extends Array — the PR description acknowledges it ("SnapshotableArray no longer extends Array — it is now a plain class") but the name stays. Both blind solution-space proposers independently expected the class to still be array-backed, so a future reader is likely to hit the same surprise.

The symbol is internal to the package (not exported via api-report; only consumer is SnapshotableQueue in consensusQueue.ts), so renaming in-PR is cheap — e.g. SnapshotableCollection or SnapshotableList. If the rename is deferred, a one-line doc comment noting the legacy name predates the linked-list backing would save the next reader from re-deriving "why is this called Array?".

[anthony-murphy]

Deep Review: asArray() changes from return this.data (alias) to materializing a fresh T[] by iterating the list; loadFrom changes from this.data = from (alias) to per-element push. Both surfaces newly depend on DoublyLinkedList iteration order matching insertion order, and that's the sole load-bearing invariant in this area — the snapshot wire format. #1395 has no inline review history on these methods and no targeted unit test locks the contract.

There is strong transitive coverage: DoublyLinkedList iteration order is unit-tested (list.spec.ts:50-57, :71-80) and the ordered-collection fuzz model compares asArray() across clients (fuzzUtils.ts:150-159, :182-196). That lowers — but does not erase — the value of a local lock.

Suggest one ~10-line unit test in this PR: loadFrom([a,b,c]) → remove() → assert asArray() returns [b,c] and size() === 2. Locks the new iteration-based snapshot contract against future DoublyLinkedList or refactor regressions, and directly substantiates the "byte-identical wire format" claim. Alternatively, if the existing fuzz consistency check (fuzzUtils.ts:150-196) already exercises a loadFrom → partial remove() → asArray() cycle after a snapshot/load, calling that out in the PR description is sufficient.

[anthony-murphy]

Deep Review

Reviewed commit 7c6cbc6 on 2026-06-02.

Readiness: 7/10 — ALMOST READY

Small, well-scoped perf change backing SnapshotableArray with DoublyLinkedList<T> to make ConsensusQueue.remove() strict O(1). Correctness, history, and exports all check out — no blocking issues. Four polish items remain: a design-tradeoff conversation (linked-list at base vs. head-pointer scoped to the queue), the snapshot round-trip test gap on the open inline thread, a stale child-class doc, and a PR-description/diff drift.

Path to Ready

• Resolve inline threads
• Update the PR description — replace "no tests touched" with a note that snapshotableArray.spec.ts is added (covering loadFrom → remove → asArray/size), and optionally mention the head-pointer + amortized-compaction alternative that was considered.

Context for Reviewers

• snapshotableArray.ts has been frozen for ~5 years (0 commits in last 6 months; last substantive change was ConsensusQueue: acquire/release semantics, handle support #1395, vladsud, 2020-03-05). Snapshot wire format is the load-bearing invariant — focus on asArray() materialization order and loadFrom push order.
• vladsud authored ConsensusQueue: acquire/release semantics, handle support #1395 and is the only living source of design-intent for the original extends Array choice, the snapshot wire format, and ConsensusQueue acquire/release semantics — worth tagging as reviewer.
• fuzzUtils.ts:155-159 reflectively reads (a as any).data and calls asArray() on it. The cast was always a lie; after this PR it still works incidentally because asArray() exists on SnapshotableArray itself. Future refactors of the data field must update that site.
• DoublyLinkedList import lands on the already-allowed @fluidframework/core-utils/internal entrypoint — no package.json change.

For human reviewer

• Needs human judgment — Both blind solution-space proposals independently chose head-index + amortized compaction over DoublyLinkedList. The PR's choice is defensible (strict O(1) worst-case shift, reuse of in-tree primitive) at the cost of per-add node allocation. Worth a taste call from someone who'd rather not pull DoublyLinkedList into this DDS — see the inline thread on snapshotableArray.ts.
• Cannot be assessed by the pipeline — Whether per-node allocation overhead is acceptable in the target workload (high queue depth, frequent acquire) is a runtime/perf judgment that needs measurement, not static review.
• Sign-off — vladsud recommended given his ownership of the original design.

Review history (2 prior reviews)

• 4fd6d36 2026-05-27 · 8/10 — three polish threads, none blocking
• 2635b93 2026-05-27 · 8/10 — initial review, two polish threads filed

[anthony-murphy]

Deep Review: Both blind solution-space proposers independently chose a head-pointer + amortized compaction scoped to SnapshotableQueue and explicitly rejected the linked-list direction, citing per-add node allocation, GC pressure, and base-class blast radius. The PR mutates the base class (protected data: DoublyLinkedList<T>) and asArray() now materializes T[] per call instead of returning a reference.

The present-tense blast radius is narrow — SnapshotableArray isn't exported, only SnapshotableQueue (and a test mirror) extends it, and consensusOrderedCollection.ts:208-216 immediately serializes the asArray() result so the end-to-end snapshot cost was already O(n). That's why this is a polish point, not a blocker.

Two ways to land cleanly:

• (a) Document the deliberate tradeoff in the new doc comment on SnapshotableArray — strict O(1) worst-case remove and reuse of an in-tree primitive vs. per-add allocation and lost asArray() reference return — so the next reader sees why the head-pointer alternative was rejected. No code change required.
• (b) If you'd rather keep base-class storage as T[], push the DoublyLinkedList field plus add/remove/asArray/size overrides down into SnapshotableQueue so any future sibling subclass doesn't inherit a primitive it doesn't need.

Either is fine; (a) is the cheaper path if there's no near-term plan for a second subclass.