Unify MergeTree Walks into Single Interative Function #11603
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
anthony-murphy
commented
Aug 19, 2022
| @@ -837,7 +844,7 @@ export class MergeTree { | |||
| offset = start; | |||
| return false; | |||
| }; | |||
| this.searchBlock(this.root, pos, 0, refSeq, clientId, { leaf }, undefined, localSeq); | |||
| this.nodeMap(refSeq, clientId, leaf, undefined, undefined, pos, pos + 1, localSeq); | |||
There was a problem hiding this comment.
search block was the wrong method here, as it stops at every segment, whereas nodemap uses the tree to optimally find the position.
There was a problem hiding this comment.
What consequences did that have? perf?
There was a problem hiding this comment.
walking the tree with node walk is O(log N), whereas walking all segments with search will be O(N), so yeah it was perf
github-actions
bot
added
area: dds
anthony-murphy
commented
Aug 19, 2022
| post?: BlockAction<TClientData>, | ||
| start: number = 0, | ||
| end?: number, | ||
| localSeq?: number, |
There was a problem hiding this comment.
bonus. adding local seq here was cheap, which helps get containing segment, which helps for rebase, rollback, re-connect etc
…to unify-some-merge-tree-walks
github-actions
bot
added
area: contributor experience
area: dds: sharedstring
public api change
anthony-murphy
commented
Aug 20, 2022
|
Why merge into next rather than main? |
This was referenced Nov 5, 2022
This was referenced Nov 5, 2022
This was referenced Nov 5, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
In merge tree there were multiple near duplicate methods for walking the tree. Additionally, all these methods were recursive. This change unifies the tree walks into a single method, that is iterative rather than recursive. The primary motivation for this change in related to #1009 where it is necessary to do tree walks to shift local references for each undo/redo, as the tree grew these walks grew deeper, and the repeated stack allocations of the recursive calls lead to performance issues. In addition, removing all the duplicated variations will make maintenance easier, and should reduce bundle size.