From dd433df79e801862beebfecde4d65be6609b213c Mon Sep 17 00:00:00 2001 From: Timothy Trindle <48774137+titrindl@users.noreply.github.com> Date: Mon, 30 Sep 2024 16:25:12 -0700 Subject: [PATCH] mergeTree: implement sided obliterate (#22643) This PR implements the new OBLITERATE_SIDED op type introduced by #22596. This allows obliteration ranges to be defined using either inclusive or exclusive bounds on each endpoint. Using an exclusive bound will allow the obliterated range to "grow" to include concurrently inserted segments adjacent to that endpoint. --- packages/dds/merge-tree/src/client.ts | 108 ++++++++++++++--- packages/dds/merge-tree/src/mergeTree.ts | 114 ++++++++++++++---- packages/dds/merge-tree/src/mergeTreeNodes.ts | 3 +- .../test/obliterate.rangeExpansion.spec.ts | 87 ++++++++----- .../merge-tree/src/test/testClientLogger.ts | 27 +++-- 5 files changed, 262 insertions(+), 77 deletions(-) diff --git a/packages/dds/merge-tree/src/client.ts b/packages/dds/merge-tree/src/client.ts index f0fdb463d3cd..3093405bb1eb 100644 --- a/packages/dds/merge-tree/src/client.ts +++ b/packages/dds/merge-tree/src/client.ts @@ -77,7 +77,7 @@ import { } from "./ops.js"; import { PropertySet } from "./properties.js"; import { DetachedReferencePosition, ReferencePosition } from "./referencePositions.js"; -import { type InteriorSequencePlace } from "./sequencePlace.js"; +import { Side, type InteriorSequencePlace } from "./sequencePlace.js"; import { SnapshotLoader } from "./snapshotLoader.js"; import { SnapshotV1 } from "./snapshotV1.js"; import { SnapshotLegacy } from "./snapshotlegacy.js"; @@ -494,17 +494,16 @@ export class Client extends TypedEventEmitter { const op = opArgs.op; const clientArgs = this.getClientSequenceArgs(opArgs); if (this._mergeTree.options?.mergeTreeEnableSidedObliterate) { - /* - const _start: InteriorSequencePlace = - typeof op.pos1 === "object" - ? { pos: op.pos1.pos, side: op.pos1.before ? Side.Before : Side.After } - : { pos: op.pos1, side: Side.Before }; - const _end: InteriorSequencePlace = - typeof op.pos2 === "object" - ? { pos: op.pos2.pos, side: op.pos2.before ? Side.Before : Side.After } - : { pos: op.pos2 - 1, side: Side.After }; - */ - assert(false, "TODO: sided obliterate will come in a follow-up PR shortly."); + const { start, end } = this.getValidSidedRange(op, clientArgs); + this._mergeTree.obliterateRange( + start, + end, + clientArgs.referenceSequenceNumber, + clientArgs.clientId, + clientArgs.sequenceNumber, + false, + opArgs, + ); } else { assert( op.type === MergeTreeDeltaType.OBLITERATE, @@ -598,6 +597,82 @@ export class Client extends TypedEventEmitter { ); } + /** + * Returns a valid range for the op, or throws if the range is invalid + * @param op - The op to generate the range for + * @param clientArgs - The client args for the op + * @throws LoggingError if the range is invalid + */ + private getValidSidedRange( + // eslint-disable-next-line import/no-deprecated + op: IMergeTreeObliterateSidedMsg | IMergeTreeObliterateMsg, + clientArgs: IMergeTreeClientSequenceArgs, + ): { + start: InteriorSequencePlace; + end: InteriorSequencePlace; + } { + const invalidPositions: string[] = []; + let start: InteriorSequencePlace | undefined; + let end: InteriorSequencePlace | undefined; + if (op.pos1 === undefined) { + invalidPositions.push("start"); + } else { + start = + typeof op.pos1 === "object" + ? { pos: op.pos1.pos, side: op.pos1.before ? Side.Before : Side.After } + : { pos: op.pos1, side: Side.Before }; + } + if (op.pos2 === undefined) { + invalidPositions.push("end"); + } else { + end = + typeof op.pos2 === "object" + ? { pos: op.pos2.pos, side: op.pos2.before ? Side.Before : Side.After } + : { pos: op.pos2 - 1, side: Side.After }; + } + + // Validate if local op + if (clientArgs.clientId === this.getClientId()) { + const length = this._mergeTree.getLength( + this.getCollabWindow().currentSeq, + this.getClientId(), + ); + if (start !== undefined && (start.pos >= length || start.pos < 0)) { + // start out of bounds + invalidPositions.push("start"); + } + if (end !== undefined && (end.pos >= length || end.pos < 0)) { + invalidPositions.push("end"); + } + if ( + start !== undefined && + end !== undefined && + (start.pos > end.pos || + (start.pos === end.pos && start.side !== end.side && start.side === Side.After)) + ) { + // end is before start + invalidPositions.push("inverted"); + } + if (invalidPositions.length > 0) { + throw new LoggingError("InvalidRange", { + usageError: true, + invalidPositions: invalidPositions.toString(), + length, + opType: op.type, + opPos1Relative: op.relativePos1 !== undefined, + opPos2Relative: op.relativePos2 !== undefined, + opPos1: JSON.stringify(op.pos1), + opPos2: JSON.stringify(op.pos2), + start: JSON.stringify(start), + end: JSON.stringify(end), + }); + } + } + + assert(start !== undefined && end !== undefined, "Missing start or end of range"); + return { start, end }; + } + /** * Returns a valid range for the op, or undefined * @param op - The op to generate the range for @@ -976,7 +1051,8 @@ export class Client extends TypedEventEmitter { this.applyAnnotateRangeOp(opArgs); break; } - case MergeTreeDeltaType.OBLITERATE: { + case MergeTreeDeltaType.OBLITERATE: + case MergeTreeDeltaType.OBLITERATE_SIDED: { this.applyObliterateRangeOp(opArgs); break; } @@ -1010,14 +1086,11 @@ export class Client extends TypedEventEmitter { this.applyAnnotateRangeOp({ op }); break; } + case MergeTreeDeltaType.OBLITERATE_SIDED: case MergeTreeDeltaType.OBLITERATE: { this.applyObliterateRangeOp({ op }); break; } - case MergeTreeDeltaType.OBLITERATE_SIDED: { - assert(false, "TODO: sided obliterate will come in a follow-up PR shortly."); - break; - } case MergeTreeDeltaType.GROUP: { op.ops.map((o) => this.applyStashedOp(o)); break; @@ -1246,6 +1319,7 @@ export class Client extends TypedEventEmitter { this.applyRemoveRangeOp(opArgs); break; } + case MergeTreeDeltaType.OBLITERATE_SIDED: case MergeTreeDeltaType.OBLITERATE: { this.applyObliterateRangeOp(opArgs); break; diff --git a/packages/dds/merge-tree/src/mergeTree.ts b/packages/dds/merge-tree/src/mergeTree.ts index d76daa8e5948..5b5137224bc9 100644 --- a/packages/dds/merge-tree/src/mergeTree.ts +++ b/packages/dds/merge-tree/src/mergeTree.ts @@ -90,6 +90,7 @@ import { } from "./referencePositions.js"; // eslint-disable-next-line import/no-deprecated import { PropertiesRollback } from "./segmentPropertiesManager.js"; +import { Side, type InteriorSequencePlace } from "./sequencePlace.js"; import { SortedSegmentSet } from "./sortedSegmentSet.js"; import { zamboniSegments } from "./zamboni.js"; @@ -1247,7 +1248,10 @@ export class MergeTree { }); }); - if (opArgs.op.type === MergeTreeDeltaType.OBLITERATE) { + if ( + opArgs.op.type === MergeTreeDeltaType.OBLITERATE || + opArgs.op.type === MergeTreeDeltaType.OBLITERATE_SIDED + ) { this.obliterates.addOrUpdate(pendingSegmentGroup.obliterateInfo!); } @@ -1255,7 +1259,8 @@ export class MergeTree { // positions after slide are final if ( opArgs.op.type === MergeTreeDeltaType.REMOVE || - opArgs.op.type === MergeTreeDeltaType.OBLITERATE + opArgs.op.type === MergeTreeDeltaType.OBLITERATE || + opArgs.op.type === MergeTreeDeltaType.OBLITERATE_SIDED ) { this.slideAckedRemovedSegmentReferences(pendingSegmentGroup.segments); } @@ -1555,13 +1560,13 @@ export class MergeTree { movedClientIds.unshift(ob.clientId); movedSeqs.unshift(ob.seq); } else { - if (newest === undefined || normalizedNewestSeq < normalizedObSeq) { - normalizedNewestSeq = normalizedObSeq; - newest = ob; - } movedClientIds.push(ob.clientId); movedSeqs.push(ob.seq); } + if (newest === undefined || normalizedNewestSeq < normalizedObSeq) { + normalizedNewestSeq = normalizedObSeq; + newest = ob; + } } } @@ -1588,7 +1593,10 @@ export class MergeTree { if (newSegment.parent) { this.blockUpdatePathLengths(newSegment.parent, seq, clientId); } + } else if (oldest && newest?.clientId === clientId) { + newSegment.prevObliterateByInserter = newest; } + saveIfLocal(newSegment); } } @@ -1880,19 +1888,20 @@ export class MergeTree { } } - public obliterateRange( - start: number, - end: number, + private obliterateRangeSided( + start: InteriorSequencePlace, + end: InteriorSequencePlace, refSeq: number, clientId: number, seq: number, overwrite: boolean = false, opArgs: IMergeTreeDeltaOpArgs, ): void { - errorIfOptionNotTrue(this.options, "mergeTreeEnableObliterate"); + const startPos = start.side === Side.Before ? start.pos : start.pos + 1; + const endPos = end.side === Side.Before ? end.pos : end.pos + 1; - this.ensureIntervalBoundary(start, refSeq, clientId); - this.ensureIntervalBoundary(end, refSeq, clientId); + this.ensureIntervalBoundary(startPos, refSeq, clientId); + this.ensureIntervalBoundary(endPos, refSeq, clientId); let _overwrite = overwrite; const localOverlapWithRefs: ISegment[] = []; @@ -1910,8 +1919,8 @@ export class MergeTree { segmentGroup: undefined, }; - const { segment: startSeg } = this.getContainingSegment(start, refSeq, clientId); - const { segment: endSeg } = this.getContainingSegment(end - 1, refSeq, clientId); + const { segment: startSeg } = this.getContainingSegment(start.pos, refSeq, clientId); + const { segment: endSeg } = this.getContainingSegment(end.pos, refSeq, clientId); assert( startSeg !== undefined && endSeg !== undefined, 0xa3f /* segments cannot be undefined */, @@ -1919,7 +1928,7 @@ export class MergeTree { obliterate.start = this.createLocalReferencePosition( startSeg, - 0, + start.side === Side.Before ? 0 : Math.max(startSeg.cachedLength - 1, 0), ReferenceType.StayOnRemove, { obliterate, @@ -1928,20 +1937,53 @@ export class MergeTree { obliterate.end = this.createLocalReferencePosition( endSeg, - endSeg.cachedLength - 1, + end.side === Side.Before ? 0 : Math.max(endSeg.cachedLength - 1, 0), ReferenceType.StayOnRemove, { obliterate, }, ); + // Always create a segment group for obliterate, + // even if there are no segments currently in the obliteration range. + // Segments may be concurrently inserted into the obliteration range, + // at which point they are added to the segment group. + obliterate.segmentGroup = { + segments: [], + localSeq, + refSeq: this.collabWindow.currentSeq, + obliterateInfo: obliterate, + }; + if (this.collabWindow.collaborating && clientId === this.collabWindow.clientId) { + this.pendingSegments.push(obliterate.segmentGroup); + } + this.obliterates.addOrUpdate(obliterate); + const markMoved = ( segment: ISegment, pos: number, _start: number, _end: number, ): boolean => { + if ( + (start.side === Side.After && startPos === pos + segment.cachedLength) || // exclusive start segment + (end.side === Side.Before && + endPos === pos && + isSegmentPresent(segment, { refSeq, localSeq })) // exclusive end segment + ) { + // We walk these segments because we want to also walk any concurrently inserted segments between here and the obliterated segments. + // These segments are outside of the obliteration range though, so return true to keep walking. + return true; + } const existingMoveInfo = toMoveInfo(segment); + + if (segment.prevObliterateByInserter?.seq === UnassignedSequenceNumber) { + // We chose to not obliterate this segment because we are aware of an unacked local obliteration. + // The local obliterate has not been sequenced yet, so it is still the newest obliterate we are aware of. + // Other clients will also choose not to obliterate this segment because the most recent obliteration has the same clientId + return true; + } + if ( clientId !== segment.clientId && segment.seq !== undefined && @@ -1992,7 +2034,6 @@ export class MergeTree { obliterate.segmentGroup, localSeq, ); - obliterate.segmentGroup.obliterateInfo ??= obliterate; } else { if (MergeTree.options.zamboniSegments) { this.addToLRUSet(segment, seq); @@ -2022,14 +2063,12 @@ export class MergeTree { markMoved, undefined, afterMarkMoved, - start, - end, + start.pos, + end.pos + 1, // include the segment containing the end reference undefined, seq === UnassignedSequenceNumber ? undefined : seq, ); - this.obliterates.addOrUpdate(obliterate); - this.slideAckedRemovedSegmentReferences(localOverlapWithRefs); // opArgs == undefined => test code if (movedSegments.length > 0) { @@ -2055,6 +2094,39 @@ export class MergeTree { } } + public obliterateRange( + start: number | InteriorSequencePlace, + end: number | InteriorSequencePlace, + refSeq: number, + clientId: number, + seq: number, + overwrite: boolean = false, + opArgs: IMergeTreeDeltaOpArgs, + ): void { + errorIfOptionNotTrue(this.options, "mergeTreeEnableObliterate"); + if (this.options?.mergeTreeEnableSidedObliterate) { + assert( + typeof start === "object" && typeof end === "object", + "Start and end must be of type InteriorSequencePlace if mergeTreeEnableSidedObliterate is enabled.", + ); + this.obliterateRangeSided(start, end, refSeq, clientId, seq, overwrite, opArgs); + } else { + assert( + typeof start === "number" && typeof end === "number", + "Start and end must be numbers if mergeTreeEnableSidedObliterate is not enabled.", + ); + this.obliterateRangeSided( + { pos: start, side: Side.Before }, + { pos: end - 1, side: Side.After }, + refSeq, + clientId, + seq, + overwrite, + opArgs, + ); + } + } + public markRangeRemoved( start: number, end: number, diff --git a/packages/dds/merge-tree/src/mergeTreeNodes.ts b/packages/dds/merge-tree/src/mergeTreeNodes.ts index 0ce08c7d1643..4738bc9cdaec 100644 --- a/packages/dds/merge-tree/src/mergeTreeNodes.ts +++ b/packages/dds/merge-tree/src/mergeTreeNodes.ts @@ -668,7 +668,8 @@ export abstract class BaseSegment implements ISegment { return false; } - case MergeTreeDeltaType.OBLITERATE: { + case MergeTreeDeltaType.OBLITERATE: + case MergeTreeDeltaType.OBLITERATE_SIDED: { const moveInfo: IMoveInfo | undefined = toMoveInfo(this); assert(moveInfo !== undefined, 0x86e /* On obliterate ack, missing move info! */); const obliterateInfo = segmentGroup.obliterateInfo; diff --git a/packages/dds/merge-tree/src/test/obliterate.rangeExpansion.spec.ts b/packages/dds/merge-tree/src/test/obliterate.rangeExpansion.spec.ts index d358d5e418fd..6d6ef9a8604c 100644 --- a/packages/dds/merge-tree/src/test/obliterate.rangeExpansion.spec.ts +++ b/packages/dds/merge-tree/src/test/obliterate.rangeExpansion.spec.ts @@ -22,8 +22,7 @@ function itCorrectlyObliterates({ const events: number[] = []; const helper = new ReconnectTestHelper({ - // TODO: uncomment this once the flag is checked in - /* mergeTreeEnableSidedObliterate: true */ + mergeTreeEnableSidedObliterate: true, }); helper.clients.A.on("delta", (opArgs, deltaArgs) => { events.push(deltaArgs.operation); @@ -34,8 +33,15 @@ function itCorrectlyObliterates({ helper.logger.validate({ baseText: expectedText }); }); } +itCorrectlyObliterates.skip = ({ + title, +}: { + title: string; + action: (helper: ReconnectTestHelper) => void; + expectedText: string; +}) => it.skip(title, () => {}); -describe.skip("obliterate", () => { +describe("obliterate", () => { itCorrectlyObliterates({ title: "obliterate adjacent insert", action: (helper) => { @@ -61,17 +67,6 @@ describe.skip("obliterate", () => { }, expectedText: "XYZhe world", }); - itCorrectlyObliterates({ - title: "zero length obliterate in the middle of the string", - action: (helper) => { - helper.insertText("A", 0, "hello world"); - helper.processAllOps(); - - helper.obliterateRange("A", { pos: 0, side: Side.After }, { pos: 1, side: Side.Before }); - helper.insertText("B", 0, "more "); - }, - expectedText: "hello world", - }); itCorrectlyObliterates({ title: "obliterate, then insert at the end of the string", action: (helper) => { @@ -102,17 +97,6 @@ describe.skip("obliterate", () => { }, expectedText: "hello", }); - itCorrectlyObliterates({ - title: "zero length obliterate in the middle of the string", - action: (helper) => { - helper.insertText("A", 0, "hello world"); - helper.processAllOps(); - - helper.obliterateRange("A", { pos: 0, side: Side.After }, { pos: 1, side: Side.Before }); - helper.insertText("B", 0, "more "); - }, - expectedText: "hello world", - }); itCorrectlyObliterates({ title: "obliterate, then insert at the end of the string", action: (helper) => { @@ -143,9 +127,58 @@ describe.skip("obliterate", () => { }, expectedText: "hello", }); + describe("zero length", () => { + // TODO: #17785: Allow start and end to be used as obliteration range endpoints. + itCorrectlyObliterates.skip({ + title: "zero length obliterate at the start of the string", + action: (helper) => { + helper.insertText("A", 0, "hello world"); + helper.processAllOps(); + + helper.obliterateRange( + "A", + { pos: -1, side: Side.After }, + { pos: 0, side: Side.Before }, + ); + helper.insertText("B", 0, "more "); + }, + expectedText: "hello world", + }); + itCorrectlyObliterates({ + title: "zero length obliterate in the middle of the string", + action: (helper) => { + helper.insertText("A", 0, "hello world"); + helper.processAllOps(); + + helper.obliterateRange( + "A", + { pos: 0, side: Side.After }, + { pos: 1, side: Side.Before }, + ); + helper.insertText("B", 1, "more "); + }, + expectedText: "hello world", + }); + // TODO: #17785: Allow start and end to be used as obliteration range endpoints. + itCorrectlyObliterates.skip({ + title: "zero length obliterate at the end of the string", + action: (helper) => { + helper.insertText("A", 0, "hello world"); + helper.processAllOps(); + + helper.obliterateRange( + "A", + { pos: helper.clients.A.getLength() - 1, side: Side.After }, + { pos: -1, side: Side.Before }, + ); + helper.insertText("B", helper.clients.B.getLength(), " more"); + }, + expectedText: "hello world", + }); + }); }); -describe.skip("overlapping edits", () => { +describe("overlapping edits", () => { itCorrectlyObliterates({ title: "overlapping obliterate and obliterate", action: (helper) => { @@ -268,7 +301,7 @@ describe.skip("reconnect", () => { }); }); -describe.skip("sided obliterates", () => { +describe("sided obliterates", () => { /** * All test cases will operate on the same numerical positions, but differ on their sidedness: * 1. A expand both endpoints, B expand neither endpoint = expand range on both endpoints diff --git a/packages/dds/merge-tree/src/test/testClientLogger.ts b/packages/dds/merge-tree/src/test/testClientLogger.ts index bef4fe748172..8d6d85a56925 100644 --- a/packages/dds/merge-tree/src/test/testClientLogger.ts +++ b/packages/dds/merge-tree/src/test/testClientLogger.ts @@ -23,7 +23,7 @@ import { toRemovalInfo, type ISegment, } from "../mergeTreeNodes.js"; -import { IMergeTreeOp } from "../ops.js"; +import { IMergeTreeOp, MergeTreeDeltaType } from "../ops.js"; import { PropertySet, matchProperties } from "../properties.js"; import { TextSegment } from "../textSegment.js"; @@ -35,16 +35,21 @@ function getOpString(msg: ISequencedDocumentMessage | undefined): string { } const op = msg.contents as IMergeTreeOp; const opType = op.type.toString(); - // eslint-disable-next-line @typescript-eslint/dot-notation - const pos1Side = op?.["before1"] === undefined ? "" : op["before1"] ? "[" : "("; - // eslint-disable-next-line @typescript-eslint/dot-notation - const pos2Side = op?.["before2"] === undefined ? "" : op["before2"] ? ")" : "]"; - const opPos = - // eslint-disable-next-line @typescript-eslint/dot-notation - op?.["pos1"] === undefined - ? "" - : // eslint-disable-next-line @typescript-eslint/dot-notation - `@${pos1Side}${op["pos1"]}${op["pos2"] === undefined ? "" : `,${op["pos2"]}${pos2Side}`}`; + let opPos; + if (op.type === MergeTreeDeltaType.OBLITERATE_SIDED) { + const pos1Side = + op.type === MergeTreeDeltaType.OBLITERATE_SIDED ? (op.pos1.before ? "[" : "(") : ""; + const pos2Side = + op.type === MergeTreeDeltaType.OBLITERATE_SIDED ? (op.pos2.before ? ")" : "]") : ""; + opPos = `@${pos1Side}${op.pos1.pos},${op.pos2.pos}${pos2Side}`; + } else { + opPos = + // eslint-disable-next-line @typescript-eslint/dot-notation + op?.["pos1"] === undefined + ? "" + : // eslint-disable-next-line @typescript-eslint/dot-notation + `@${op["pos1"]}${op["pos2"] === undefined ? "" : `,${op["pos2"]}`}`; + } const seq = msg.sequenceNumber < 0 ? "L" : msg.sequenceNumber.toString(); const ref = msg.referenceSequenceNumber.toString();