diff --git a/packages/dds/tree/src/core/tree/anchorSet.ts b/packages/dds/tree/src/core/tree/anchorSet.ts index fdade7d3dfd..88dc174a61a 100644 --- a/packages/dds/tree/src/core/tree/anchorSet.ts +++ b/packages/dds/tree/src/core/tree/anchorSet.ts @@ -1226,9 +1226,8 @@ class PathNode extends ReferenceCountedBase implements AnchorNode { assert(this.status === Status.Alive, 0x40e /* PathNode must be alive */); const key = child.parentField; const field = this.children.get(key); - // TODO: should do more optimized search (ex: binary search or better) using child.parentIndex() - // Note that this is the index in the list of child paths, not the index within the field - const childIndex = field?.indexOf(child) ?? -1; + // Note that this is the index in the list of child paths, not the index within the field. + const childIndex = field === undefined ? -1 : binaryFindIndex(field, child.parentIndex); assert(childIndex !== -1, 0x35c /* child must be parented to be removed */); field?.splice(childIndex, 1); if (field?.length === 0) { @@ -1310,6 +1309,32 @@ function binaryFind(sorted: readonly PathNode[], index: number): PathNode | unde return undefined; // If we reach here, target is not in array (or array was not sorted) } +/** + * Find the array index of a child PathNode by its parentIndex using a binary search. + * @param sorted - array of PathNode's sorted by parentIndex. + * @param index - parentIndex being looked for. + * @returns array index of the child with the requested parentIndex, or -1 if not found. + */ +function binaryFindIndex(sorted: readonly PathNode[], index: number): number { + // inclusive + let min = 0; + // exclusive + let max = sorted.length; + + while (min !== max) { + const mid = Math.floor((min + max) / 2); + const found = sorted[mid]!.parentIndex; + if (found === index) { + return mid; + } else if (found > index) { + max = mid; + } else { + min = mid + 1; + } + } + return -1; +} + interface BufferedEvent { node: PathNode; event: keyof AnchorEvents; diff --git a/packages/dds/tree/src/core/tree/sparseTree.ts b/packages/dds/tree/src/core/tree/sparseTree.ts index cd5589bed83..90b430eda18 100644 --- a/packages/dds/tree/src/core/tree/sparseTree.ts +++ b/packages/dds/tree/src/core/tree/sparseTree.ts @@ -3,7 +3,7 @@ * Licensed under the MIT License. */ -import { assert } from "@fluidframework/core-utils/internal"; +import { assert, oob } from "@fluidframework/core-utils/internal"; import type { FieldKey } from "../schema-stored/index.js"; @@ -99,8 +99,8 @@ export class SparseNode implements UpPath { if (field === undefined) { return undefined; } - // TODO: should do more optimized search (ex: binary search or better) using index. - return field.find((c) => c.parentIndex === index); + const childIndex = binaryFindIndex(field, index); + return childIndex === -1 ? undefined : field[childIndex]; } /** @@ -111,9 +111,8 @@ export class SparseNode implements UpPath { public removeChild(child: SparseNode): void { const key = child.parentField; const field = this.children.get(key); - // TODO: should do more optimized search (ex: binary search or better) using child.parentIndex() - // Note that this is the index in the list of child paths, not the index within the field - const childIndex = field?.indexOf(child) ?? -1; + // Note that this is the index in the list of child paths, not the index within the field. + const childIndex = field === undefined ? -1 : binaryFindIndex(field, child.parentIndex); assert(childIndex !== -1, 0x4a5 /* child must be parented to be removed */); field?.splice(childIndex, 1); if (field?.length === 0) { @@ -145,6 +144,34 @@ export class SparseNode implements UpPath { } } +/** + * Find the array index of a child SparseNode by its parentIndex using a binary search. + * @param sorted - array of SparseNode's sorted by parentIndex. + * @param index - parentIndex being looked for. + * @returns array index of the child with the requested parentIndex, or -1 if not found. + */ +function binaryFindIndex(sorted: readonly SparseNode[], index: number): number { + // inclusive + let min = 0; + // exclusive + let max = sorted.length; + + while (min !== max) { + const mid = Math.floor((min + max) / 2); + // mid < max <= sorted.length, so this access is always defined. + const midNode = sorted[mid] ?? oob(); + const found = midNode.parentIndex; + if (found === index) { + return mid; + } else if (found > index) { + max = mid; + } else { + min = mid + 1; + } + } + return -1; +} + export function getDescendant( ancestor: SparseNode, path: UpPath | undefined, diff --git a/packages/dds/tree/src/shared-tree-core/editManager.ts b/packages/dds/tree/src/shared-tree-core/editManager.ts index c3fe2d0d1e2..f930d2fe63a 100644 --- a/packages/dds/tree/src/shared-tree-core/editManager.ts +++ b/packages/dds/tree/src/shared-tree-core/editManager.ts @@ -424,6 +424,19 @@ export class EditManager< return branch.getLocalCommits(); } + /** + * @returns the index of the local commit with the given revision in {@link getLocalCommits} for the given branch, + * or `undefined` if no such commit exists. + * @remarks O(1) alternative to scanning the array returned by {@link getLocalCommits}. + */ + public getLocalCommitIndexByRevision( + branchId: BranchId, + revision: RevisionTag, + ): number | undefined { + const branch = this.getSharedBranch(branchId); + return branch.getLocalCommitIndexByRevision(revision); + } + /** * Gets the length of the longest branch maintained by this `EditManager`. * This may be the length of a peer branch or the local branch. @@ -676,6 +689,21 @@ class SharedBranch { */ private readonly localCommits: GraphCommit[] = []; + /** + * Side-map for O(1) revision -\> {@link localCommits} index lookup. + * @remarks + * Stored values are "absolute" indices that are NOT affected by shifts off the front of {@link localCommits}. + * The actual array index is computed as `value - localCommitsHeadOffset`. + * This avoids having to update every entry in the map when a commit is sequenced (shifted) off the front. + */ + private readonly localCommitsByRevision = new Map(); + + /** + * The number of commits that have been shifted off the front of {@link localCommits} since the last bulk rebuild. + * Used together with {@link localCommitsByRevision} to translate absolute indices into current array indices. + */ + private localCommitsHeadOffset = 0; + /** * Records extra data associated with sequenced commits. * This does not include an entry for the {@link trunkBase}. @@ -709,6 +737,10 @@ class SharedBranch { this.localBranch.events.on("afterChange", (event) => { if (event.type === "append") { for (const commit of event.newCommits) { + this.localCommitsByRevision.set( + commit.revision, + this.localCommits.length + this.localCommitsHeadOffset, + ); this.localCommits.push(commit); } } else { @@ -717,6 +749,11 @@ class SharedBranch { [this.localBranch.getHead(), this.localCommits], this.trunk.getHead(), ); + this.localCommitsByRevision.clear(); + this.localCommitsHeadOffset = 0; + for (const [i, commit] of this.localCommits.entries()) { + this.localCommitsByRevision.set(commit.revision, i); + } } }); } @@ -890,6 +927,18 @@ class SharedBranch { return this.localCommits; } + /** + * @returns the index of the local commit with the given revision in {@link getLocalCommits}, or `undefined` if none exists. + * @remarks This is an O(1) alternative to calling `findIndex` on the array returned by {@link getLocalCommits}. + */ + public getLocalCommitIndexByRevision(revision: RevisionTag): number | undefined { + const absoluteIndex = this.localCommitsByRevision.get(revision); + if (absoluteIndex === undefined) { + return undefined; + } + return absoluteIndex - this.localCommitsHeadOffset; + } + /** * Promote the oldest un-sequenced commit on the local branch to the head of the trunk. * @param sequenceId - The sequence id of the new trunk commit @@ -918,6 +967,12 @@ class SharedBranch { firstLocalCommit !== undefined, 0x6b5 /* Received a sequenced change from the local session despite having no local changes */, ); + this.localCommitsByRevision.delete(firstLocalCommit.revision); + this.localCommitsHeadOffset += 1; + if (this.localCommits.length === 0) { + // Reset the offset when the array drains so it cannot grow unboundedly across sessions. + this.localCommitsHeadOffset = 0; + } const prevSequenceId = this.getCommitSequenceId(this.trunk.getHead().revision); this.pushGraphCommitToTrunk(sequenceId, firstLocalCommit, sessionId); diff --git a/packages/dds/tree/src/shared-tree-core/sharedTreeCore.ts b/packages/dds/tree/src/shared-tree-core/sharedTreeCore.ts index ec8bc318e89..05d209222f1 100644 --- a/packages/dds/tree/src/shared-tree-core/sharedTreeCore.ts +++ b/packages/dds/tree/src/shared-tree-core/sharedTreeCore.ts @@ -555,8 +555,14 @@ export class SharedTreeCore const getLocalCommits = (): GraphCommit[] => { const localCommits = this.editManager.getLocalCommits(branchId); - const revisionIndex = localCommits.findIndex((c) => c.revision === revision); - assert(revisionIndex >= 0, 0xbdb /* revision must exist in local commits */); + const revisionIndex = this.editManager.getLocalCommitIndexByRevision( + branchId, + revision, + ); + assert( + revisionIndex !== undefined, + 0xbdb /* revision must exist in local commits */, + ); return localCommits.slice(revisionIndex); }; diff --git a/packages/dds/tree/src/test/core/tree/anchorSet.spec.ts b/packages/dds/tree/src/test/core/tree/anchorSet.spec.ts index e4e49411a47..3a341d9330e 100644 --- a/packages/dds/tree/src/test/core/tree/anchorSet.spec.ts +++ b/packages/dds/tree/src/test/core/tree/anchorSet.spec.ts @@ -554,6 +554,38 @@ describe("AnchorSet", () => { assert.equal(listenerFired, true); }); + it("removeChild locates the child by parentIndex in a sparse, non-monotonic field", () => { + // This is a regression test for `PathNode.removeChild`'s binary search. + // When the child array is sparse (so the array index is not equal to the parentIndex) + // and contains children with non-monotonic parentIndex values relative to the array + // indices, the binary search must still locate the correct child to remove. + // The order in which we forget anchors guarantees that no `removeChild` invocation + // hits the trivial case where the child sits at array index 0. + const anchors = new AnchorSet(); + // Sparse parentIndex values: array indices 0, 1, 2, 3 hold parentIndex 3, 7, 42, 100. + const anchorAt3 = anchors.track(makePath([fieldFoo, 3])); + const anchorAt7 = anchors.track(makePath([fieldFoo, 7])); + const anchorAt42 = anchors.track(makePath([fieldFoo, 42])); + const anchorAt100 = anchors.track(makePath([fieldFoo, 100])); + + // Remove from the middle of the field, exercising a real binary search. + anchors.forget(anchorAt42); + assert.equal(anchors.locate(anchorAt3)?.parentIndex, 3); + assert.equal(anchors.locate(anchorAt7)?.parentIndex, 7); + assert.equal(anchors.locate(anchorAt100)?.parentIndex, 100); + + // Remove from the end of the field. The remaining array now holds parentIndex 3 and 7, + // so the search target (100) is past the last child and must terminate correctly. + anchors.forget(anchorAt100); + assert.equal(anchors.locate(anchorAt3)?.parentIndex, 3); + assert.equal(anchors.locate(anchorAt7)?.parentIndex, 7); + + // Remove the trailing remaining child, then the leading one. + anchors.forget(anchorAt7); + assert.equal(anchors.locate(anchorAt3)?.parentIndex, 3); + anchors.forget(anchorAt3); + }); + // Simple scenario using just anchorSets to validate if cache implementation of the FlexTree.treeStatus api works. it("AnchorNode cache can be set and retrieved.", () => { const anchors = new AnchorSet(); diff --git a/packages/dds/tree/src/test/shared-tree-core/edit-manager/editManagerCorrectness.test.ts b/packages/dds/tree/src/test/shared-tree-core/edit-manager/editManagerCorrectness.test.ts index 60b0c6cd3a9..e20f26669b1 100644 --- a/packages/dds/tree/src/test/shared-tree-core/edit-manager/editManagerCorrectness.test.ts +++ b/packages/dds/tree/src/test/shared-tree-core/edit-manager/editManagerCorrectness.test.ts @@ -846,6 +846,72 @@ export function testCorrectness(): void { assert.equal(manager.getLongestBranchLength(), 2); }); }); + + describe("getLocalCommitIndexByRevision", () => { + it("returns the correct index after all local commits are sequenced and new commits are appended", () => { + // This is a regression test for the side-map "headOffset drain-reset" branch. + // When all local commits have been sequenced (drained from the front), the head offset + // is reset to zero. New local commits appended afterward must still be locatable by + // revision. + const { manager } = testChangeEditManagerFactory({}); + const local1 = applyLocalCommit(manager, [], 1); + const local2 = applyLocalCommit(manager, [1], 2); + assert.equal(manager.getLocalCommitIndexByRevision("main", local1.revision), 0); + assert.equal(manager.getLocalCommitIndexByRevision("main", local2.revision), 1); + + // Sequence all local commits, draining the array and triggering the head-offset reset. + manager.addSequencedChanges([local1], local1.sessionId, brand(1), brand(0), "main"); + manager.addSequencedChanges([local2], local2.sessionId, brand(2), brand(1), "main"); + assert.deepEqual(manager.getLocalCommits("main"), []); + assert.equal( + manager.getLocalCommitIndexByRevision("main", local1.revision), + undefined, + ); + assert.equal( + manager.getLocalCommitIndexByRevision("main", local2.revision), + undefined, + ); + + // Append new local commits. With the head offset reset to 0, the absolute indices + // stored for these commits should equal their actual array indices. + const local3 = applyLocalCommit(manager, [1, 2], 3); + const local4 = applyLocalCommit(manager, [1, 2, 3], 4); + assert.equal(manager.getLocalCommitIndexByRevision("main", local3.revision), 0); + assert.equal(manager.getLocalCommitIndexByRevision("main", local4.revision), 1); + }); + + it("returns the correct index after a non-append afterChange event rebuilds the side-map", () => { + // This is a regression test for the side-map "clear-and-rebuild" branch. + // A non-append afterChange event (e.g. one triggered by rebasing the local branch + // over incoming peer commits) clears the side-map and rebuilds it from the new + // local commit set. Lookups after such an event must still be accurate. + const { manager } = testChangeEditManagerFactory({}); + const local1 = applyLocalCommit(manager, [], 1); + const local2 = applyLocalCommit(manager, [1], 2); + assert.equal(manager.getLocalCommitIndexByRevision("main", local1.revision), 0); + assert.equal(manager.getLocalCommitIndexByRevision("main", local2.revision), 1); + + // A peer commit that the local commits must be rebased over triggers a non-append + // afterChange on the local branch. + const peerCommit1 = peerCommit(peer1, [], 10); + manager.addSequencedChanges( + [peerCommit1], + peerCommit1.sessionId, + brand(1), + brand(0), + "main", + ); + + // After the rebase, the local commits remain (with the same revisions) but the + // side-map has been cleared and rebuilt by the clear-and-rebuild branch. + const localCommitsAfterRebase = manager.getLocalCommits("main"); + assert.equal(localCommitsAfterRebase.length, 2); + assert.equal(localCommitsAfterRebase[0].revision, local1.revision); + assert.equal(localCommitsAfterRebase[1].revision, local2.revision); + assert.equal(manager.getLocalCommitIndexByRevision("main", local1.revision), 0); + assert.equal(manager.getLocalCommitIndexByRevision("main", local2.revision), 1); + }); + }); }); /**