optimize splay tree #907
optimize splay tree #907
Conversation
To prevent the tree from becoming skewed, I balanced it by splaying the first node of the sequence every 500 linear insert operations. The value 500 was determined experimentally.
WalkthroughThe changes in this pull request modify the Changes
Assessment against linked issues
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
packages/sdk/src/util/splay_tree.ts (2)
173-174: Consider initializingfirstNodein the constructorWhile
firstNodeis optional, explicitly initializing it in the constructor enhances code clarity and prevents potential undefined issues.Apply this diff to initialize
firstNode:constructor(root?: SplayNode<V>) { this.root = root; this.linearCount = 0; + this.firstNode = undefined; }
263-274: Add comments to clarify the splaying logic for linear insertionsAdding descriptive comments helps others understand how
linearCountandfirstNodework together to prevent tree skewness due to sequential insertions.Consider adding comments like this:
if (target === this.root) { + // Increment linearCount when inserting after the root node this.linearCount++; if (this.linearCount == 1) { + // Store the first node of the linear insertion sequence this.firstNode = newNode; } else if (this.linearCount > 500) { + // Splay the first node to rebalance the tree after 500 insertions this.splayNode(this.firstNode); this.linearCount = 0; } } else { + // Reset linearCount if insertion is not after the root this.linearCount = 0; }
| this.splayNode(this.firstNode); | ||
| this.linearCount = 0; |
There was a problem hiding this comment.
Ensure firstNode is defined before splaying
Although firstNode should be defined when linearCount > 500, adding a safety check can prevent unexpected errors if the state changes in the future.
Apply this diff to add a null check:
} else if (this.linearCount > 500) {
+ if (this.firstNode) {
this.splayNode(this.firstNode);
+ }
this.linearCount = 0;Committable suggestion was skipped due to low confidence.
packages/sdk/src/util/splay_tree.ts
Outdated
| if (target == this.root) { | ||
| this.linearCount++; | ||
| if (this.linearCount==1) { | ||
| this.firstNode = newNode; | ||
| } else if (this.linearCount > 500) { | ||
| this.splayNode(this.firstNode); | ||
| this.linearCount = 0; | ||
| } | ||
| } else { | ||
| this.linearCount = 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the splay threshold configurable
Hardcoding the value 500 limits flexibility. Defining it as a constant or making it configurable allows for easier tuning based on different use cases.
Define a constant at the top of the class:
export class SplayTree<V> {
private root?: SplayNode<V>;
+ private static readonly SPLAY_THRESHOLD = 500;
private linearCount: number;
private firstNode?: SplayNode<V>;Update the code to use the constant:
- } else if (this.linearCount > 500) {
+ } else if (this.linearCount > SplayTree.SPLAY_THRESHOLD) {Committable suggestion was skipped due to low confidence.
packages/sdk/src/util/splay_tree.ts
Outdated
| if (target == this.root) { | ||
| this.linearCount++; | ||
| if (this.linearCount==1) { | ||
| this.firstNode = newNode; | ||
| } else if (this.linearCount > 500) { | ||
| this.splayNode(this.firstNode); | ||
| this.linearCount = 0; | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Use strict equality === instead of ==
Using strict equality ensures both value and type are compared, preventing potential bugs with type coercion.
Apply this diff to use strict equality:
-if (target == this.root) {
+if (target === this.root) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (target == this.root) { | |
| this.linearCount++; | |
| if (this.linearCount==1) { | |
| this.firstNode = newNode; | |
| } else if (this.linearCount > 500) { | |
| this.splayNode(this.firstNode); | |
| this.linearCount = 0; | |
| } | |
| } else { | |
| if (target === this.root) { | |
| this.linearCount++; | |
| if (this.linearCount==1) { | |
| this.firstNode = newNode; | |
| } else if (this.linearCount > 500) { | |
| this.splayNode(this.firstNode); | |
| this.linearCount = 0; | |
| } | |
| } else { |
Move hardcoded constant to readonly attribute in Splay Tree
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/sdk/src/util/splay_tree.ts (1)
173-175: LGTM! Consider making SPLAY_THRESHOLD configurable.The new properties align well with the PR objectives for optimizing the splay tree performance. The static
SPLAY_THRESHOLDeffectively implements the proposed solution of splaying every 500 linear insert operations.To enhance flexibility, consider making
SPLAY_THRESHOLDconfigurable:- private static readonly SPLAY_THRESHOLD = 500; + private static SPLAY_THRESHOLD = 500; + + public static setSplayThreshold(threshold: number): void { + if (threshold > 0) { + SplayTree.SPLAY_THRESHOLD = threshold; + } + }This allows for easier tuning based on different use cases or performance requirements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/sdk/src/util/splay_tree.ts (4 hunks)
🔇 Additional comments (2)
packages/sdk/src/util/splay_tree.ts (2)
179-179: LGTM! Proper initialization of linearCount.Initializing
linearCountto 0 in the constructor is correct and aligns with the new balancing strategy. This ensures that each new SplayTree instance starts with a fresh count for linear insertions.
218-218: Consider the impact of splaying after find operations.While splaying after find operations can improve amortized performance in typical splay trees, it might interfere with the new balancing strategy for linear insertions. Splaying nodes found by the
findmethod could potentially break linear sequences that the new strategy is trying to manage.To assess the impact, let's analyze the usage of the
findmethod:Consider whether splaying after find operations is necessary given the new balancing strategy, and evaluate its potential impact on the overall performance objectives of this PR.
| if (target == this.root) { | ||
| this.linearCount++; | ||
| if (this.linearCount == 1) { | ||
| this.firstNode = newNode; | ||
| } else if (this.linearCount > SplayTree.SPLAY_THRESHOLD) { | ||
| this.splayNode(this.firstNode); | ||
| this.linearCount = 0; | ||
| } | ||
| } else { | ||
| this.linearCount = 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
Implement the new balancing strategy with some improvements.
The new logic effectively implements the proposed balancing strategy for linear insertions, aligning well with the PR objectives. However, there are a few points to address:
- Use strict equality (
===) instead of loose equality (==) for comparisons. - Add a null check before splaying
firstNode. - Consider resetting
firstNodeafter splaying.
Apply this diff to address the points mentioned:
- if (target == this.root) {
+ if (target === this.root) {
this.linearCount++;
- if (this.linearCount == 1) {
+ if (this.linearCount === 1) {
this.firstNode = newNode;
} else if (this.linearCount > SplayTree.SPLAY_THRESHOLD) {
- this.splayNode(this.firstNode);
+ if (this.firstNode) {
+ this.splayNode(this.firstNode);
+ this.firstNode = undefined;
+ }
this.linearCount = 0;
}
} else {
this.linearCount = 0;
+ this.firstNode = undefined;
}These changes improve type safety, prevent potential null pointer exceptions, and ensure proper reset of the firstNode reference.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (target == this.root) { | |
| this.linearCount++; | |
| if (this.linearCount == 1) { | |
| this.firstNode = newNode; | |
| } else if (this.linearCount > SplayTree.SPLAY_THRESHOLD) { | |
| this.splayNode(this.firstNode); | |
| this.linearCount = 0; | |
| } | |
| } else { | |
| this.linearCount = 0; | |
| } | |
| if (target === this.root) { | |
| this.linearCount++; | |
| if (this.linearCount === 1) { | |
| this.firstNode = newNode; | |
| } else if (this.linearCount > SplayTree.SPLAY_THRESHOLD) { | |
| if (this.firstNode) { | |
| this.splayNode(this.firstNode); | |
| this.firstNode = undefined; | |
| } | |
| this.linearCount = 0; | |
| } | |
| } else { | |
| this.linearCount = 0; | |
| this.firstNode = undefined; | |
| } |
To prevent the tree from becoming skewed, I balanced it by splaying the first node of the sequence every 500 linear insert operations. The value 500 was determined experimentally.
What this PR does / why we need it:
Previous
This is the result of benchmarking in the current implementation.
previous pr(max height splay)
In previous PR, we chose to balance the deepest node by splaying it whenever the tree was heightened, but the optimization was not good.
The cost of reproducing the height and the overhead of checking the height for each operation and performing the splay operation seem to be larger than expected.
By triggering balancing based on height, the splay operation seems to occur more often than is required for balancing, resulting in performance degradation.
In this PR
block continuous linear opeartion
The main idea is that the cause of the skew tree is a linear insert operation, so if such an operation is repeatedly entered, balancing is performed.
In insertAfter, the count is increased when the previous node is root, and the first node of the linear operation is splayed when the count is n or more.
Below is the result of experimenting with$n$ , and the performance was the best when $n$ was 500.
go test
C++ benchmark (based on a modified version of @justiceHui's code):
Data used: editing-trace.json.
Experimental code is available here.
Which issue(s) this PR fixes:
Fixes #906
Special notes for your reviewer:
same pr as yorkie-team/yorkie#1025
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit
Summary by CodeRabbit