From fe3e4ae8524f48bc82e85dd8e6744cb3a1f6bac7 Mon Sep 17 00:00:00 2001 From: Matt Rakow Date: Thu, 13 Aug 2026 19:29:15 -0700 Subject: [PATCH 1/3] refactor(merge-tree): replace RedBlackTree with Map for key/value lookups Client.clientNameToIds and TestServer.upstreamMap used RedBlackTree purely as a key/value dictionary - neither called any ordered operation (floor, ceil, min, max, walk, mapRange). A plain Map provides the same behavior with O(1) lookup instead of O(log n) pointer chasing. Note that getOrAddShortClientId previously tested the returned node for truthiness, which was only correct because a node object is always truthy. The Map equivalent uses has() so that short client id 0 is handled correctly. This is a step toward removing the hand-rolled red-black tree implementation entirely. All types involved are internal-only, so there is no API surface change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- packages/dds/merge-tree/src/client.ts | 14 ++++++-------- packages/dds/merge-tree/src/test/testServer.ts | 9 +++------ 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/packages/dds/merge-tree/src/client.ts b/packages/dds/merge-tree/src/client.ts index fef69dca8dab..0fba40f24dab 100644 --- a/packages/dds/merge-tree/src/client.ts +++ b/packages/dds/merge-tree/src/client.ts @@ -31,7 +31,6 @@ import { } from "@fluidframework/telemetry-utils/internal"; import { MergeTreeTextHelper, type IMergeTreeTextHelper } from "./MergeTreeTextHelper.js"; -import { RedBlackTree } from "./collections/index.js"; import { NonCollabClient, SquashClient, UniversalSequenceNumber } from "./constants.js"; import { type LocalReferencePosition, SlidingPreference } from "./localReference.js"; import { @@ -54,7 +53,6 @@ import { type ISegmentPrivate, type Marker, type SegmentGroup, - compareStrings, isSegmentLeaf, type ISegmentInternal, type ISegmentLeaf, @@ -173,7 +171,7 @@ export class Client extends TypedEventEmitter { private readonly _mergeTree: MergeTree; - private readonly clientNameToIds = new RedBlackTree(compareStrings); + private readonly clientNameToIds = new Map(); private readonly shortClientIdMap: string[] = []; /** @@ -829,14 +827,14 @@ export class Client extends TypedEventEmitter { } getOrAddShortClientId(longClientId: string): number { - if (!this.clientNameToIds.get(longClientId)) { + if (!this.clientNameToIds.has(longClientId)) { this.addLongClientId(longClientId); } return this.getShortClientId(longClientId); } protected getShortClientId(longClientId: string): number { - return this.clientNameToIds.get(longClientId)!.data; + return this.clientNameToIds.get(longClientId)!; } getLongClientId(shortClientId: number): string { @@ -844,7 +842,7 @@ export class Client extends TypedEventEmitter { } addLongClientId(longClientId: string): void { - this.clientNameToIds.put(longClientId, this.shortClientIdMap.length); + this.clientNameToIds.set(longClientId, this.shortClientIdMap.length); this.shortClientIdMap.push(longClientId); } @@ -1718,9 +1716,9 @@ export class Client extends TypedEventEmitter { ); } else { const oldClientId = this.longClientId; - const oldData = this.clientNameToIds.get(oldClientId)!.data; + const oldData = this.clientNameToIds.get(oldClientId)!; this.longClientId = longClientId; - this.clientNameToIds.put(longClientId, oldData); + this.clientNameToIds.set(longClientId, oldData); this.shortClientIdMap[oldData] = longClientId; } } diff --git a/packages/dds/merge-tree/src/test/testServer.ts b/packages/dds/merge-tree/src/test/testServer.ts index 02f2f54f8cdd..d73c40991612 100644 --- a/packages/dds/merge-tree/src/test/testServer.ts +++ b/packages/dds/merge-tree/src/test/testServer.ts @@ -7,8 +7,6 @@ import { Heap, type IComparer } from "@fluidframework/core-utils/internal"; import type { ISequencedDocumentMessage } from "@fluidframework/driver-definitions/internal"; import { MergeTreeTextHelper } from "../MergeTreeTextHelper.js"; -import { RedBlackTree } from "../collections/index.js"; -import { compareNumbers } from "../mergeTreeNodes.js"; import { PriorPerspective } from "../perspective.js"; import type { PropertySet } from "../properties.js"; @@ -32,7 +30,7 @@ export class TestServer extends TestClient { seq = 1; clients: TestClient[] = []; clientSeqNumbers: Heap = new Heap(clientSeqComparer); - upstreamMap: RedBlackTree = new RedBlackTree(compareNumbers); + upstreamMap: Map = new Map(); constructor(options?: PropertySet) { super(options); } @@ -63,15 +61,14 @@ export class TestServer extends TestClient { // in upstream message transformUpstreamMessage(msg: ISequencedDocumentMessage): void { if (msg.referenceSequenceNumber > 0) { - msg.referenceSequenceNumber = - this.upstreamMap.get(msg.referenceSequenceNumber)?.data ?? 0; + msg.referenceSequenceNumber = this.upstreamMap.get(msg.referenceSequenceNumber) ?? 0; } msg.origin = { id: "A", sequenceNumber: msg.sequenceNumber, minimumSequenceNumber: msg.minimumSequenceNumber, }; - this.upstreamMap.put(msg.sequenceNumber, this.seq); + this.upstreamMap.set(msg.sequenceNumber, this.seq); msg.sequenceNumber = -1; } From 8f6976ec14b734fa9b7ea4da0cf3aa8e5287e177 Mon Sep 17 00:00:00 2001 From: Matt Rakow Date: Thu, 13 Aug 2026 22:19:42 -0700 Subject: [PATCH 2/3] test(merge-tree): remove the unreachable red-black tree exercises `simpleTest`, `integerTest1` and `fileTest1` are exported from beastTest.spec.ts but nothing calls them: the suite only runs `firstTest`, `randolicious`, `mergeTreeCheckedTest`, `clientServer` and `findReplacePerf`, none of which touch a RedBlackTree. Removing them also retires `LinearDictionary`, a hand-written array-backed SortedDictionary which existed solely as the oracle `fileTest1` compared the tree against, along with the two property printers and `took`. No test which runs today loses any coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../dds/merge-tree/src/test/beastTest.spec.ts | 231 ------------------ 1 file changed, 231 deletions(-) diff --git a/packages/dds/merge-tree/src/test/beastTest.spec.ts b/packages/dds/merge-tree/src/test/beastTest.spec.ts index b08e6547e0c6..ab973f5085b0 100644 --- a/packages/dds/merge-tree/src/test/beastTest.spec.ts +++ b/packages/dds/merge-tree/src/test/beastTest.spec.ts @@ -9,7 +9,6 @@ /* eslint-disable @typescript-eslint/no-base-to-string */ import { strict as assert } from "node:assert"; -import fs from "node:fs"; import path from "node:path"; import { Trace } from "@fluid-internal/client-utils"; @@ -19,13 +18,6 @@ import { createChildLogger } from "@fluidframework/telemetry-utils/internal"; import JsDiff from "diff"; import { MergeTreeTextHelper } from "../MergeTreeTextHelper.js"; -import { - type KeyComparer, - type Property, - type PropertyAction, - RedBlackTree, - type SortedDictionary, -} from "../collections/index.js"; import { LocalClientId, UnassignedSequenceNumber, @@ -35,8 +27,6 @@ import { MergeTree } from "../mergeTree.js"; import type { IMergeTreeDeltaOpArgs } from "../mergeTreeDeltaCallback.js"; import { type IJSONMarkerSegment, - compareNumbers, - compareStrings, reservedMarkerIdKey, type ISegmentPrivate, } from "../mergeTreeNodes.js"; @@ -53,102 +43,6 @@ import { TestClient, getStats, specToSegment } from "./testClient.js"; import { TestServer } from "./testServer.js"; import { loadTextFromFile, nodeOrdinalsHaveIntegrity } from "./testUtils.js"; -function LinearDictionary( - compareKeys: KeyComparer, -): SortedDictionary { - const props: Property[] = []; - const compareProps = (a: Property, b: Property): number => - compareKeys(a.key, b.key); - function mapRange( - action: PropertyAction, - accum?: TAccum, - start?: TKey, - end?: TKey, - ): void { - let _start = start; - let _end = end; - - if (props.length > 0) { - return; - } - - // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing -- using ??= could change behavior if value is falsy - if (_start === undefined) { - _start = min()!.key; - } - // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing -- using ??= could change behavior if value is falsy - if (_end === undefined) { - _end = max()!.key; - } - for (let i = 0, len = props.length; i < len; i++) { - if (compareKeys(_start, props[i].key) <= 0) { - const ecmp = compareKeys(_end, props[i].key); - if (ecmp < 0) { - break; - } - if (!action(props[i], accum)) { - break; - } - } - } - } - - function map(action: PropertyAction, accum?: TAccum): void { - mapRange(action, accum); - } - - function min(): Property | undefined { - if (props.length > 0) { - return props[0]; - } - } - function max(): Property | undefined { - if (props.length > 0) { - return props[props.length - 1]; - } - } - - function get(key: TKey): Property | undefined { - for (let i = 0, len = props.length; i < len; i++) { - if (props[i].key === key) { - return props[i]; - } - } - } - - function put(key: TKey, data: TData): void { - if (key !== undefined) { - if (data === undefined) { - remove(key); - } else { - props.push({ key, data }); - props.sort(compareProps); // Go to insertion sort if too slow - } - } - } - function remove(key: TKey): void { - if (key !== undefined) { - for (let i = 0, len = props.length; i < len; i++) { - if (props[i].key === key) { - props[i] = props[len - 1]; - props.length--; - props.sort(compareProps); - break; - } - } - } - } - return { - min, - max, - map, - mapRange, - remove, - get, - put, - }; -} - let logLines: string[]; function log(message: string | number): void { if (logLines) { @@ -156,137 +50,12 @@ function log(message: string | number): void { } } -function printStringProperty(p?: Property): boolean { - log(`[${p?.key}, ${p?.data}]`); - return true; -} - -function printStringNumProperty(p: Property): boolean { - log(`[${p.key}, ${p.data}]`); - return true; -} - -export function simpleTest(): void { - const a = ["Aardvark", "cute", "Baboon", "big", "Chameleon", "colorful", "Dingo", "wild"]; - - const beast = new RedBlackTree(compareStrings); - for (let i = 0; i < a.length; i += 2) { - beast.put(a[i], a[i + 1]); - } - beast.map((element) => printStringProperty(element)); - log("Map B D"); - log("Map Aardvark Dingo"); - log("Map Baboon Chameleon"); - printStringProperty(beast.get("Chameleon")); -} - const clock = (): Trace => Trace.start(); -function took(desc: string, trace: Trace): number { - const duration = trace.trace().duration; - log(`${desc} took ${duration} ms`); - return duration; -} - function elapsedMicroseconds(trace: Trace): number { return trace.trace().duration * 1000; } -export function integerTest1(): number { - const random = makeRandom(0xdeadbeef, 0xfeedbed); - const imin = 0; - const imax = 10000000; - const intCount = 1100000; - const beast = new RedBlackTree(compareNumbers); - - const randInt = (): number => random.integer(imin, imax); - const pos: number[] = Array.from({ length: intCount }); - let i = 0; - let redo = false; - function onConflict(key: number, currentKey: number): { data: number } { - redo = true; - return { data: currentKey }; - } - let conflictCount = 0; - let start = clock(); - while (i < intCount) { - pos[i] = randInt(); - beast.put(pos[i], i, onConflict); - if (redo) { - conflictCount++; - redo = false; - } else { - i++; - } - } - took("test gen", start); - const errorCount = 0; - start = clock(); - for (let j = 0, len = pos.length; j < len; j++) { - const cp = pos[j]; - /* let prop = */ beast.get(cp); - } - const getdur = took("get all keys", start); - log(`cost per get is ${((1000 * getdur) / intCount).toFixed(3)} us`); - log(`duplicates ${conflictCount}, errors ${errorCount}`); - return errorCount; -} - -export function fileTest1(): void { - const content = fs.readFileSync( - path.join(_dirname, "../../../public/literature/shakespeare.txt"), - "utf8", - ); - const a = content.split("\n"); - const iterCount = a.length >> 2; - const removeCount = 10; - log(`len: ${a.length}`); - - for (let k = 0; k < iterCount; k++) { - const beast = new RedBlackTree(compareStrings); - const linearBeast = LinearDictionary(compareStrings); - for (let i = 0, len = a.length; i < len; i++) { - a[i] = a[i].trim(); - if (a[i].length > 0) { - beast.put(a[i], i); - linearBeast.put(a[i], i); - } - } - if (k === 0) { - beast.map((element) => printStringNumProperty(element)); - log("BTREE..."); - } - const removedAnimals: string[] = []; - for (let j = 0; j < removeCount; j++) { - const removeIndex = Math.floor(Math.random() * a.length); - log(`Removing: ${a[removeIndex]} at ${removeIndex}`); - beast.remove(a[removeIndex]); - linearBeast.remove(a[removeIndex]); - removedAnimals.push(a[removeIndex]); - } - for (const animal of a) { - if (animal.length > 0 && !removedAnimals.includes(animal)) { - const prop = beast.get(animal); - const linProp = linearBeast.get(animal); - // log(`Trying key ${animal}`); - if (prop) { - // printStringNumProperty(prop); - if ( - // eslint-disable-next-line @typescript-eslint/prefer-optional-chain -- TODO: ADO#58520 Code owners should verify if this code change is safe and make it if so or update this comment otherwise - linProp === undefined || - prop.key !== linProp.key || - prop.data !== linProp.data - ) { - log(`Linear BST does not match RB BST at key ${animal}`); - } - } else { - log(`hmm...bad key: ${animal}`); - } - } - } - } -} - function printTextSegment(textSegment: ISegmentPrivate, pos: number): boolean { log(textSegment.toString()); log(`at [${pos}, ${pos + textSegment.cachedLength})`); From 2b3c480a3354939b37cadfdf577f7b24d5b556f6 Mon Sep 17 00:00:00 2001 From: Matt Rakow Date: Mon, 31 Aug 2026 09:55:59 -0700 Subject: [PATCH 3/3] refactor(merge-tree): delete the unused comparator helpers `compareNumbers` and `compareStrings` in mergeTreeNodes.ts have no remaining callers. They were removed from the public API in #17952 after a deprecation period, are not re-exported from any barrel and appear in no api-report, and the last references were the RedBlackTree instantiations and LinearDictionary comparator removed earlier in this PR. The identically named helpers in `dds/tree` and `id-compressor` are separate local definitions and are unaffected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- packages/dds/merge-tree/src/mergeTreeNodes.ts | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/packages/dds/merge-tree/src/mergeTreeNodes.ts b/packages/dds/merge-tree/src/mergeTreeNodes.ts index ab755a4060d4..8278933d7ac5 100644 --- a/packages/dds/merge-tree/src/mergeTreeNodes.ts +++ b/packages/dds/merge-tree/src/mergeTreeNodes.ts @@ -694,13 +694,3 @@ export class CollaborationWindow { }; } } - -/** - * Compares two numbers. - */ -export const compareNumbers = (a: number, b: number): number => a - b; - -/** - * Compares two strings. - */ -export const compareStrings = (a: string, b: string): number => a.localeCompare(b);