-
Notifications
You must be signed in to change notification settings - Fork 539
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.
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
Refactor(tree): Use TreeChunk in more places #24001
base: main
Are you sure you want to change the base?
Changes from all commits
d8ca2b8
4dde583
b6fcd2b
e1179d6
00a0f10
333e75f
05bb646
baa58ad
4594c06
e9f4bd6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,12 +8,8 @@ import { assert } from "@fluidframework/core-utils/internal"; | |
import { type NestedMap, setInNestedMap, tryGetFromNestedMap } from "../../util/index.js"; | ||
import type { FieldKey } from "../schema-stored/index.js"; | ||
|
||
import type { ITreeCursorSynchronous } from "./cursor.js"; | ||
// eslint-disable-next-line import/no-duplicates | ||
import { mapCursorField, type ITreeCursorSynchronous } from "./cursor.js"; | ||
import type * as Delta from "./delta.js"; | ||
// Since ProtoNodes is reexported, import it directly to avoid forcing Delta to be reexported. | ||
// eslint-disable-next-line import/no-duplicates | ||
import type { ProtoNodes } from "./delta.js"; | ||
import { | ||
areDetachedNodeIdsEqual, | ||
isAttachMark, | ||
|
@@ -24,7 +20,7 @@ import { | |
import type { DetachedFieldIndex } from "./detachedFieldIndex.js"; | ||
import type { ForestRootId, Major, Minor } from "./detachedFieldIndexTypes.js"; | ||
import type { NodeIndex, PlaceIndex, Range } from "./pathTree.js"; | ||
import type { RevisionTag } from "../index.js"; | ||
import type { RevisionTag, TreeChunk } from "../index.js"; | ||
|
||
/** | ||
* Implementation notes: | ||
|
@@ -59,7 +55,7 @@ import type { RevisionTag } from "../index.js"; | |
* This needs to happen last to allow modifications to detached roots to be applied before they are destroyed. | ||
* | ||
* The details of the delta visit algorithm can impact how/when events are emitted by the objects that own the visitors. | ||
* For example, as of 2024-03-27, the subtreecChanged event of an AnchorNode is emitted when exiting a node during a | ||
* For example, as of 2024-03-27, the subtreeChanged event of an AnchorNode is emitted when exiting a node during a | ||
* delta visit, and thus the two-pass nature of the algorithm means the event fires twice for any given change. | ||
* This two-pass nature also means that the event may fire at a time where no change is visible in the tree. E.g., | ||
* if a node is being replaced, when the event fires during the detach pass no change in the tree has happened so the | ||
|
@@ -89,9 +85,10 @@ export function visitDelta( | |
const rootDestructions: Delta.DetachedNodeDestruction[] = []; | ||
const refreshers: NestedMap<Major, Minor, ITreeCursorSynchronous> = new Map(); | ||
delta.refreshers?.forEach(({ id: { major, minor }, trees }) => { | ||
for (let i = 0; i < trees.length; i += 1) { | ||
const treeCursors = nodeCursorsFromChunk(trees); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think either here or on nodeCursorsFromChunk there should probably be a TODO that we should not split up sequences into individual nodes during insert and instead store them as a sequence. |
||
for (let i = 0; i < trees.topLevelLength; i += 1) { | ||
const offsettedId = minor + i; | ||
setInNestedMap(refreshers, major, offsettedId, trees[i]); | ||
setInNestedMap(refreshers, major, offsettedId, treeCursors[i]); | ||
} | ||
}); | ||
const detachConfig: PassConfig = { | ||
|
@@ -266,7 +263,7 @@ export interface DeltaVisitor { | |
* @param destination - The key for a new detached field. | ||
* A field with this key must not already exist. | ||
*/ | ||
create(content: ProtoNodes, destination: FieldKey): void; | ||
create(content: readonly ITreeCursorSynchronous[], destination: FieldKey): void; | ||
/** | ||
* Recursively destroys the given detached field and all of the nodes within it. | ||
* @param detachedField - The key for the detached field to destroy. | ||
|
@@ -476,7 +473,13 @@ function processBuilds( | |
): void { | ||
if (builds !== undefined) { | ||
for (const { id, trees } of builds) { | ||
buildTrees(id, trees, config.detachedFieldIndex, config.latestRevision, visitor); | ||
buildTrees( | ||
id, | ||
nodeCursorsFromChunk(trees), | ||
config.detachedFieldIndex, | ||
config.latestRevision, | ||
visitor, | ||
); | ||
} | ||
} | ||
} | ||
|
@@ -592,3 +595,12 @@ function attachPass( | |
} | ||
} | ||
} | ||
|
||
/** | ||
* Converts a chunk of trees into an array of cursors. | ||
* | ||
* TODO: Update the visitDelta logic and downstream APIs to avoid splitting up sequences into individual nodes. | ||
*/ | ||
function nodeCursorsFromChunk(trees: TreeChunk): ITreeCursorSynchronous[] { | ||
return mapCursorField(trees.cursor(), (c) => c.fork()); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -174,6 +174,9 @@ export function chunkField( | |
policy: ChunkCompressor, | ||
): TreeChunk[] { | ||
const length = cursor.getFieldLength(); | ||
if (length === 0) { | ||
return []; | ||
} | ||
Comment on lines
+177
to
+179
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is interesting. chunkRange would actually handle this case correctly if the assert in it were removed. Changing the assert in chunkRange to be: seems like a better fix to me. It would be good to at least add a regression test for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The assert triggers when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I got that assert slightly wrong. Working around JS not having XOR by using ===, I need to negate one of the sides.... I made #24009 which cleans this up based on your changes and test. |
||
const started = cursor.firstNode(); | ||
assert(started, 0x57c /* field to chunk should have at least one node */); | ||
return chunkRange(cursor, policy, length, false); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.