Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8349525: RBTree: provide leftmost, rightmost, and a simple way to print trees #23486

Closed
wants to merge 4 commits into from
+253 −75
Closed

8349525: RBTree: provide leftmost, rightmost, and a simple way to print trees #23486

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ce35dc6
RBTree: leftmost, rightmost, print_on
tstuefe Feb 5, 2025
56fbca4
fix constness, lp64 issues
tstuefe Feb 6, 2025
c96cfa3
feedback johan
tstuefe Feb 7, 2025
4f9eddb
feedback caspar
tstuefe Feb 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions src/hotspot/share/utilities/rbTree.hpp
View file
Original file line number Diff line number Diff line change
@@ -114,7 +114,7 @@ class RBTree {
#endif // ASSERT
}; // End: RBNode

typedef RBTree<K, V, COMPARATOR, ALLOCATOR>::RBNode NodeType;
typedef TreeType::RBNode NodeType;

private:
RBNode* _root;
@@ -278,7 +278,7 @@ class RBTree {
}

// Returns leftmost node, nullptr if tree is empty.
// If COMPARATOR::cmp(a, b) behaves canonically ("1" for a < b), this will the smallest key value.
// If COMPARATOR::cmp(a, b) behaves canonically (positive value for a > b), this will the smallest key value.
const RBNode* leftmost() const {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a thought, no change needed:

The intrusive tree PR has the member RBNode* _first to get the leftmost node in constant time instead of having to traverse down the tree, but at the cost of an extra check when inserting/removing. Which solution do you prefer? I don't really have a preference so can adapt that PR either which way.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think iteration is way rarer than insert, so I would save the cycles and the added complexity.

Different way to look at it:

  • for small trees, depth is small, so few hops to the first node, so not much gained my keeping _first
  • for last trees, the majority of cycles is spent hopping between nodes, not hopping to the first node. Even then, as @jdksjolen remarked, the tree is balanced so the depth is predictable.

RBNode* n = _root, *n2 = nullptr;
while (n != nullptr) {
@@ -289,7 +289,7 @@ class RBTree {
}

// Returns rightmost node, nullptr if tree is empty.
// If COMPARATOR::cmp(a, b) behaves canonically ("1" for a < b), this will the largest key value.
// If COMPARATOR::cmp(a, b) behaves canonically (positive value for a > b), this will the largest key value.
const RBNode* rightmost() const {
RBNode* n = _root, *n2 = nullptr;
while (n != nullptr) {
@@ -300,8 +300,6 @@ class RBTree {
}

RBNode* leftmost() { return const_cast<NodeType*>(static_cast<const TreeType*>(this)->leftmost()); }

// Returns rightmost node (smallest key). Returns nullptr if tree is empty.
RBNode* rightmost() { return const_cast<NodeType*>(static_cast<const TreeType*>(this)->rightmost()); }

struct Range {
3 changes: 2 additions & 1 deletion src/hotspot/share/utilities/rbTree.inline.hpp
View file
Original file line number Diff line number Diff line change
@@ -487,6 +487,7 @@ inline void RBTree<K, V, COMPARATOR, ALLOCATOR>::visit_in_order(F f) const {
while (stack_idx > 0 || head != nullptr) {
while (head != nullptr) {
to_visit[stack_idx++] = head;
assert(stack_idx <= (int)(sizeof(to_visit)/sizeof(to_visit[0])), "stack too deep");
head = head->_left;
}
head = to_visit[--stack_idx];
@@ -569,7 +570,7 @@ void RBTree<K, V, COMPARATOR, ALLOCATOR>::print_node_on(outputStream* st, int de
st->print("] = ");
print_T<V>(st, n->val());
st->cr();
depth ++;
depth++;
if (n->_right != nullptr) {
print_node_on(st, depth, n->_right);
}
8 changes: 5 additions & 3 deletions test/hotspot/gtest/utilities/test_rbtree.cpp
View file
Original file line number Diff line number Diff line change
@@ -546,7 +546,11 @@ TEST_VM_F(RBTreeTest, LeftMostRightMost) {
}

struct PtrCmp {
static int cmp(const void* a, const void* b) { return a == b ? 0 : (a > b ? 1 : -1); }
static int cmp(const void* a, const void* b) {
const uintptr_t ai = p2u(a);
const uintptr_t bi = p2u(b);
return ai == bi ? 0 : (ai > bi ? 1 : -1);
}
};

TEST_VM(RBTreeTestNonFixture, TestPrintPointerTree) {
@@ -572,7 +576,6 @@ TEST_VM(RBTreeTestNonFixture, TestPrintPointerTree) {
tree.upsert(p3, 3);
stringStream ss;
tree.print_on(&ss);
// tty->print_cr("%s", ss.base());
const char* const N = nullptr;
ASSERT_NE(strstr(ss.base(), s1), N);
ASSERT_NE(strstr(ss.base(), s2), N);
@@ -597,7 +600,6 @@ TEST_VM(RBTreeTestNonFixture, TestPrintIntegerTree) {
tree.upsert(i3, 3);
stringStream ss;
tree.print_on(&ss);
// tty->print_cr("%s", ss.base());
const char* const N = nullptr;
ASSERT_NE(strstr(ss.base(), s1), N);
ASSERT_NE(strstr(ss.base(), s2), N);