Finish basic testing; Move on to stress testing small delta #38
Conversation
- Will wrap the each of the data points in an optional. - Make MIN_SHORT_DEL as proxy for "unfilled" short delta [can do later too]
- We can always add 1 more bit to check if the first member of chunk is occupied, rest can be checked by assigning MIN_SHORT_DEL as special for not occupied and 0 for occupied but unused pointer. [We can discuss later]
WalkthroughThe recent updates focus on enhancing a tree data structure's functionality and testability. Key developments include refined tree operations for node management, the introduction of optional data storage, migration to Bazel's Bzlmod for dependencies, and targeted adjustments to build and test configurations. Collectively, these changes aim to improve the robustness, efficiency, and maintenance of the tree data structure while also streamlining dependency management. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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
Outside diff range and nitpick comments (1)
hhds/tree.hpp (1)
Line range hint
13-56: Thetreeclass intree.hppis well-structured with comprehensive methods for tree manipulation. However, consider optimizing the handling of sibling navigation to reduce potential performance bottlenecks, especially in large trees.- // Current implementation + // Proposed optimized implementationAlso applies to: 57-791
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- hhds/testing.cpp (1 hunks)
- hhds/tree.cpp (1 hunks)
- hhds/tree.hpp (15 hunks)
Additional comments not posted (2)
hhds/tree.cpp (1)
4-50: Themainfunction effectively demonstrates various tree operations. Ensure the output statements are clear and consistent for better readability.hhds/testing.cpp (1)
6-79: Themainfunction intesting.cppis structured to test tree operations under more extensive conditions, including loops for adding multiple children. Consider adding more detailed error messages in the exception handling to aid debugging.
- Note: I have commented out the compilation of `*graph*.{h/c}cpp` since the files seem to be incomplete.
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
MODULE.bazel.lockis excluded by!**/*.lock
Files selected for processing (5)
- MODULE.bazel (1 hunks)
- hhds/BUILD (2 hunks)
- hhds/lhtree.hpp (1 hunks)
- hhds/tests/tree_test.cpp (1 hunks)
- hhds/tree.cpp (1 hunks)
Files not summarized due to errors (1)
- hhds/lhtree.hpp: Error: Message exceeds token limit
Files skipped from review due to trivial changes (1)
- MODULE.bazel
Additional comments not posted (4)
hhds/tests/tree_test.cpp (2)
7-9: Simplify the main function.The
mainfunction is currently a stub and returns 0. This is typical for a placeholder or a test setup file. Ensure that this function is either properly implemented or marked clearly as a placeholder if further development is intended.
1-5: Review the necessity and correctness of include statements.The file includes several headers which seem necessary for the operations described in the PR summary. However, ensure that all included files (
benchmark.h,tree.hpp,lhtree.hpp) are used in the test implementations that are planned or exist in other parts of the test suite.hhds/BUILD (1)
Line range hint
20-92: Review changes to the BUILD file.The modifications in the BUILD file, including the renaming of targets and the adjustment of dependencies, are aligned with the changes described in the PR. However, ensure that all dependencies listed are necessary and correctly scoped to the targets they support.
hhds/lhtree.hpp (1)
285-324: Review of theTree_indexclassThe implementation of the
Tree_indexclass appears robust and well-structured. The use ofconstexprfor constructors and methods ensures that these operations can be evaluated at compile time when possible, which is beneficial for performance.However, the use of the packed attribute (line 285) should be justified as it can affect performance due to misaligned accesses, especially on architectures where alignment is crucial.
| Tree_index tree<X>::insert_next_sibling(const Tree_index &sibling, const X &data) { | ||
| I(sibling.level > 0); // No siblings to root | ||
|
|
||
| auto parent = get_parent(sibling); | ||
| auto *parent_lc_pos = ref_last_child_pos(parent); | ||
|
|
||
| GI(get_first_child(parent).pos != -1, get_parent(get_first_child(parent)).pos == parent.pos); | ||
|
|
||
| Tree_index child(sibling.level, -1); | ||
|
|
||
| if (has_next_sibling_space(sibling)) { | ||
| I((sibling.pos & 3) != 3); // sibling can not be the last if there is space | ||
| I(is_last_next_sibling_chunk(sibling)); | ||
|
|
||
| make_space_after(sibling); | ||
|
|
||
| auto last_child_pos = increase_size(sibling); | ||
| I(data_stack[sibling.level].size() > sibling.pos); | ||
|
|
||
| child.pos = sibling.pos + 1; | ||
|
|
||
| set_data(child, data); | ||
| I(is_leaf(child)); | ||
| if (*parent_lc_pos >> 2 == sibling.pos >> 2) { // new child is the youngest now | ||
| *parent_lc_pos = last_child_pos; | ||
| } | ||
|
|
||
| } else { | ||
| // WARNING: can not get ref, because the create can increse size and pointer is bogus | ||
| auto npos = get_next_sibling_pos(sibling); | ||
|
|
||
| if ((npos >> 2) != 0) { | ||
| Tree_index next_bucket_child(sibling.level, (npos >> 2) << 2); | ||
| I(pointers_stack[sibling.level][sibling.pos >> 2].parent == pointers_stack[sibling.level][next_bucket_child.pos >> 2].parent); | ||
| if (has_next_sibling_space(next_bucket_child)) { // No need to re-alloc. | ||
|
|
||
| if ((sibling.pos & 3) == 3) { // insert next bucket | ||
| make_space_after(next_bucket_child, false); | ||
| child.pos = next_bucket_child.pos; | ||
| } else { | ||
| make_space_after(sibling, next_bucket_child); | ||
| child.pos = sibling.pos + 1; | ||
| } | ||
| auto last_child_pos = increase_size(next_bucket_child); | ||
| set_data(child, data); | ||
| I(is_leaf(child)); | ||
|
|
||
| if ((*parent_lc_pos) >> 2 == next_bucket_child.pos >> 2) { | ||
| *parent_lc_pos = last_child_pos; | ||
| } | ||
|
|
||
| return child; | ||
| } | ||
| } | ||
|
|
||
| Tree_index created_next_sibling(sibling.level, -1); | ||
|
|
||
| if ((sibling.pos & 3) == 3) { // last sibling in bucket | ||
| created_next_sibling.pos = create_space(parent, data); | ||
|
|
||
| child.pos = created_next_sibling.pos; | ||
| } else { | ||
| created_next_sibling.pos = create_space(parent, data_stack[sibling.level][sibling.pos | 3]); | ||
| auto &l = pointers_stack[created_next_sibling.level]; | ||
| l[created_next_sibling.pos >> 2].first_child[0] = l[sibling.pos >> 2].first_child[3]; | ||
| l[created_next_sibling.pos >> 2].last_child[0] = l[sibling.pos >> 2].last_child[3]; | ||
| auto fc = l[sibling.pos >> 2].first_child[3]; | ||
| if (fc != -1) { | ||
| adjust_parent_pointer(Tree_index(sibling.level + 1, fc), sibling.pos | 3, created_next_sibling.pos); | ||
| } | ||
|
|
||
| make_space_after(sibling); | ||
|
|
||
| child.pos = sibling.pos + 1; | ||
|
|
||
| set_data(child, data); | ||
| I(is_leaf(child)); | ||
| } | ||
| I(pointers_stack[sibling.level][sibling.pos >> 2].parent | ||
| == pointers_stack[sibling.level][created_next_sibling.pos >> 2].parent); | ||
|
|
||
| // insert created in the next chain (if last *next_sibling>>2 is zero) | ||
| auto *next_sibling = ref_next_sibling_pos(sibling); | ||
| I(((*next_sibling) & 3) == 0); | ||
| auto *next_next_sibling = ref_next_sibling_pos(created_next_sibling); | ||
| *next_next_sibling |= *next_sibling; | ||
| *next_sibling = created_next_sibling.pos; | ||
|
|
||
| if ((*parent_lc_pos) >> 2 == sibling.pos >> 2) { // new child is the youngest now | ||
| *parent_lc_pos = created_next_sibling.pos; | ||
| } | ||
|
|
||
| I(!is_leaf(parent)); // Add sibling, so already has child | ||
| } | ||
|
|
||
| I(!child.is_invalid()); | ||
| I(is_leaf(child)); | ||
|
|
||
| return child; | ||
| } |
There was a problem hiding this comment.
Review of tree manipulation methods
The tree manipulation methods (lines 1060-1230) are crucial for the functionality of the tree data structure. The methods such as add_child, append_sibling, and insert_next_sibling are implemented with careful attention to detail.
However, there are potential improvements:
- The methods frequently check conditions and throw exceptions using
I(...). While this is useful for debugging, it might be beneficial to provide more descriptive error messages or handle errors in a way that is more informative to the users of the library. - The
insert_next_siblingmethod (lines 1131-1230) is quite complex and could benefit from further comments or refactoring to improve readability and maintainability.
Consider refactoring complex methods like insert_next_sibling to break them into smaller, more manageable functions. Additionally, improve error handling to provide more context-specific information.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- hhds/tests/tree_test.cpp (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- hhds/tests/tree_test.cpp
Basic pathological tests completed to make sure that most functionality (queries and basic add/append) work. Test files for now are
tree.cppandtesting.cpp. Need to test more complicated appends where the chunk might get shifted aroundSummary by CodeRabbit
New Features
add_root,get_data,set_data, andprint_tree.Refactor
std::optionalfor data storage.hhdstocoreand adjusted source and header file inclusions.tree_test.Chores
WORKSPACEtoMODULE.bazel.