Ensure find and indexOf perform splay
#904
Conversation
Added splay operation after locating the node in the Find function. Simplified IndexOf() function by utilizing the splay operation, reducing the need for additional traversal logic and ensuring efficient access. By incorporating splay in both functions, the tree maintains its self-balancing property and ensures amortized O(log n) time complexity for future operations.
The benchmarks include three tests: a stress test that randomly performs insert, delete, find, and range delete operations; a skewed tree test with 1000 random accesses; and a text editing benchmark based on the editing-trace.json file.
|
|
WalkthroughThe changes introduce modifications to the Changes
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/sdk/src/util/splay_tree.ts (2 hunks)
- packages/sdk/test/bench/splay_tree.bench.ts (1 hunks)
🧰 Additional context used
🪛 Biome
packages/sdk/test/bench/splay_tree.bench.ts
[error] 7-9: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 6-6: Don't use 'Boolean' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'boolean' instead(lint/complexity/noBannedTypes)
Fixed a type error that occurred while reading the editing-trace.json file.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
packages/sdk/test/bench/splay_tree.bench.ts (2)
58-77: Enhance benchmark documentation and expectationsThe benchmark tests cover a good range of sizes for both stress testing and random access. However, there are a couple of areas where we could improve:
Documentation: There's no explanation for why these specific sizes (10000, 20000, 30000) were chosen. Adding comments to explain the rationale behind these choices would be helpful for future maintainers.
Performance Expectations: There are no baseline or expected performance metrics mentioned. Adding these would make it easier to identify performance regressions in the future.
Consider adding comments to explain the choice of sizes and include baseline performance expectations. For example:
describe('splay_tree.edit', () => { // We test with sizes 10000, 20000, and 30000 to cover small, medium, and large tree sizes // These sizes were chosen based on typical usage patterns in our application bench('splay_tree.stress 10000', () => { stressTest(10000); }, { // Add a comment explaining the expected performance // e.g., "Expected to complete in < 100ms on our CI environment" }); // ... (repeat for other benchmarks) });Additionally, consider adding a comment at the top of the file explaining the overall purpose of these benchmarks and how they relate to the splay tree implementation improvements.
1-96: Overall assessment of the splay tree benchmarksThe benchmarks implemented in this file are comprehensive and cover various aspects of the SplayTree performance, which aligns well with the PR objectives of improving the splay tree operation. However, there are a few areas where the benchmarks could be enhanced to more directly address the specific issue mentioned in the PR (ensuring splay operation after find):
- Explicit testing of splay operation after find in both
benchmarkRandomAccessandstressTestfunctions.- Improving reproducibility of the stress test by using a seeded random number generator.
- Enhancing robustness of the editing-trace benchmark by replacing non-null assertions with proper null checks.
- Adding verification of the final tree state in the editing-trace benchmark.
Additionally, there are minor improvements to be made in terms of TypeScript best practices (using
booleaninstead ofBoolean) and removing unnecessary code (theStringNodeconstructor).Overall, with these suggested improvements, the benchmarks will provide a more targeted and robust way to verify the correct implementation of the splay tree operations, especially the critical splay-after-find behavior.
Consider adding a specific benchmark that focuses solely on the splay-after-find behavior. This could involve a series of find operations followed by checks to ensure the found node is at the root of the tree. This would directly address the main objective of the PR and provide a clear way to verify that the issue has been resolved.
🧰 Tools
🪛 Biome
[error] 7-9: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 6-6: Don't use 'Boolean' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'boolean' instead(lint/complexity/noBannedTypes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/sdk/test/bench/splay_tree.bench.ts (1 hunks)
🧰 Additional context used
🪛 Biome
packages/sdk/test/bench/splay_tree.bench.ts
[error] 7-9: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 6-6: Don't use 'Boolean' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'boolean' instead(lint/complexity/noBannedTypes)
🔇 Additional comments (1)
packages/sdk/test/bench/splay_tree.bench.ts (1)
23-31: Verify if the benchmark tests splay operation after findThe
benchmarkRandomAccessfunction seems to be correctly implementing a random access benchmark. However, given the PR objectives to fix the splay tree operation, particularly ensuring that Find and IndexOf functions perform the necessary splay operation, it's not clear if this benchmark is effectively testing this aspect.Could you verify if this benchmark is sufficient to test the splay operation after find? If not, consider modifying the benchmark to explicitly test this behavior. For example, you could add assertions to check if the found node is at the root of the tree after each find operation.
| const stressTest = (size: number) => { | ||
| const tree = new SplayTree<string>(); | ||
| let treeSize = 1; | ||
| for (let i = 0; i < size; i++) { | ||
| const op = Math.floor(Math.random() * 3); | ||
| if (op == 0) { | ||
| const node = tree.find(Math.floor(Math.random() * treeSize))[0]; | ||
| if (node != undefined) { | ||
| tree.insertAfter(node, StringNode.create('A')); | ||
| } else { | ||
| tree.insert(StringNode.create('A')); | ||
| } | ||
| treeSize++; | ||
| } else if (op == 1) { | ||
| tree.find(Math.floor(Math.random() * treeSize)); | ||
| } else { | ||
| const node = tree.find(Math.floor(Math.random() * treeSize))[0]; | ||
| if (node != undefined) { | ||
| tree.delete(node); | ||
| treeSize--; | ||
| } | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance stress test for splay operation and reproducibility
The stressTest function provides a comprehensive test of the SplayTree operations. However, there are two areas where it could be improved:
-
Splay Operation Verification: Given the PR objectives, it would be beneficial to explicitly verify that the splay operation is performed after each find operation.
-
Test Reproducibility: The use of
Math.random()makes the test non-deterministic, which could lead to inconsistent results across different runs.
Consider the following improvements:
-
Add checks to verify the splay operation after each find. For example, you could check if the found node is at the root of the tree after each find operation.
-
Use a seeded random number generator to make the test deterministic and reproducible. You can use a library like
seedrandomfor this purpose.
Here's a sketch of how you might implement these changes:
import seedrandom from 'seedrandom';
const stressTest = (size: number, seed: string) => {
const rng = seedrandom(seed);
const tree = new SplayTree<string>();
let treeSize = 1;
for (let i = 0; i < size; i++) {
const op = Math.floor(rng() * 3);
if (op == 0) {
// ... (insert operation)
} else if (op == 1) {
const index = Math.floor(rng() * treeSize);
const [node] = tree.find(index);
if (node) {
// Verify splay operation
expect(tree.root).toBe(node);
}
} else {
// ... (delete operation)
}
}
};
// Usage
bench('splay_tree.stress 10000', () => {
stressTest(10000, 'fixed-seed-for-reproducibility');
});This approach will make your tests more targeted towards the PR objectives and more reliable across different runs.
Removed the redundant constructor that only called the super constructor with the same parameters. Changed the 'removed' property type from 'Boolean' to the lowercase 'boolean' type for consistency and clarity.
hackerwins
changed the title
find and indexOf perform splay
Enhance splay tree efficiency by incorporating splay operations after node location in Find and IndexOf functions. This change maintains the tree's self-balancing property and ensures amortized O(log n) time complexity for future operations. Simplify IndexOf() by leveraging the splay operation, reducing additional traversal logic and improving overall performance.
What this PR does / why we need it?
This PR addresses a critical issue in the splay tree implementation where the Find and IndexOf functions do not perform the necessary splay operation after locating a node. By ensuring that these functions splay the accessed node to the root, we maintain the amortized O(log n) time complexity for subsequent operations. This enhancement optimizes the overall efficiency and performance of the splay tree, ensuring that frequently accessed nodes remain close to the root and improving access times for future queries.
Any background context you want to provide?
Same PR as the Go repository
yorkie-team/yorkie#1017
What are the relevant tickets?
Fixes #903
Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
SplayTreefunctionality for improved node access and manipulation.SplayTreeunder various conditions, including random access and stress tests.Bug Fixes
getNodeAtandgetIndexOfNodemethods for more reliable node retrieval.