From 2eb374ba408f0d0f725d90c660271934d4a7a3d8 Mon Sep 17 00:00:00 2001 From: Jenn Date: Wed, 13 Mar 2024 20:17:51 +0000 Subject: [PATCH 1/5] todo --- packages/dds/tree/src/core/tree/visitDelta.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/dds/tree/src/core/tree/visitDelta.ts b/packages/dds/tree/src/core/tree/visitDelta.ts index 3b5b934d7281..384502eca5ec 100644 --- a/packages/dds/tree/src/core/tree/visitDelta.ts +++ b/packages/dds/tree/src/core/tree/visitDelta.ts @@ -78,6 +78,7 @@ export function visitDelta( rootDestructions, }; processBuilds(delta.build, detachConfig, visitor); + // todoj process refreshers without building them? visitFieldMarks(delta.fields, visitor, detachConfig); fixedPointVisitOfRoots(visitor, detachPassRoots, detachConfig); transferRoots(rootTransfers, attachPassRoots, detachedFieldIndex, visitor); From f043d723f1bb9c6d98eca8a4c7a507338728044c Mon Sep 17 00:00:00 2001 From: Jenn Date: Mon, 25 Mar 2024 17:50:42 +0000 Subject: [PATCH 2/5] create refreshers when needed --- packages/dds/tree/src/core/tree/visitDelta.ts | 78 +++++++-- .../dds/tree/src/test/tree/visitDelta.spec.ts | 149 ++++++++++++++++++ 2 files changed, 212 insertions(+), 15 deletions(-) diff --git a/packages/dds/tree/src/core/tree/visitDelta.ts b/packages/dds/tree/src/core/tree/visitDelta.ts index 26504f0b8c94..6f7580e6fe57 100644 --- a/packages/dds/tree/src/core/tree/visitDelta.ts +++ b/packages/dds/tree/src/core/tree/visitDelta.ts @@ -5,7 +5,10 @@ import { assert } from "@fluidframework/core-utils"; +import { NestedMap } from "../../index.js"; +import { nestedMapFromFlatList, tryGetFromNestedMap } from "../../util/index.js"; import { FieldKey } from "../schema-stored/index.js"; +import { ITreeCursorSynchronous } from "./cursor.js"; import * as Delta from "./delta.js"; import { ProtoNodes } from "./delta.js"; import { @@ -16,6 +19,7 @@ import { offsetDetachId, } from "./deltaUtil.js"; import { DetachedFieldIndex, ForestRootId } from "./detachedFieldIndex.js"; +import { Major, Minor } from "./detachedFieldIndexTypes.js"; import { NodeIndex, PlaceIndex, Range } from "./pathTree.js"; /** @@ -70,8 +74,18 @@ export function visitDelta( const attachPassRoots: Map = new Map(); const rootTransfers: Delta.DetachedNodeRename[] = []; const rootDestructions: Delta.DetachedNodeDestruction[] = []; + const refresherData: [Major, Minor, ITreeCursorSynchronous][] = []; + delta.refreshers?.forEach(({ id: { major, minor }, trees }) => { + for (let i = 0; i < trees.length; i += 1) { + const offsettedId = minor + i; + refresherData.push([major, offsettedId, trees[i]]); + } + }); + const refreshers: NestedMap = + nestedMapFromFlatList(refresherData); const detachConfig: PassConfig = { func: detachPass, + refreshers, detachedFieldIndex, detachPassRoots, attachPassRoots, @@ -79,12 +93,12 @@ export function visitDelta( rootDestructions, }; processBuilds(delta.build, detachConfig, visitor); - // todoj process refreshers without building them? visitFieldMarks(delta.fields, visitor, detachConfig); fixedPointVisitOfRoots(visitor, detachPassRoots, detachConfig); transferRoots(rootTransfers, attachPassRoots, detachedFieldIndex, visitor); const attachConfig: PassConfig = { func: attachPass, + refreshers, detachedFieldIndex, detachPassRoots, attachPassRoots, @@ -290,7 +304,16 @@ export interface DeltaVisitor { interface PassConfig { readonly func: Pass; readonly detachedFieldIndex: DetachedFieldIndex; - + /** + * A mapping between forest root id and trees that represent refresher data. Each entry is only + * created in the forest once needed. + */ + readonly refreshers: NestedMap; + /** + * Nested changes on roots that need to be visited as part of the detach pass. + * Each entry is removed when its associated changes are visited. + */ + // readonly detachPassRoots: Map; /** * Nested changes on roots that need to be visited as part of the detach pass. * Each entry is removed when its associated changes are visited. @@ -360,7 +383,13 @@ function visitNode( function detachPass(delta: Delta.FieldChanges, visitor: DeltaVisitor, config: PassConfig): void { if (delta.global !== undefined) { for (const { id, fields } of delta.global) { - const root = config.detachedFieldIndex.getEntry(id); + let root = config.detachedFieldIndex.tryGetEntry(id); + if (root === undefined) { + const tree = tryGetFromNestedMap(config.refreshers, id.major, id.minor); + assert(tree !== undefined, "refresher data not found"); + buildTrees(id, [tree], config, visitor); + root = config.detachedFieldIndex.getEntry(id); + } config.detachPassRoots.set(root, fields); config.attachPassRoots.set(root, fields); } @@ -397,6 +426,25 @@ function detachPass(delta: Delta.FieldChanges, visitor: DeltaVisitor, config: Pa } } +function buildTrees( + id: Delta.DetachedNodeId, + trees: readonly ITreeCursorSynchronous[], + config: PassConfig, + visitor: DeltaVisitor, +) { + for (let i = 0; i < trees.length; i += 1) { + const offsettedId = offsetDetachId(id, i); + let root = config.detachedFieldIndex.tryGetEntry(offsettedId); + // Tree building is idempotent. We can therefore ignore build instructions for trees that already exist. + // The idempotence is leveraged by undo/redo as well as sandwich rebasing. + if (root === undefined) { + root = config.detachedFieldIndex.createEntry(offsettedId); + const field = config.detachedFieldIndex.toFieldKey(root); + visitor.create([trees[i]], field); + } + } +} + function processBuilds( builds: readonly Delta.DetachedNodeBuild[] | undefined, config: PassConfig, @@ -404,17 +452,7 @@ function processBuilds( ) { if (builds !== undefined) { for (const { id, trees } of builds) { - for (let i = 0; i < trees.length; i += 1) { - const offsettedId = offsetDetachId(id, i); - let root = config.detachedFieldIndex.tryGetEntry(offsettedId); - // Tree building is idempotent. We can therefore ignore build instructions for trees that already exist. - // The idempotence is leveraged by undo/redo as well as sandwich rebasing. - if (root === undefined) { - root = config.detachedFieldIndex.createEntry(offsettedId); - const field = config.detachedFieldIndex.toFieldKey(root); - visitor.create([trees[i]], field); - } - } + buildTrees(id, trees, config, visitor); } } } @@ -442,7 +480,17 @@ function attachPass(delta: Delta.FieldChanges, visitor: DeltaVisitor, config: Pa for (let i = 0; i < mark.count; i += 1) { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion const offsetAttachId = offsetDetachId(mark.attach!, i); - const sourceRoot = config.detachedFieldIndex.getEntry(offsetAttachId); + let sourceRoot = config.detachedFieldIndex.tryGetEntry(offsetAttachId); + if (sourceRoot === undefined) { + const tree = tryGetFromNestedMap( + config.refreshers, + offsetAttachId.major, + offsetAttachId.minor, + ); + assert(tree !== undefined, "refresher data not found"); + buildTrees(offsetAttachId, [tree], config, visitor); + sourceRoot = config.detachedFieldIndex.getEntry(offsetAttachId); + } const sourceField = config.detachedFieldIndex.toFieldKey(sourceRoot); const offsetIndex = index + i; if (isReplaceMark(mark)) { diff --git a/packages/dds/tree/src/test/tree/visitDelta.spec.ts b/packages/dds/tree/src/test/tree/visitDelta.spec.ts index 89d3937d959a..4ed2d1b47594 100644 --- a/packages/dds/tree/src/test/tree/visitDelta.spec.ts +++ b/packages/dds/tree/src/test/tree/visitDelta.spec.ts @@ -990,6 +990,155 @@ describe("visitDelta", () => { { id: node1, root: 3 }, ]); }); + + describe.only("builds refreshers", () => { + it("for restores at the root", () => { + const index = makeDetachedFieldIndex("", testRevisionTagCodec); + const node = { minor: 42 }; + const rootFieldDelta: DeltaFieldChanges = { + local: [{ count: 1, attach: node }], + }; + const delta: DeltaRoot = { + refreshers: [{ id: node, trees: [content] }], + fields: new Map([[rootKey, rootFieldDelta]]), + }; + const expected: VisitScript = [ + ["enterField", rootKey], + ["exitField", rootKey], + ["enterField", rootKey], + ["create", [content], field0], + ["attach", field0, 1, 0], + ["exitField", rootKey], + ]; + testVisit(delta, expected, index); + assert.equal(index.entries().next().done, true); + }); + + it("for restores under a child", () => { + const index = makeDetachedFieldIndex("", testRevisionTagCodec); + const buildId = { minor: 42 }; + const rootFieldDelta: DeltaFieldChanges = { + local: [ + { + count: 1, + fields: new Map([[fooKey, { local: [{ count: 1, attach: buildId }] }]]), + }, + ], + }; + const expected: VisitScript = [ + ["enterField", rootKey], + ["enterNode", 0], + ["enterField", fooKey], + ["exitField", fooKey], + ["exitNode", 0], + ["exitField", rootKey], + ["enterField", rootKey], + ["enterNode", 0], + ["enterField", fooKey], + ["create", [content], field0], + ["attach", field0, 1, 0], + ["exitField", fooKey], + ["exitNode", 0], + ["exitField", rootKey], + ]; + const delta: DeltaRoot = { + refreshers: [{ id: buildId, trees: [content] }], + fields: new Map([[rootKey, rootFieldDelta]]), + }; + testVisit(delta, expected, index); + assert.equal(index.entries().next().done, true); + }); + + it("for partial restores", () => { + const index = makeDetachedFieldIndex("", testRevisionTagCodec); + const node = { minor: 42 }; + const rootFieldDelta: DeltaFieldChanges = { + local: [{ count: 1, attach: { minor: 43 } }], + }; + const delta: DeltaRoot = { + refreshers: [{ id: node, trees: [content, content] }], + fields: new Map([[rootKey, rootFieldDelta]]), + }; + const expected: VisitScript = [ + ["enterField", rootKey], + ["exitField", rootKey], + ["enterField", rootKey], + ["create", [content], field0], + ["attach", field0, 1, 0], + ["exitField", rootKey], + ]; + testVisit(delta, expected, index); + assert.equal(index.entries().next().done, true); + }); + + it("for changes to repair data", () => { + const index = makeDetachedFieldIndex("", testRevisionTagCodec); + const refresherId = { minor: 42 }; + const buildId = { minor: 43 }; + const rootFieldDelta: DeltaFieldChanges = { + global: [ + { + id: refresherId, + fields: new Map([[fooKey, { local: [{ count: 1, attach: buildId }] }]]), + }, + ], + }; + const expected: VisitScript = [ + ["create", [content], field0], + ["enterField", rootKey], + ["create", [content], field1], + ["exitField", rootKey], + ["enterField", field1], + ["enterNode", 0], + ["enterField", fooKey], + ["exitField", fooKey], + ["exitNode", 0], + ["exitField", field1], + ["enterField", rootKey], + ["exitField", rootKey], + ["enterField", field1], + ["enterNode", 0], + ["enterField", fooKey], + ["attach", field0, 1, 0], + ["exitField", fooKey], + ["exitNode", 0], + ["exitField", field1], + ]; + const delta: DeltaRoot = { + refreshers: [{ id: refresherId, trees: [content] }], + build: [{ id: buildId, trees: [content] }], + fields: new Map([[rootKey, rootFieldDelta]]), + }; + testVisit(delta, expected, index); + }); + }); + + it("tolerates superfluous refreshers", () => { + const index = makeDetachedFieldIndex("", testRevisionTagCodec); + const node = { minor: 42 }; + const node2 = { minor: 43 }; + const rootFieldDelta: DeltaFieldChanges = { + local: [{ count: 1, attach: node2 }], + }; + const delta: DeltaRoot = { + refreshers: [ + { id: node, trees: [content] }, + { id: node2, trees: [content] }, + ], + fields: new Map([[rootKey, rootFieldDelta]]), + }; + const expected: VisitScript = [ + ["enterField", rootKey], + ["exitField", rootKey], + ["enterField", rootKey], + ["create", [content], field0], + ["attach", field0, 1, 0], + ["exitField", rootKey], + ]; + testVisit(delta, expected, index); + assert.equal(index.entries().next().done, true); + }); + describe("rename chains", () => { const pointA = { minor: 1 }; for (const cycle of [false, true]) { From f1573f03fd504f6628da0cc498daae4a4e2f6590 Mon Sep 17 00:00:00 2001 From: Jenn Date: Mon, 25 Mar 2024 11:32:17 -0700 Subject: [PATCH 3/5] Update packages/dds/tree/src/test/tree/visitDelta.spec.ts Co-authored-by: yann-achard-MS <97201204+yann-achard-MS@users.noreply.github.com> --- packages/dds/tree/src/test/tree/visitDelta.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dds/tree/src/test/tree/visitDelta.spec.ts b/packages/dds/tree/src/test/tree/visitDelta.spec.ts index 4ed2d1b47594..0ac7c318f5a5 100644 --- a/packages/dds/tree/src/test/tree/visitDelta.spec.ts +++ b/packages/dds/tree/src/test/tree/visitDelta.spec.ts @@ -991,7 +991,7 @@ describe("visitDelta", () => { ]); }); - describe.only("builds refreshers", () => { + describe("refreshers", () => { it("for restores at the root", () => { const index = makeDetachedFieldIndex("", testRevisionTagCodec); const node = { minor: 42 }; From 74e16819e3dc39fdec6a6e72282bf338e80316d8 Mon Sep 17 00:00:00 2001 From: Jenn Date: Mon, 25 Mar 2024 18:52:01 +0000 Subject: [PATCH 4/5] pr feedback --- packages/dds/tree/src/core/tree/visitDelta.ts | 13 +--- .../dds/tree/src/test/tree/visitDelta.spec.ts | 73 +++++++++++++------ 2 files changed, 52 insertions(+), 34 deletions(-) diff --git a/packages/dds/tree/src/core/tree/visitDelta.ts b/packages/dds/tree/src/core/tree/visitDelta.ts index 6f7580e6fe57..bf1e5e70e078 100644 --- a/packages/dds/tree/src/core/tree/visitDelta.ts +++ b/packages/dds/tree/src/core/tree/visitDelta.ts @@ -6,7 +6,7 @@ import { assert } from "@fluidframework/core-utils"; import { NestedMap } from "../../index.js"; -import { nestedMapFromFlatList, tryGetFromNestedMap } from "../../util/index.js"; +import { setInNestedMap, tryGetFromNestedMap } from "../../util/index.js"; import { FieldKey } from "../schema-stored/index.js"; import { ITreeCursorSynchronous } from "./cursor.js"; import * as Delta from "./delta.js"; @@ -74,15 +74,13 @@ export function visitDelta( const attachPassRoots: Map = new Map(); const rootTransfers: Delta.DetachedNodeRename[] = []; const rootDestructions: Delta.DetachedNodeDestruction[] = []; - const refresherData: [Major, Minor, ITreeCursorSynchronous][] = []; + const refreshers: NestedMap = new Map(); delta.refreshers?.forEach(({ id: { major, minor }, trees }) => { for (let i = 0; i < trees.length; i += 1) { const offsettedId = minor + i; - refresherData.push([major, offsettedId, trees[i]]); + setInNestedMap(refreshers, major, offsettedId, trees[i]); } }); - const refreshers: NestedMap = - nestedMapFromFlatList(refresherData); const detachConfig: PassConfig = { func: detachPass, refreshers, @@ -309,11 +307,6 @@ interface PassConfig { * created in the forest once needed. */ readonly refreshers: NestedMap; - /** - * Nested changes on roots that need to be visited as part of the detach pass. - * Each entry is removed when its associated changes are visited. - */ - // readonly detachPassRoots: Map; /** * Nested changes on roots that need to be visited as part of the detach pass. * Each entry is removed when its associated changes are visited. diff --git a/packages/dds/tree/src/test/tree/visitDelta.spec.ts b/packages/dds/tree/src/test/tree/visitDelta.spec.ts index 0ac7c318f5a5..05c77c6fff93 100644 --- a/packages/dds/tree/src/test/tree/visitDelta.spec.ts +++ b/packages/dds/tree/src/test/tree/visitDelta.spec.ts @@ -1113,30 +1113,55 @@ describe("visitDelta", () => { }); }); - it("tolerates superfluous refreshers", () => { - const index = makeDetachedFieldIndex("", testRevisionTagCodec); - const node = { minor: 42 }; - const node2 = { minor: 43 }; - const rootFieldDelta: DeltaFieldChanges = { - local: [{ count: 1, attach: node2 }], - }; - const delta: DeltaRoot = { - refreshers: [ - { id: node, trees: [content] }, - { id: node2, trees: [content] }, - ], - fields: new Map([[rootKey, rootFieldDelta]]), - }; - const expected: VisitScript = [ - ["enterField", rootKey], - ["exitField", rootKey], - ["enterField", rootKey], - ["create", [content], field0], - ["attach", field0, 1, 0], - ["exitField", rootKey], - ]; - testVisit(delta, expected, index); - assert.equal(index.entries().next().done, true); + describe("tolerates superfluous refreshers", () => { + it("when the delta can be applied without the refresher", () => { + const index = makeDetachedFieldIndex("", testRevisionTagCodec); + const node = { minor: 42 }; + const node2 = { minor: 43 }; + const rootFieldDelta: DeltaFieldChanges = { + local: [{ count: 1, attach: node2 }], + }; + const delta: DeltaRoot = { + refreshers: [ + { id: node, trees: [content] }, + { id: node2, trees: [content] }, + ], + fields: new Map([[rootKey, rootFieldDelta]]), + }; + const expected: VisitScript = [ + ["enterField", rootKey], + ["exitField", rootKey], + ["enterField", rootKey], + ["create", [content], field0], + ["attach", field0, 1, 0], + ["exitField", rootKey], + ]; + testVisit(delta, expected, index); + assert.equal(index.entries().next().done, true); + }); + + it("when the refreshed tree already exists in the forest", () => { + const index = makeDetachedFieldIndex("", testRevisionTagCodec); + const node = { minor: 42 }; + const rootFieldDelta: DeltaFieldChanges = { + local: [{ count: 1, attach: node }], + }; + const delta: DeltaRoot = { + build: [{ id: node, trees: [content] }], + refreshers: [{ id: node, trees: [content] }], + fields: new Map([[rootKey, rootFieldDelta]]), + }; + const expected: VisitScript = [ + ["create", [content], field0], + ["enterField", rootKey], + ["exitField", rootKey], + ["enterField", rootKey], + ["attach", field0, 1, 0], + ["exitField", rootKey], + ]; + testVisit(delta, expected, index); + assert.equal(index.entries().next().done, true); + }); }); describe("rename chains", () => { From 3ed88c361519bf767ac220b22d3cfa536dd8dc2e Mon Sep 17 00:00:00 2001 From: Jenn Date: Mon, 25 Mar 2024 19:29:47 +0000 Subject: [PATCH 5/5] another test --- .../dds/tree/src/test/tree/visitDelta.spec.ts | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/packages/dds/tree/src/test/tree/visitDelta.spec.ts b/packages/dds/tree/src/test/tree/visitDelta.spec.ts index 05c77c6fff93..68b0387de627 100644 --- a/packages/dds/tree/src/test/tree/visitDelta.spec.ts +++ b/packages/dds/tree/src/test/tree/visitDelta.spec.ts @@ -1141,6 +1141,28 @@ describe("visitDelta", () => { }); it("when the refreshed tree already exists in the forest", () => { + const index = makeDetachedFieldIndex("", testRevisionTagCodec); + const node = { minor: 42 }; + index.createEntry(node, 1); + const rootFieldDelta: DeltaFieldChanges = { + local: [{ count: 1, attach: node }], + }; + const delta: DeltaRoot = { + refreshers: [{ id: node, trees: [content] }], + fields: new Map([[rootKey, rootFieldDelta]]), + }; + const expected: VisitScript = [ + ["enterField", rootKey], + ["exitField", rootKey], + ["enterField", rootKey], + ["attach", field0, 1, 0], + ["exitField", rootKey], + ]; + testVisit(delta, expected, index); + assert.equal(index.entries().next().done, true); + }); + + it("when the refreshed tree is included in the builds", () => { const index = makeDetachedFieldIndex("", testRevisionTagCodec); const node = { minor: 42 }; const rootFieldDelta: DeltaFieldChanges = {