Skip to content

refactor(merge-tree): drop unnecessary red-black tree usage #28128

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Matt Rakow (ChumpChief) wants to merge 3 commits into
base: main
Choose a base branch
from
+9 −255
Open

refactor(merge-tree): drop unnecessary red-black tree usage #28128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fe3e4ae
refactor(merge-tree): replace RedBlackTree with Map for key/value loo…
ChumpChief Aug 14, 2026
8f6976e
test(merge-tree): remove the unreachable red-black tree exercises
ChumpChief Aug 14, 2026
2b3c480
refactor(merge-tree): delete the unused comparator helpers
ChumpChief Aug 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions packages/dds/merge-tree/src/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -54,7 +53,6 @@ import {
type ISegmentPrivate,
type Marker,
type SegmentGroup,
compareStrings,
isSegmentLeaf,
type ISegmentInternal,
type ISegmentLeaf,
Expand Down Expand Up @@ -173,7 +171,7 @@ export class Client extends TypedEventEmitter<IClientEvents> {

private readonly _mergeTree: MergeTree;

private readonly clientNameToIds = new RedBlackTree<string, number>(compareStrings);
private readonly clientNameToIds = new Map<string, number>();
private readonly shortClientIdMap: string[] = [];

/**
Expand Down Expand Up @@ -829,22 +827,22 @@ export class Client extends TypedEventEmitter<IClientEvents> {
}

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 {
return shortClientId >= 0 ? this.shortClientIdMap[shortClientId] : "original";
}

addLongClientId(longClientId: string): void {
this.clientNameToIds.put(longClientId, this.shortClientIdMap.length);
this.clientNameToIds.set(longClientId, this.shortClientIdMap.length);
this.shortClientIdMap.push(longClientId);
}

Expand Down Expand Up @@ -1718,9 +1716,9 @@ export class Client extends TypedEventEmitter<IClientEvents> {
);
} 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;
}
}
Expand Down
10 changes: 0 additions & 10 deletions packages/dds/merge-tree/src/mergeTreeNodes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
231 changes: 0 additions & 231 deletions packages/dds/merge-tree/src/test/beastTest.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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,
Expand All @@ -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";
Expand All @@ -53,240 +43,19 @@ import { TestClient, getStats, specToSegment } from "./testClient.js";
import { TestServer } from "./testServer.js";
import { loadTextFromFile, nodeOrdinalsHaveIntegrity } from "./testUtils.js";

function LinearDictionary<TKey, TData>(
compareKeys: KeyComparer<TKey>,
): SortedDictionary<TKey, TData> {
const props: Property<TKey, TData>[] = [];
const compareProps = (a: Property<TKey, TData>, b: Property<TKey, TData>): number =>
compareKeys(a.key, b.key);
function mapRange<TAccum>(
action: PropertyAction<TKey, TData>,
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<TAccum>(action: PropertyAction<TKey, TData>, accum?: TAccum): void {
mapRange(action, accum);
}

function min(): Property<TKey, TData> | undefined {
if (props.length > 0) {
return props[0];
}
}
function max(): Property<TKey, TData> | undefined {
if (props.length > 0) {
return props[props.length - 1];
}
}

function get(key: TKey): Property<TKey, TData> | 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) {
logLines.push(message.toString());
}
}

function printStringProperty(p?: Property<string, string>): boolean {
log(`[${p?.key}, ${p?.data}]`);
return true;
}

function printStringNumProperty(p: Property<string, number>): 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<string, string>(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<number, number>(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<string, number>(compareStrings);
const linearBeast = LinearDictionary<string, number>(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})`);
Expand Down
9 changes: 3 additions & 6 deletions packages/dds/merge-tree/src/test/testServer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand All @@ -32,7 +30,7 @@ export class TestServer extends TestClient {
seq = 1;
clients: TestClient[] = [];
clientSeqNumbers: Heap<ClientSeq> = new Heap<ClientSeq>(clientSeqComparer);
upstreamMap: RedBlackTree<number, number> = new RedBlackTree<number, number>(compareNumbers);
upstreamMap: Map<number, number> = new Map<number, number>();
constructor(options?: PropertySet) {
super(options);
}
Expand Down Expand Up @@ -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;
}

Expand Down
Loading