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 4fd6d36 on 2026-05-27.

Readiness: 8/10 — ALMOST READY

Small, well-scoped perf change: ConsensusQueue.remove() goes from O(n) to O(1) by backing SnapshotableArray with DoublyLinkedList. Correctness, history, and solution-space all converge that the change is safe and the snapshot wire format is preserved. Three polish items remain on the two open inline threads — none blocking, but resolving them moves the score to 9–10.

Path to Ready

• Resolve inline threads

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). This is the first non-trivial change to the surface since introduction.
• vladsud authored ConsensusQueue: acquire/release semantics, handle support #1395 and is the only living source of design-intent context for the original extends Array choice, the snapshot wire format, and ConsensusQueue acquire/release semantics — worth tagging as reviewer.
• The only load-bearing invariant in scope is snapshot wire-format compatibility. Focus attention on DoublyLinkedList iteration order matching insertion order in asArray(), and on loadFrom preserving order via the for…of push loop.
• 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 (reuse of in-tree primitive, no compaction tuning, no slot-nulling for GC) at the cost of per-node allocations. Worth a taste call from someone who'd rather not pull DoublyLinkedList into this DDS.
• Cannot be assessed by the pipeline — Whether the 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 (1 prior review)

• 2635b93 2026-05-27 · 8/10 — initial review, two polish threads filed