-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8349211: Add support for intrusive trees to the utilities red-black tree #23416
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.
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
Conversation
|
👋 Welcome back cnorrbin! A progress list of the required criteria for merging this PR into |
|
@caspernorrbin This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 307 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@jdksjolen, @tstuefe, @xmas92) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
@caspernorrbin The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
jdksjolen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks,
A few comments. Still looking at this.
| #include "utilities/globalDefinitions.hpp" | ||
| #include <type_traits> | ||
|
|
||
| struct Empty {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will export the name Empty to everyone, is it possible to move it to inside of the class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty was used as the value type for the intrusive tree, but I discovered that it didn't quite work as expected, because sizeof(Empty) == 1 due to it requiring a unique address. This means that we would waste space in a lot of cases. For example, 8-byte keys would lead to 40-byte RBNodes despite storing only three 8-byte pointers and one 8-byte key.
I explored an alternative approach by adding the option to use void as the value type instead, and removing the need for a value member altogether. By using a base class containing only the value and using conditional inheritance from either it or Empty (which benefits from empty base optimization), we can have a zero-size overhead.
Code-wise, this solution doesn't look as clean. We need added templating and ENABLE_IFs for functions with value references (since void& doesn't work). Another positive however is that this enables key-only red-black trees for scenarios where values aren't necessary.
Let me know what you think :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's a bit "ugly", but I'm willing to pay the price for a little bit of complexity in order to have this feature work as expected. Please add "empty base optimization" as a phrase in a comment regarding this, so that people can find out on their own why the code looks as it does.
| struct Nothing {}; | ||
| struct Nothing {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now can use Empty instead.
|
Ping @stooke |
|
@caspernorrbin Thank you for this. Will make the RB tree much more useful to me. About the example: Why not skip the useless insert_node creation? Let insert_at_cursor just accept uninitialized Node* memory, since it will initialize all Node members anyway? I will do a full review after I get home. |
|
@caspernorrbin could you massage this patch a bit to reduce the delta to the last version? That is a good idea in general (I usually do a manual minimize-delta sweep before I undraft a PR for review). A lot seems to be code movement at first glance. |
As of now, the cursor doesn't track to what key its pointing to. All
A lot is new or rewritten so git diff had a hard time picking up related changes. I tried my best to order and group the changes so functionality overlaps. |
|
I changed the way The example would now look something like this: struct MyIntrusiveStructure {
Node node; // The tree node is part of an external structure
int data;
MyIntrusiveStructure(int data) : data(data) {}
Node* get_node() { return &node; }
static MyIntrusiveStructure* cast_to_self(Node* node) { return (MyIntrusiveStructure*)node; }
};
Tree my_intrusive_tree;
Cursor insert_cursor = my_intrusive_tree.cursor_find(0);
// Custom allocation here is just malloc
MyIntrusiveStructure* place = (MyIntrusiveStructure*)os::malloc(sizeof(MyIntrusiveStructure), mtTest);
new (place) MyIntrusiveStructure(123);
my_intrusive_tree.insert_at_cursor(place->get_node(), insert_cursor);
Cursor find_cursor = my_intrusive_tree.cursor_find(0);
int found_data = MyIntrusiveStructure::cast_to_self(find_cursor.node())->data; |
| // Inserts a node with the given key into the tree, | ||
| // does nothing if the key already exist. | ||
| void upsert(const K& key) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
upsert is a bad name for function, as it is a portmanteau of "update" and "insert", indicating that this should change something if the key is found. What's the goal with this function? It should probably return the allocated node if it's to be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This follows the pattern of the other value-less functions keeping the name, but I agree that it's less logical here. Would one insert(k) and one upsert(k, v) be a good solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
| inline const typename RBTree<K, V, COMPARATOR, ALLOCATOR>::Cursor | ||
| RBTree<K, V, COMPARATOR, ALLOCATOR>::cursor_find(const K& key) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using Tree = RBTree<K, V, COMPARATOR, ALLOCATOR:
using Cur = typename RBTree<K, V, COMPARATOR, ALLOCATOR>::Cursor;Though I'm pretty sure Thomas gave you a typedef for Tree, I don't think the Cur can be done with a typedef.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can access TreeType here in any meaningful way, since that is a part of the tree class and we're outside the class here, and we would still need to specify the template parameters. But please correct me if I'm wrong :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know :-), you might be right!
| public: | ||
| bool valid() const { return _insert_location != nullptr; } | ||
| bool found() const { return *_insert_location != nullptr; } | ||
| RBNode* node() { return _insert_location == nullptr ? nullptr : *_insert_location; } | ||
| RBNode* node() const { return _insert_location == nullptr ? nullptr : *_insert_location; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any case where I don't need to check the validity of the cursor? That is, do I ever want to use node() without first calling valid() or afterwards checking whether the returned value was null?
If the answer to that is: "No, there is no such case", then we shouldn't return null on !valid() node. We should instead add `assert(valid(), "must be");". If the answer is yes, then could you please tell me what that situation is :P?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's been useful in a couple of places, where we want "the node or nullptr otherwise", since you get the valid check and the node in one. A few examples where we don't check validity:
In visit_range_in_order, we iterate until the node (or nullptr) is reached.
In upsert, we extract the node node into a local variable and either modify the node or reuse the variable.
In closest_gt, we return the next node from the cursor, which could be invalid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, then leave it as is :-).
jdksjolen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright,
I'm okay with this. As all the internals now use cursors we basically get the intrusive-style API tested for free.
Thanks for your efforts on this!
tstuefe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @caspernorrbin,
Sorry for the delay. I tried to use the tree in a tiny small allocator (basically what I plan to do in Metaspace and in other places), and though it is now possible in principle I think we can simplify things and make them more user-friendly. Here my findings:
-
It would be good to have RBNode simplified and defined outside of the tree. Possibly even in a different header. I can see RBNode being used in places where I don't know exactly what tree it goes into. It could even go into multiple trees at the same time or at different times (so the data structure would have either multiple RBNode inlined or a single one that gets repurposed for different trees).
-
In that line, it would be good to have the key in RBNode being mutable. Having it const means I am forced to write constructors for containing structures. That is cumbersome.
-
To me, it seems the code could be a lot simpler if you were to just use standard subclassing (AbstractRBTree->(NonIntrusiveRBTree|IntrusiveRBTree) etc. All these
std::is_sameare a bit much. There would also be no need for theRBTreeNoopAllocator. The vtable tax you'd pay won't matter much in reality since I don't foresee many cases where the specific tree type is not known. -
I found I had little need for cursors at all. Mostly, they just got in the way. Cursor exists to modify tree structure, but why would I ever do that manually? It is different with simple structures like linked lists, but here the tree balances itself, so it has the last say about its structure anyway. I would be perfectly happy with just the simple ability to add/remove nodes manually, use nodes to find nearby nodes (as in, nodes of nearby keys), iterate nodes with a functor etc.
-
The few cases I needed a cursor it was because the API forced me to (e.g. when removing a node from the tree). With insertion, it got very weird. So I have an RBNode*, want to insert it into the tree, now I need an empty Cursor to do that? So, I create an empty cursor with that key, then use that with insert_at_cursor? Why?
-
Let's say I already have a nearby node (result of closest_gt, for instance), but it does not satisfy me, so I add a new one. For that I need to call normal insert, so the search is done all over again (see my remark above above). It would be good if we could have an insertion with a node as an insertion hint.
-
I found that I miss a closest_ge (greater or equal). Please add that. It's really needed for memory management trees.
-
In closest_gt() etc, please extend the comments to say what the behavior is when the node is not found. I assume null is returned?
I'll continue the work later (have vacation next week).
|
I just pushed a change addressing some of the points mentioned:
Any feedback would be appreciated! |
|
Just pushed another change, this time separating the node class into struct MyIntrusiveStructure {
IntrusiveRBNode node; // The tree node is part of an external structure
int key;
int data;
MyIntrusiveStructure(int key, int data) : key(key), data(data) {}
IntrusiveRBNode* get_node() { return &node; }
static MyIntrusiveStructure* cast_to_self(IntrusiveRBNode* node) { return (MyIntrusiveStructure*)node; }
};
struct IntrusiveCmp {
static int cmp(int a, const IntrusiveTreeNode* b) {
return a - MyIntrusiveStructure::cast_to_self(b)->key;
}
};
IntrusiveRBTree<int, IntrusiveCmp> my_intrusive_tree;
const int key = 0;
// Custom allocation here is just malloc
MyIntrusiveStructure* place = (MyIntrusiveStructure*)os::malloc(sizeof(MyIntrusiveStructure), mtTest);
new (place) MyIntrusiveStructure(key, 123);
my_intrusive_tree.insert(key, place->get_node());
IntrusiveRBNode* found_node = my_intrusive_tree.find_node(key);
int found_data = MyIntrusiveStructure::cast_to_self(found_node)->data;The key changes are:
|
|
@caspernorrbin This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
xmas92
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for being slow in reviewing.
The changes look good. I've also tried using this tree as a replacement for the tree we have been using in our ZGC project and its been working almost seamless.
I have a couple comments, I'll leave it up to you if it is something we want to fix in this PR or as followups if at all.
Thanks for all the work. I'll talk with you next week and then approve.
| const RBNode* curr = closest_geq(from); | ||
| if (curr == nullptr) return; | ||
| const RBNode* const end = closest_geq(to); | ||
| inline void AbstractRBTree<K, NodeType, COMPARATOR>::visit_range_in_order(const K& from, const K& to, F f) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preexisting.
This is an exclusive end. I would think inclusive end would be more natural. Otherwise you cannot iterate all the way to the end. (Currently can be worked around if the largest possible K is not in the tree, by using it as to).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to be inclusive instead, thank you for reviewing :)
| template <typename K, typename NodeType, typename COMPARATOR> | ||
| template <typename F> | ||
| inline void RBTree<K, V, COMPARATOR, ALLOCATOR>::visit_range_in_order(const K& from, const K& to, F f) const { | ||
| assert(COMPARATOR::cmp(from, to) <= 0, "from must be less or equal to to"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seem unfortunate to loose these assert, would be nice to find these sort of errors early.
Maybe we can have some verification functions on the tree which takes (const K& from, const K& to, const NodeType* end_node) which can dispatch to the correct COMPARATOR function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added asserts to check that from <= start and start <= to. As long as we find a node we can indirectly test this, since from <= start <= to => from <= to.
| if (old_node == new_node) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be a future enhancement. But it would be nice that if the COMPARATOR (or the NodeType) supplied a cmp(const NodeType* a, const NodeType* b) we could use it to check the order invariants for the children and parent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cmp(const NodeType* a, const NodeType* b) function is identical to the one you needed to give as a template to verify_self if you're using intrusive trees. I made that function be a part of the COMPARATOR template instead. Now, you supply a second cmp in COMPARATOR used for verification only, and that is used in both verify_self and replace_at_cursor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm.
Thanks!
| if (start != nullptr) { | ||
| assert_leq(from, start); | ||
| assert_geq(to, start); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we should add an else branch here where we assert end == nullptr / end == start. But given that we will more than likely just crash when reading start->next(), it does not matter to much. Regardless of any assert, a bad interval will crash early.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an assert just in case.
jdksjolen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACTION: main -- Error. Program `/home/runner/work/jdk/jdk/bundles/jdk/jdk-25/fastdebug/bin/java' timed out (timeout set to 480000ms, elapsed time including timeout handling was 520263ms).
REASON: User specified action: run main/othervm -Xmx1g -Xms1g -Xlog:gc -XX:+UnlockDiagnosticVMOptions -XX:+UnlockExperimentalVMOptions -XX:ShenandoahTargetNumRegions=2048 -XX:+UseShenandoahGC -XX:ShenandoahGCHeuristics=adaptive -XX:ShenandoahGCMode=generational -XX:+ShenandoahVerify TestAllocHumongousFragment
I think it's safe to say that this crash is unrelated to your changes.
Still LGTM.
|
Thank you everyone for your time on this! /integrate |
|
@caspernorrbin |
|
/sponsor |
|
Going to push as commit 4f97c4c.
Your commit was automatically rebased without conflicts. |
|
@jdksjolen @caspernorrbin Pushed as commit 4f97c4c. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Hi everyone,
The recently integrated red-black tree can be made more flexible by adding support of intrusive trees. In an intrusive tree, the user has full control over node allocation and placement instead of having the tree manage it internally.
Two key changes enable this feature:
Many of the auxiliary tree functions have been updated to use these new features, resulting in simplified and cleaned-up code. More tests have also been added to cover both new and existing functionality.
An example of how you could use the intrusive tree is found below:
Please let me know any feedback or concerns!
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23416/head:pull/23416$ git checkout pull/23416Update a local copy of the PR:
$ git checkout pull/23416$ git pull https://git.openjdk.org/jdk.git pull/23416/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 23416View PR using the GUI difftool:
$ git pr show -t 23416Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23416.diff
Using Webrev
Link to Webrev Comment