Apply max height splay method #989
Conversation
To prevent the tree from becoming skewed, we applied a heuristic cal- led the 'max height splay' method. When the size of the tree is n, and its height exceeds 50*(log n), the deepest node is selected and splayed to the root. To avoid excessive balancing when the tree has few nodes, the balancing process is skipped if the number of nodes is 1000 or fewer. Since the 'max height splay' method requires knowledge of the tree's height, we added a height attribute to the nodes of the splay tree and included code to check the height during testing.
WalkthroughThe changes involve updates to the Changes
Possibly related issues
Possibly related PRs
Suggested labels
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: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- pkg/document/document_test.go (1 hunks)
- pkg/splay/splay.go (14 hunks)
- pkg/splay/splay_test.go (5 hunks)
- test/bench/document_bench_test.go (1 hunks)
Additional comments not posted (23)
pkg/splay/splay_test.go (4)
59-59: Expected output format ofToTestString()has changed.The expected output of
tree.ToTestString()now includes an additional numerical value, likely representing the height or depth of the node in the tree.
61-61: Consistent change in the expected output format ofToTestString().The expected outputs of
tree.ToTestString()consistently include an additional numerical value for each node, likely representing the height or depth of the node in the tree.Also applies to: 63-63, 65-65
68-68: Consistent change in the expected output format ofToTestString()after splaying.The expected output of
tree.ToTestString()includes the additional numerical value for each node, even after splayingnodeB, which is consistent with the previous observations.
95-95: Consistent change in the expected output format ofToTestString()after various operations.The expected outputs of
tree.ToTestString()consistently include the additional numerical value for each node, even after various insertion and deletion operations, which is consistent with the previous observations.Also applies to: 98-98, 101-101, 104-104, 108-108
test/bench/document_bench_test.go (3)
145-145: Verify if the changes to the expected output ofToTestString()are intentional and correct.The expected output strings for the array's
ToTestString()method have been updated to include additional elements and indices. Please ensure that these changes align with the intended behavior and the updates made to theToTestString()method.To verify the correctness of the changes, please provide more context on the updates made to the
ToTestString()method and confirm if the new expected output strings match the desired behavior. If possible, share the relevant code changes made to theToTestString()method.Also applies to: 150-150, 155-155, 160-160
145-160: Skipped reviewThe
benchmarkTreeDeleteAllfunction was not part of the changed code segment. Skipping the review for this function.
145-160: Skipped reviewThe
benchmarkTreeEditGCfunction was not part of the changed code segment. Skipping the review for this function.pkg/splay/splay.go (12)
39-39: LGTM!The addition of the
heightfield to theNodestruct is approved. It enables tracking the height of each node, which is essential for maintaining the balance of the tree.
49-50: LGTM!Initializing the
heightfield to 1 in theNewNodefunction is approved. It correctly sets the height of a new node to 1.
75-80: LGTM!The implementation of the
leftHeightmethod is approved. It correctly retrieves the height of the left child node, returning 0 if the left child is nil.
82-87: LGTM!The implementation of the
rightHeightmethod is approved. It correctly retrieves the height of the right child node, returning 0 if the right child is nil.
111-112: LGTM!The addition of the
nodeCountfield to theTreestruct is approved. It enables tracking the total number of nodes in the tree, which is essential for the new balancing mechanism.
118-119: LGTM!Initializing the
nodeCountfield to 0 in theNewTreefunction is approved. It correctly sets the node count of a new tree to 0.
Line range hint
135-150: LGTM!The changes to the
InsertAftermethod are approved:
- Calling the
BalancingTreeHeightmethod before splaying ensures that the tree remains balanced after inserting a new node.- Updating both the height and weight of nodes using the
UpdateNodemethod maintains the consistency of node properties.- Incrementing the
nodeCountafter inserting a new node keeps track of the total number of nodes in the tree.
152-156: LGTM!The implementation of the
UpdateNodemethod is approved. It correctly recalculates the height and weight of a node with the value and children, streamlining the maintenance of node properties.
158-168: LGTM!The implementation of the
MaxHeightSplaymethod is approved. It correctly finds a deepest node by traversing down the tree based on the height of child nodes and splays the found node to the root, helping to restructure the tree and promote balance.
226-226: LGTM!Calling the
BalancingTreeHeightmethod before finding the index of a node in theIndexOfmethod is approved. It ensures that the tree remains balanced during the operation.
246-246: LGTM!Calling the
BalancingTreeHeightmethod before finding a node at a given index in theFindmethod is approved. It ensures that the tree remains balanced during the operation.
Line range hint
348-377: LGTM!The changes to the
Deletemethod are approved:
- Calling the
BalancingTreeHeightmethod before deleting a node ensures that the tree remains balanced during the operation.- Updating both the height and weight of the root node using the
UpdateNodemethod after deletion maintains the consistency of node properties.- Decrementing the
nodeCountafter deleting a node keeps track of the total number of nodes in the tree.pkg/document/document_test.go (4)
135-135: LGTM!The update to the expected output string in the assertion for
root.GetArray("k1").ToTestString()is approved. It reflects the inclusion of height information in the array's string representation, aligning with the modifications made to the internal structure of the array.
140-140: LGTM!The update to the expected output string in the assertion for
root.GetArray("k1").ToTestString()after deleting an element is approved. It reflects the inclusion of height information in the array's string representation, aligning with the modifications made to the internal structure of the array.
145-145: LGTM!The update to the expected output string in the assertion for
root.GetArray("k1").ToTestString()after inserting an element is approved. It reflects the inclusion of height information in the array's string representation, aligning with the modifications made to the internal structure of the array.
150-150: LGTM!The update to the expected output string in the assertion for
root.GetArray("k1").ToTestString()after inserting another element is approved. It reflects the inclusion of height information in the array's string representation, aligning with the modifications made to the internal structure of the array.
|
|
…#983) Added functionality allowing users to delete accounts and change passwords through the CLI to support recent development of admin ChangePassword and DeleteAccount APIs on the server side.
|
Do you mean #957? |
hackerwins
requested review from
hackerwins
and removed request for
raararaara
This reverts commit e6eb7fc.
Add benchmark tests to see how much performance optimization improves the splay tree.
Fixed an issue where the delete operation attempted to remove nodes outside the valid range. The index range is now progressively narrowed during the operation.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
admin/client.go (2)
381-392: LGTM! Ensure proper checks and cleanup.The
DeleteAccountfunction looks good and follows the existing code style. However, please ensure the following:
- Proper authentication and authorization checks are in place before allowing account deletion.
- Cascading effects of account deletion are handled, such as cleaning up associated data or resources.
394-406: LGTM! Ensure password validation and secure storage.The
ChangePasswordfunction looks good and follows the existing code style. However, please ensure the following:
- Proper validation is in place for the new password, such as checking for minimum length, complexity, etc.
- The old password is correctly verified before allowing the change.
- Secure practices are followed, such as hashing and salting passwords before storage.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (9)
- admin/client.go (1 hunks)
- cmd/yorkie/commands.go (2 hunks)
- cmd/yorkie/user/change_password.go (1 hunks)
- cmd/yorkie/user/delete_account.go (1 hunks)
- cmd/yorkie/user/login.go (4 hunks)
- cmd/yorkie/user/logout.go (2 hunks)
- cmd/yorkie/user/user.go (1 hunks)
- go.mod (2 hunks)
- test/bench/tree_editing_bench_test.go (1 hunks)
Additional comments not posted (32)
cmd/yorkie/user/user.go (4)
1-15: LGTM!The copyright notice and license information are properly formatted and attributed. The Apache License 2.0 is a widely used open-source license that provides clear terms and conditions for using, modifying, and distributing the code.
17-18: LGTM!The package declaration and the accompanying comment are clear and descriptive. The package name "user" aligns with the functionality it provides, and the comment succinctly explains the purpose of the package.
20-20: LGTM!The import statement for the Cobra library is properly formatted and aligns with the package's purpose of providing a CLI command. Cobra is a widely used and well-maintained library for building CLI applications in Go, making it a suitable choice for this package.
22-28: LGTM!The
SubCmdvariable declaration is properly formatted and follows Go's naming conventions. The usage string and short description provide clear information about the command's purpose and how to invoke it. This variable serves as the foundation for the user command and can be further extended with additional subcommands and functionalities related to user account management.cmd/yorkie/user/logout.go (2)
17-17: Package change looks good.The package has been changed from
maintouser, which aligns with the goal of better encapsulating user-related functionalities within the codebase. This change should improve code organization and clarity.
71-71: Verify the impact of the command registration change.The command registration has been altered from adding the command to
rootCmdto adding it toSubCmd. While this change suggests a reorganization of command hierarchies, it's important to ensure that thelogoutcommand is still accessible and functioning as expected from the CLI.Run the following script to verify the command registration change:
Verification successful
Verification of command registration change confirmed
The command registration change has been correctly implemented. The
logoutcommand is now registered under theusersubcommand (SubCmd), which is then added to the root command. This change is consistent with other user-related commands and follows a clear pattern for better organization of the CLI structure.Key points:
- The
logoutcommand is added toSubCmdin theinit()function ofcmd/yorkie/user/logout.go.- Other user-related commands (login, delete_account, change_password) follow the same pattern.
- The
user.SubCmdis added torootCmdincmd/yorkie/commands.go.This new structure allows for a more organized and hierarchical command layout, improving the overall user experience of the CLI.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `logout` command is accessible and functioning as expected from the CLI. # Test 1: Check if the `logout` command is accessible under the `user` subcommand. # Expect: The `logout` command should be listed in the help output. yorkie user --help # Test 2: Attempt to log out using the `logout` command. # Expect: The command should execute successfully and log out the user. yorkie user logout # Test 3: Verify that the user is logged out by attempting to perform an operation that requires authentication. # Expect: The operation should fail due to the user being logged out. yorkie document listLength of output: 288
Script:
#!/bin/bash # Find relevant files in the cmd directory echo "Relevant files in cmd directory:" fd . cmd # Search for SubCmd definition echo -e "\nSubCmd definition:" ast-grep --lang go --pattern 'var SubCmd = &cobra.Command{$$$}' # Search for commands registered under SubCmd echo -e "\nCommands registered under SubCmd:" ast-grep --lang go --pattern 'SubCmd.AddCommand($_)' # Search for any remaining references to rootCmd related to logout echo -e "\nReferences to rootCmd related to logout:" rg 'rootCmd.*logout' cmdLength of output: 1171
Script:
#!/bin/bash # Search for Command definitions in the user package echo "Command definitions in user package:" ast-grep --lang go --pattern 'var $_ = &cobra.Command{$$$}' cmd/yorkie/user/ # Look for AddCommand calls in the user package echo -e "\nAddCommand calls in user package:" ast-grep --lang go --pattern '$_.AddCommand($_)' cmd/yorkie/user/ # Check the content of cmd/yorkie/user/user.go echo -e "\nContent of cmd/yorkie/user/user.go:" cat cmd/yorkie/user/user.go # Search for cobra.Command usage in logout.go echo -e "\ncobra.Command usage in logout.go:" ast-grep --lang go --pattern 'cobra.Command{$$$}' cmd/yorkie/user/logout.goLength of output: 2879
Script:
#!/bin/bash # Search for imports of the "cmd/yorkie/user" package echo "Imports of cmd/yorkie/user package:" rg 'import.*".*cmd/yorkie/user"' cmd # Look for functions that might be initializing commands echo -e "\nFunctions initializing commands:" ast-grep --lang go --pattern 'func $_() { $$$ SubCmd.AddCommand($_) $$$ }' # Check for AddCommand calls related to user or logout commands echo -e "\nAddCommand calls related to user or logout:" rg 'AddCommand.*user|AddCommand.*logout' cmdLength of output: 8120
cmd/yorkie/commands.go (2)
30-30: LGTM!The import statement for the
userpackage is syntactically correct and follows the expected import path pattern for the Yorkie project.Based on the AI-generated summary, this import is part of the effort to introduce user management functionality to the Yorkie command-line interface. The
userpackage is expected to contain the necessary code for implementing user-related operations.
53-53: Looks good!The addition of
user.SubCmdto therootCmdis syntactically correct and follows the established pattern for registering subcommands in the Yorkie command-line interface.According to the AI-generated summary, this change is part of the effort to introduce user management functionality to the Yorkie CLI. By adding the
usersubcommand, users will be able to perform operations related to user management directly from the command line, enhancing the overall capabilities and usability of the Yorkie tool.cmd/yorkie/user/login.go (5)
17-18: LGTM!The package name change from
maintouseris appropriate, as it aligns with the purpose of the file to provide user-related commands.
22-23: LGTM!The
fmtandospackage imports are necessary for the new password input functionality, such as printing messages and reading from the standard input.
26-26: LGTM!The
golang.org/x/termpackage import is necessary for the new password input functionality, as it provides functions for reading password input securely without echoing the characters.
45-52: LGTM!The code enhances security by prompting the user for a password and reading it securely using
term.ReadPasswordfunction. The password is read from the standard input, and error handling is performed to ensure successful password reading. The password is then stored in thepasswordvariable for further use.
Line range hint
96-111: LGTM!Marking the
usernameflag as required ensures that the user provides a username when running the command, which aligns with the updated login functionality. Adding the command toSubCmdinstead ofrootCmdindicates a change in the command structure within the application, which is appropriate for the user-related commands.cmd/yorkie/user/change_password.go (4)
36-71: LGTM!The
changePasswordCmdfunction is well-structured and follows the Cobra command pattern. It properly handles errors and cleans up resources usingdefer. The main logic for changing the password is encapsulated in theRunEfunction, which retrieves passwords, establishes a connection to the admin service, changes the password, and deletes the authentication session.
73-91: LGTM!The
getPasswordsfunction correctly prompts the user to enter their current and new passwords securely usingterm.ReadPassword. It handles errors appropriately and returns the passwords as strings, which is suitable for the use case.
93-105: LGTM!The
deleteAuthSessionfunction correctly deletes the authentication session associated with the givenrpcAddr. It loads the configuration, deletes the authentication entry, and saves the updated configuration. Error handling is performed appropriately for both loading and saving the configuration.
107-130: LGTM!The
initfunction correctly initializes thechange-passwordcommand and sets up its flags. It creates the command usingchangePasswordCmd(), sets up flags for username, RPC address, and insecure connection option. The username flag is appropriately marked as required. The command is properly added to theSubCmdusingSubCmd.AddCommand().go.mod (3)
30-30: Verify compatibility and update YAML handling code.The addition of
gopkg.in/yaml.v3at versionv3.0.1is a significant change that could enhance YAML handling capabilities within the project. Please ensure that:
- The new dependency is compatible with the existing codebase and doesn't introduce any breaking changes.
- The relevant code that handles YAML is updated to leverage the new features or APIs provided by this dependency.
76-76: Thoroughly test the codebase with the updated dependency.The update of
golang.org/x/sysfromv0.15.0tov0.23.0could bring improvements in system call handling or compatibility with newer Go features. However, it's essential to thoroughly test the codebase to ensure that the update doesn't break any existing functionality.Please perform comprehensive testing and verify that all system call-related operations work as expected with the updated version.
77-77: Verify compatibility and update terminal handling code.The addition of
golang.org/x/termat versionv0.23.0could provide additional terminal handling functionalities. Please ensure that:
- The new dependency is compatible with the existing codebase and doesn't introduce any breaking changes.
- The relevant code that interacts with terminals is updated to leverage the new features or APIs provided by this dependency, such as reading password input, controlling terminal modes, etc.
test/bench/tree_editing_bench_test.go (7)
36-39: LGTM!The
stringValuestruct is well-defined with appropriate fields and access modifiers for internal usage within the package.
41-45: LGTM!The
newSplayNodefunction is a helpful utility for creating new splay tree nodes with the given string content. It encapsulates the node creation logic and improves code readability.
47-52: LGTM!The
Lenmethod correctly returns the length of the string content stored in thestringValueinstance, considering theremovedflag. It provides a way to determine the length of the value stored in the splay tree node.
54-56: LGTM!The
Stringmethod correctly returns the string representation of thestringValueinstance by returning thecontentfield. It satisfies thefmt.Stringerinterface, enabling the instance to be printed or formatted as a string.
58-64: LGTM!The
buildSplayTreefunction correctly constructs a splay tree with the specified number of nodes. It creates a new empty tree and iteratively inserts nodes with the string "a" using thenewSplayNodefunction. The function provides a convenient way to build a splay tree for testing purposes.
66-101: LGTM!The
BenchmarkSplayTreefunction provides comprehensive benchmarks for evaluating the performance of insertion, random reads, and deletions on the splay tree. It defines sub-benchmarks for different tree sizes and measures the time taken for each operation using thetesting.Bframework. The benchmarks cover the critical operations and provide valuable insights into the performance characteristics of the splay tree implementation.
102-129: LGTM!The benchmark function for editing the splay tree based on an editing trace is well-implemented. It reads the editing trace from a file, creates a new empty splay tree, and performs insertions or deletions based on the edit mode. The benchmark provides a realistic scenario for evaluating the performance of the splay tree under dynamic editing conditions. It measures the time taken to perform the edits, giving valuable insights into the efficiency of the splay tree implementation in handling real-world editing scenarios.
cmd/yorkie/user/delete_account.go (5)
33-70: LGTM!The
deleteAccountCmdfunction is well-structured and handles the account deletion flow correctly. It prompts the user for a password and confirmation, loads the configuration, and attempts to delete the account from the server. The function provides appropriate feedback to the user based on the success or failure of the account deletion.
72-82: LGTM!The
getPasswordfunction correctly prompts the user for a password and returns it as a string. It handles errors from theterm.ReadPasswordfunction and converts the password from a byte slice to a string before returning it.
84-100: LGTM!The
makeConfirmationfunction correctly prompts the user to type 'DELETE' to confirm the account deletion. It handles errors from thefmt.Scanlnfunction and returnsfalseif the user enters anything other than 'DELETE'.
102-132: LGTM!The
deleteAccountFromServerfunction correctly attempts to delete the user account from the server. It dials the admin client using the provided RPC address and authentication token, sets a timeout of 30 seconds for the account deletion operation, and calls theDeleteAccountmethod on the admin client with the provided username and password. The function handles errors from theDeleteAccountmethod and updates the configuration to remove the account details and save the changes.
134-157: LGTM!The
initfunction correctly initializes thedelete-accountcommand and adds it to theSubCmdcommand. It adds the requiredusernameflag and optionalrpc-addrandinsecureflags to the command, marks theusernameflag as required, and adds thedelete-accountcommand to theSubCmdcommand.
|
Adding both weight and height to each node increases memory usage, and the cost of re-computation becomes higher. Additionally, when splaying based on height, it seems that more splay operations are performed than anticipated. Therefore, @m4ushold and I have decided to close this PR for now. |
What this PR does / why we need it:
This PR aims to address the performance and efficiency degradation in the crdt.Text data structure caused by skewness in the Splay tree. Splay trees, while efficient for operations over continuous ranges, have the downside that consecutively inserted elements may become linearly arranged, leading to a skewed tree.
When performing M operations on a tree with N nodes, the overall performance is generally guaranteed to be O((N+M) log N). However, if the tree becomes skewed, each operation might initially take O(N) time, though this could eventually improve. Such skewness can degrade the performance of the crdt.Text data structure, making it critical to explore ways to prevent this skewness and maintain the tree's efficiency.
This PR introduces a new method called MaxHeightSplay, which mitigates the skewing of the tree by controlling the upper bound of the tree height while preserving the characteristics of the Splay tree. MaxHeightSplay works by identifying the deepest node and performing a splay operation to reduce the tree's height.
During each operation (insert, delete, find, etc.) on the Splay tree, if the number of nodes n exceeds 1000 and the tree height exceeds 50*log n, MaxHeightSplay is invoked. Although this is a simple heuristic, experimental results show that during 260,000 operations, the total number of rotations performed (sum.rot) increased by 1.150 times, while the maximum number of rotations performed during any individual operation (mx.rot) decreased by 0.151 times. The experimental code can be found here.
Moreover, it is believed that no further performance improvements are necessary; attempting more aggressive balancing could compromise the inherent characteristics of the Splay tree.
In the previous PR, calculating the balance on every splay operation led to issues such as stack overflow. In this PR, the issue has been resolved by invoking MaxHeightSplay during each operation on the Splay tree.
Which issue(s) this PR fixes:
Address #941
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests