Remove merge-tree's red-black tree

[Matt Rakow (ChumpChief)]

Description

Tracked by AB#80505.

merge-tree carried a hand-written red-black tree (collections/rbTree.ts, plus an augmented IntervalTree built on it). This PR removes both and migrates every consumer to something simpler, then deletes the files.

There were five consumers:

consumer	replacement
Client.clientNameToIds	Map
TestServer.upstreamMap	Map
EndpointIndex (sequence)	SortedSet
EndpointInRangeIndex / StartpointInRangeIndex (sequence)	SortedSet
OverlappingIntervalsIndex (sequence, via IntervalTree)	new SequenceIntervalOverlapSet — sorted array + segment tree

The first two were plain key/value lookups that never used the ordering at all. The three interval indexes now share a SequenceIntervalEndpointSet base built on merge-tree's existing SortedSet. The overlapping-intervals index needed a real replacement, since it depended on the tree's interval augmentation; that is SequenceIntervalOverlapSet, a sorted array with a segment tree of maximum end positions layered over it for pruning.

All of the removed types were @internal and none appeared in an API report, so there is no public API surface change here.

Net: −1,418 / +1,022 across 20 files. Bundle size should improve by a couple of percent on the affected entry points — the automated bundle-size check on this PR will report the exact numbers.

Bugs fixed

1. Intervals sharing an end position were collapsed (EndpointIndex). compareEnd alone is only a partial order, and RedBlackTree treats compare === 0 as the same key, so only one of several intervals ending at the same position was retained. Reachable through previousInterval / nextInterval. New SortedSet orders by (end, id), which is total. Changeset included.
2. gatherIterationResults with both a start and an end always returned []. The probe built by createTransientIntervalFromSequence carries a fresh uuid(), and the lookup compared with compare(), which includes the interval id — so it never matched anything. Reachable through the @legacy @beta iterator API. Changeset included.
3. OverlappingIntervalsIndex.remove of an interval that was never added threw TypeError. It called the tree's removeExisting, which skips the containment check remove performs. Not reachable today, since IntervalCollection only removes what it added, but the new implementation is a no-op as intended.

Bug 1 has some history: PR #6425 ("Make endIntervalTree preserve nodes with different start positions", Paul Leathers (@pleath), 2021) fixed it, but was closed unmerged with a note about rethinking it after PR #6407 merged. That PR merged shortly after and the fix was never revisited. It was never a regression — the collapsing behavior dates to the index's introduction in 2018; the later PR just made it reachable in more situations.

Performance

Everything below was measured, not assumed, against the interval counts this repo actually targets (the fuzz suite's maxIntervals and the sequence perf spec configs). Numbers are means over repeated passes on a warmed VM.

Endpoint indexes — full pass (build + floor/ceil probe of every item + remove all):

source	N	RedBlackTree	SortedSet	speedup
fuzz maxIntervals	100	104.9 µs	16.4 µs	6.4×
perf spec (×2 configs)	200	270.8 µs	38.8 µs	7.0×
perf spec (heaviest)	2,000	5,002.6 µs	1,060.6 µs	4.7×

The array only loses to the tree above ~40,000 intervals, roughly 20× the heaviest size the repo targets.

Range indexes — findIntervalsWith(End|Start)pointInRange, per query, k = results returned:

N	k	RedBlackTree	SortedSet	speedup
200	3	1.2 µs	0.6 µs	2.1×
200	200	6.5 µs	0.2 µs	38×
2,000	21	3.0 µs	0.2 µs	14×
2,000	2,000	74.8 µs	1.3 µs	59×
20,000	201	8.5 µs	0.2 µs	51×
20,000	20,000	765.6 µs	24.2 µs	32×

The old range indexes walked the tree node-by-node and allocated per step; the new ones binary-search once and slice.

Overlapping intervals index — build cost, adding all N to a fresh index:

config	N	IntervalTree	overlap set	speedup
perf spec 1	200	4.90 ms	0.20 ms	24×
perf spec 2	200	5.99 ms	0.17 ms	35×
perf spec 3	2,000	155.19 ms	2.45 ms	63×

Building 2,000 intervals in the tree issued 57,620 calls to union() — re-deriving the augmented min/max on every rotation, which is where ~85% of the build time went.

Query, 2,000 intervals, swept across the document, including five document-spanning intervals (the case that defeats a simple prefix-maximum optimization):

query position	IntervalTree	overlap set	speedup
1% of doc	13.4 µs	5.7 µs	2.4×
25% of doc	16.9 µs	7.4 µs	2.3×
50% of doc	18.1 µs	8.7 µs	2.1×
75% of doc	20.6 µs	9.2 µs	2.2×
100% of doc	14.2 µs	6.9 µs	2.1×

Interleaved mutation and query — the risk case, since the overlap set rebuilds its segment tree on the first query after any add or remove:

config	N	IntervalTree	overlap set	speedup
perf spec 1	200	54.7 µs	14.3 µs	3.8×
perf spec 2	200	88.3 µs	16.9 µs	5.2×
perf spec 3	2,000	79.6 µs	50.2 µs	1.6×

Still faster, because the tree's insert is expensive enough to outweigh the array's O(n) rebuild.

Dead code removed along the way

• OverlappingIntervalsIndex.map / mapUntil — @internal, no callers.
• beastTest.spec.ts's simpleTest, integerTest1, fileTest1, and LinearDictionary — unreachable exercises of the tree (231 lines).

Testing

New endpointIndex.spec.ts covers the endpoint index directly, including the same-end-position case that was broken. Existing merge-tree and sequence suites pass (1,568 and 1,694 respectively), and pnpm lint is clean in both packages with no API report drift.

[github-actions]

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

Based on the diff (2522 lines, 20 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

[github-actions]

🔭 PR Review Fleet Report

Note

This report is generated by an experimental AI review fleet and is provided as a beta feature. Findings are a starting point for discussion, not a gate. Use your own judgement.

Verdict: ⚠️ Approve with Suggestions

0 Spicy, 2 Pungent, 0 Smelly

Findings

Sev	#	Area	File	What	Fix
🧄 Pungent	H1	Performance	packages/dds/merge-tree/src/sortedSet.ts:28	The interval indexes backing IIntervalCollection (EndpointIndex, EndpointInRangeIndex, StartpointInRangeIndex, and the overlap index in sequenceIntervalOverlapSet.ts) were rewritten from RedBlackTree-based structures (O(log n) put/remove) to SortedSet/plain sorted arrays whose addOrUpdate/remove use Array.prototype.splice, an O(n) operation because it shifts all trailing elements. These indexes are updated on every interval add, remove, and endpoint slide: LocalIntervalCollection.add/removeIntervalFromIndexes (packages/dds/sequence/src/intervalCollection.ts lines ~182-250, 320-331) call index.add/index.remove on all attached indexes for every local or remote op that creates, deletes, or moves an interval endpoint (e.g. text edits sliding an interval boundary). In documents with thousands of intervals (comments, formatting ranges, etc.), each such op now costs O(n) instead of O(log n), which will visibly degrade typing/remote-op processing latency as interval count grows.	Keep an O(log n) insert/remove data structure (e.g. reintroduce a balanced tree, or use a skip list / indexed structure supporting sub-linear splice) for these interval indexes, or at minimum benchmark and document the acceptable interval-count ceiling; splice-based arrays should not back structures mutated on the per-op hot path for large collections.
🧄 Pungent	H2	Performance	packages/dds/sequence/src/intervalIndex/sequenceIntervalOverlapSet.ts:141	SequenceIntervalOverlapSet.add/remove mark maxEndsStale = true, and rebuildMaxEndsIfStale fully rebuilds the segment tree in O(n) on the very next findOverlapping call. OverlappingIntervalsIndex backs IIntervalCollection.findOverlappingIntervals, which application code commonly calls right after creating/moving an interval (e.g. to detect collisions) or during remote op processing when interval slide handlers query overlap state. When adds/removes interleave with overlap queries rather than arriving in a batch, every query after a mutation forces an O(n) full segment-tree rebuild instead of the O(log n) incremental augmented-node update the previous RedBlackTree-backed IntervalTree performed, degrading overlap queries from O(log n + k) to O(n) amortized per interleaved add/query cycle at scale (thousands of intervals).	Update the segment tree incrementally on individual add/remove (recompute only the O(log n) ancestor chain of the changed leaf) instead of invalidating and fully rebuilding the whole array-backed tree on the next query.

---

View workflow run

[Copilot]

Pull request overview

Removes merge-tree’s red-black tree and migrates interval indexing and key/value consumers to simpler structures.

Changes:

• Replaces lookup trees with Map and endpoint indexes with SortedSet.
• Adds an array-backed segment tree for overlap queries.
• Removes obsolete tree code and adds regression tests and changesets.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
.changeset/olive-cases-repeat.md	Documents endpoint-index fix.
.changeset/tidy-melons-grow.md	Documents bounded-iteration fix.
packages/dds/merge-tree/src/client.ts	Uses Map for client IDs.
packages/dds/merge-tree/src/collections/index.ts	Removes collection exports.
packages/dds/merge-tree/src/collections/rbTree.ts	Deletes red-black tree.
packages/dds/merge-tree/src/index.ts	Removes tree API exports.
packages/dds/merge-tree/src/test/beastTest.spec.ts	Removes obsolete tree exercises.
packages/dds/merge-tree/src/test/index.ts	Removes test tree exports.
packages/dds/merge-tree/src/test/testServer.ts	Uses Map for upstream mappings.
packages/dds/sequence/src/intervalIndex/endpointIndex.ts	Migrates endpoint lookup.
packages/dds/sequence/src/intervalIndex/endpointInRangeIndex.ts	Migrates end-range queries.
packages/dds/sequence/src/intervalIndex/intervalIndexUtils.ts	Removes comparison overrides.
packages/dds/sequence/src/intervalIndex/overlappingIntervalsIndex.ts	Uses the new overlap set.
packages/dds/sequence/src/intervalIndex/sequenceIntervalEndpointSet.ts	Adds shared endpoint sets.
packages/dds/sequence/src/intervalIndex/sequenceIntervalOverlapSet.ts	Adds segment-tree overlap indexing.
packages/dds/sequence/src/intervalIndex/startpointInRangeIndex.ts	Migrates start-range queries.
packages/dds/sequence/src/intervalTree.ts	Deletes interval tree wrapper.
packages/dds/sequence/src/test/collections.intervalTree.spec.ts	Removes obsolete tests.
packages/dds/sequence/src/test/endpointIndex.spec.ts	Adds endpoint regressions.
packages/dds/sequence/src/test/overlappingIntervalsIndex.spec.ts	Adds overlap and iteration tests.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Matt Rakow (ChumpChief)]

On H1 and H2 (array + splice is asymptotically worse than a balanced tree): that was the hypothesis this change was built to test, and it was measured rather than assumed, at the sizes this repo targets (the fuzz suite's maxIntervals and the sequence perf spec configs).

H1 — endpoint indexes, full pass (build + floor/ceil probe of every item + remove all): 6.4× faster at N=100, 7.0× at N=200, 4.7× at N=2,000. splice is a memmove; each tree insert allocates a node and pointer-chases through rotations. The array only loses above ~40,000 intervals, roughly 20× the heaviest size targeted here. SortedSet is also pre-existing merge-tree code that was already doing this job elsewhere.

H2 — interleaved mutation and query is called out as the risk in the description and was benchmarked directly: still 3.8× / 5.2× / 1.6× faster. The rebuild is O(n) with a tiny constant, while the tree re-derives its augmented min/max on every rotation — building 2,000 intervals issued 57,620 union() calls, ~85% of its build time. Build alone is 24–63× faster.

The incremental root-to-leaf max-end update H2 suggests is a reasonable follow-up if these indexes ever need to scale into the tens of thousands, but it isn't needed to avoid a regression — the new code is faster at every size this repo targets. Full tables are in the PR description.

[Copilot]

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.

Suppressed comments (3)

packages/dds/sequence/src/intervalIndex/sequenceIntervalOverlapSet.ts:152

• After the segment tree has been built, removing entries only marks it stale, so maxEnds keeps leaf references to every removed interval until a later non-empty overlap query rebuilds it. Since this index stays attached to each collection, deleting intervals without querying again retains disposed interval objects indefinitely. Clear the cached array on removal while keeping the stale flag so the next query still rebuilds it.

	public remove(interval: BaseSequenceInterval): void {
		const index = this.indexOf(interval);
		if (index !== undefined) {
			this.ordered.splice(index, 1);
			this.maxEndsStale = true;

packages/dds/sequence/src/test/overlappingIntervalsIndex.spec.ts:37

• This helper discards interval IDs, so the removal tests with identical endpoints pass even if the implementation removes the wrong interval and returns the removed instance. Include the ID in the rendered value so these assertions distinguish the expected interval from another interval at the same positions.

				`[${sharedString.localReferencePositionToPosition(
					interval.start,
				)}, ${sharedString.localReferencePositionToPosition(interval.end)}]`,

packages/dds/sequence/src/test/overlappingIntervalsIndex.spec.ts:214

• The randomized oracle only compares result counts, so equal numbers of false positives and false negatives still pass even though the test says the result agrees with the brute-force scan. Compare the returned interval IDs (order-independently) to validate actual membership.

				assert.equal(
					index.findOverlappingIntervals(start, end).length,
					expected.length,
					`mismatched overlap count for [${start}, ${end}]`,
				);

[Matt Rakow (ChumpChief)]

Second round:

M1 (fuzz coverage through removals): agreed, addressed in 3a0528a. The existing brute force comparison only ever grew the set, so the segment tree was always rebuilt over an array that had never shrunk. Added a round based fuzz which grows and then randomly shrinks the set, comparing full result sets (not just counts) against a brute force scan after every round, so the recursive index math is exercised against the lopsided shapes repeated removals produce. Verified the new coverage by mutation: changing the split point in gatherOverlapping fails the suite, reverting it passes.

H1 is the same performance finding as the previous round — already answered above with measurements. Short version: measured 4.7–7× faster at the sizes this repo targets, crossover around 40,000 intervals.

Also worth noting the bundle size check landed and confirms the secondary benefit: matrix.js −1,483 gzip, sharedString.js −969, aqueduct.js −1,040, fluidFrameworkAllAlpha.js −1,051.

[Copilot]

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.

Suppressed comments (1)

packages/dds/sequence/src/test/overlappingIntervalsIndex.spec.ts:38

• This formatter drops interval identity, so the removal test with two [10, 20] intervals still passes if remove deletes the retained object and leaves the removed one. Include the interval ID in the rendered value so the test actually verifies per-interval removal semantics.

			(interval) =>
				`[${sharedString.localReferencePositionToPosition(
					interval.start,
				)}, ${sharedString.localReferencePositionToPosition(interval.end)}]`,

[github-actions]

Bundle size comparison

Base commit: cc08e183b8d5c6c722d6983e90b8f0373067e5d0
Head commit: 91f9dbb834e77a736298f3eb3bef15aba6174f3a

Notable changes

• 🟢 fluidFrameworkAllAlpha.js: parsed 784509 → 780458 (-4051), gzip 215049 → 214007 (-1042)
• 🟢 aqueduct.js: parsed 531223 → 527167 (-4056), gzip 142114 → 141068 (-1046)
• 🟢 sharedString.js: parsed 176510 → 172421 (-4089), gzip 49798 → 48840 (-958)
• 🟢 matrix.js: parsed 160341 → 154471 (-5870), gzip 45798 → 44317 (-1481)

Per-bundle deltas

@fluid-example/bundle-size-tests

• 🟢 fluidFrameworkAllAlpha.js: parsed 784509 → 780458 (-4051), gzip 215049 → 214007 (-1042)
• azureClient.js: parsed 624847 → 624903 (+56), gzip 166640 → 166685 (+45)
• odspClient.js: parsed 597135 → 597191 (+56), gzip 159785 → 159827 (+42)
• 🟢 aqueduct.js: parsed 531223 → 527167 (-4056), gzip 142114 → 141068 (-1046)
• fluidFramework.js: parsed 403809 → 403830 (+21), gzip 114489 → 114506 (+17)
• sharedTree.js: parsed 393213 → 393227 (+14), gzip 111931 → 111940 (+9)
• containerRuntime.js: parsed 309144 → 309158 (+14), gzip 84569 → 84576 (+7)
• 🟢 sharedString.js: parsed 176510 → 172421 (-4089), gzip 49798 → 48840 (-958)
• experimentalSharedTree.js: parsed 160665 → 160665 (0), gzip 46265 → 46265 (0)
• 🟢 matrix.js: parsed 160341 → 154471 (-5870), gzip 45798 → 44317 (-1481)
• loader.js: parsed 145704 → 145718 (+14), gzip 39286 → 39302 (+16)
• odspDriver.js: parsed 103906 → 103927 (+21), gzip 32404 → 32411 (+7)
• directory.js: parsed 67110 → 67117 (+7), gzip 18859 → 18866 (+7)
• 578.js: parsed 58686 → 58686 (0), gzip 17657 → 17657 (0)
• map.js: parsed 47205 → 47212 (+7), gzip 14455 → 14462 (+7)
• odspPrefetchSnapshot.js: parsed 45635 → 45649 (+14), gzip 15242 → 15250 (+8)
• 252.js: parsed 44362 → 44362 (0), gzip 13735 → 13735 (0)
• summarizerDelayLoadedModule.js: parsed 30717 → 30717 (0), gzip 7716 → 7716 (0)
• socketModule.js: parsed 26469 → 26476 (+7), gzip 7896 → 7903 (+7)
• createNewModule.js: parsed 12454 → 12454 (0), gzip 4797 → 4797 (0)
• summaryModule.js: parsed 3789 → 3789 (0), gzip 1857 → 1857 (0)
• connectionState.js: parsed 909 → 909 (0), gzip 500 → 500 (0)
• sharedTreeAttributes.js: parsed 847 → 854 (+7), gzip 499 → 508 (+9)
• debugAssert.js: parsed 429 → 429 (0), gzip 299 → 299 (0)
• FluidFramework-HashFallback.js: parsed 419 → 419 (0), gzip 313 → 313 (0)