8367536: Change RBTree to use C++17 features #27260

caspernorrbin · 2025-09-12T13:59:51Z

Hi everyone,

C++17 lets us tidy up som of the ugly templating done in the red-black tree. We can replace the std::false_type/std::true_type tricks used to discover comparator and verifier signatures with the new std::is_invocable(_r_v), and most of the overload/SFINAE noise can disappear thanks to if constexpr.

We can now write one-liners such as:

static constexpr bool HasKeyComparator = std::is_invocable_r_v<RBTreeOrdering, decltype(&CMP::cmp), K, K>;

and then select the right branch with

if constexpr (HasKeyComparator<CMP>) { }

inside a single function instead of having several ENABLE_IF overloads.

This results in fewer lines, clearer intent, and more readable errors, while keeping behaviour identical.

Testing:

Oracle tiers 1-3

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8367536: Change RBTree to use C++17 features (Enhancement - P4)

Reviewers

Albert Mingkun Yang (@albertnetymk - Reviewer) Review applies to f20fc8e9
Kim Barrett (@kimbarrett - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27260/head:pull/27260
$ git checkout pull/27260

Update a local copy of the PR:
$ git checkout pull/27260
$ git pull https://git.openjdk.org/jdk.git pull/27260/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27260

View PR using the GUI difftool:
$ git pr show -t 27260

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27260.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-09-12T14:01:50Z

👋 Welcome back cnorrbin! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-09-12T14:03:21Z

@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:

8367536: Change RBTree to use C++17 features

Reviewed-by: kbarrett, ayang

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 114 new commits pushed to the master branch:

bca1e6e: 8365626: (fs) Improve handling of broken links in Files.isSameFile() (win)
695e36b: 8367927: Remove 8043571-related tests from problemlists
16458b6: 8367725: Incorrect reading of oop in SuspendResumeManager::suspend while thread is blocked
... and 111 more: https://git.openjdk.org/jdk/compare/02d7281b93296e7700e215804cb9e2f8341cab06...master

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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-09-12T14:03:58Z

@caspernorrbin The following label will be automatically applied to this pull request:

hotspot

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.

mlbridge · 2025-09-12T14:07:25Z

Webrevs

kimbarrett

Looks good.

kimbarrett · 2025-09-16T06:12:32Z

src/hotspot/share/utilities/rbTree.hpp

  template <typename CMP>
-  static constexpr bool HasKeyComparator = has_cmp_type<CMP, RBTreeOrdering, K, K>::value;
+  static constexpr bool HasKeyComparator =
+      std::is_invocable_r_v<RBTreeOrdering, decltype(&CMP::cmp), K, K>;


Note that these are static data member templates, which were not permitted by the style
guide until about an hour ago. :)

caspernorrbin · 2025-09-17T08:45:45Z

Thank you both for reviewing!

There seems to be a bug in older versions of gcc with partial template specialization of constexpr inside classes. This makes GHA fail on linux since it uses that older version, but works fine on newer versions or other platforms. I think the correct solutions is to make some changes to HasNodeVerifier to make GHA pass.

I have turned that back into a struct with std::false_type/std::true_type, similar in style to what it was before. It is still cleaner than the previous solutions, since we can still use std::is_invocable_r_v, and now also use std::bool_constant. With this we can get rid of the templated constexpr variables.

kimbarrett · 2025-09-17T21:30:19Z

There seems to be a bug in older versions of gcc with partial template specialization of constexpr inside classes. This makes GHA fail on linux since it uses that older version, but works fine on newer versions or other platforms. I think the correct solutions is to make some changes to HasNodeVerifier to make GHA pass.

Well that's annoying. Good job tracking down the issue.

This looks like a relatively narrow bug, only affecting static data member
templates with specializations. That's not a case I'd used in experiments and
ongoing work that have used variable templates, so thankfully I don't need to
rewrite any of those.

Sorry I seem to have not explicitly tested for this case when I proposed
permitting variable templates. It's very hard to test new features thoroughly,
and I mostly rely on compilers doing their own testing.

We could add a note to the style guide about it, though I'm reluctant to
clutter that document with this kind of thing. The fix is in gcc14, so quite
recent, even though the bug has been around for quite a while. My inclination
is to just leave things be, as it's a pretty narrow issue that seems to me
unlikely to come up often. And someday it will be moot, because we'll be
requiring a more recent version of gcc. Of course, there's the contrary
argument that you hit this pretty much right out of the gate...

Maybe leave a comment with has_not_verifier about the issue, referring to that
gcc bug? Oh, and has_node_verifier is a type, so should normally be studly
capped. Add an "Impl" suffix to avoid collision with the constant, and also
make it more obvious other code shouldn't be referencing it directly.

caspernorrbin · 2025-09-18T13:42:50Z

Maybe leave a comment with has_not_verifier about the issue, referring to that gcc bug? Oh, and has_node_verifier is a type, so should normally be studly capped. Add an "Impl" suffix to avoid collision with the constant, and also make it more obvious other code shouldn't be referencing it directly.

Thank you for the comment. It's unfortunate that such a bug exists, and that we had to encounter it almost immediately. I can definitely understand why it wasn't caught though. Seeing as it is pretty niche, I agree it shouldn't be added to the style guide at this time, perhaps in the future if it were to become a (somewhat) regular problem. Nevertheless, I pushed an update to document the issue (with the GCC PR ID) in the code along with changing the struct name to HasNodeVerifierImpl.

kimbarrett

Looks good.

jdksjolen · 2025-09-18T17:58:55Z

src/hotspot/share/utilities/rbTree.hpp

-  struct has_less_than_type : std::false_type {};
-  template <typename CMP, typename RET, typename ARG1, typename ARG2>
-  struct has_less_than_type<CMP, RET, ARG1, ARG2, decltype(static_cast<RET(*)(ARG1, ARG2)>(CMP::less), void())> : std::true_type {};
+  // Due to a bug in older GCC versions with static templeted constexpr data members (see GCC PR 71954),


templeted -> templated

kimbarrett

Still good.

is_invocable/if constexpr

0c6b03c

openjdk bot added hotspot [email protected] rfr Pull request is ready for review labels Sep 12, 2025

removed unneeded CMP templates

f20fc8e

albertnetymk approved these changes Sep 12, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 12, 2025

kimbarrett approved these changes Sep 16, 2025

View reviewed changes

change HasNodeVerifier back to struct

6f52cbf

openjdk bot removed the ready Pull request is ready to be integrated label Sep 17, 2025

Added clarifying note + change name to 'HasNodeVerifierImpl'

184b5fc

kimbarrett approved these changes Sep 18, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 18, 2025

jdksjolen reviewed Sep 18, 2025

View reviewed changes

typo

2a5eee2

openjdk bot removed the ready Pull request is ready to be integrated label Sep 19, 2025

kimbarrett approved these changes Sep 19, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 19, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

8367536: Change RBTree to use C++17 features #27260

8367536: Change RBTree to use C++17 features #27260

caspernorrbin commented Sep 12, 2025 •

edited by openjdk bot

Loading

Uh oh!

bridgekeeper bot commented Sep 12, 2025

Uh oh!

openjdk bot commented Sep 12, 2025 •

edited

Loading

Uh oh!

openjdk bot commented Sep 12, 2025 •

edited

Loading

Uh oh!

mlbridge bot commented Sep 12, 2025 •

edited

Loading

Uh oh!

kimbarrett left a comment

Uh oh!

kimbarrett Sep 16, 2025

Uh oh!

caspernorrbin commented Sep 17, 2025

Uh oh!

kimbarrett commented Sep 17, 2025

Uh oh!

caspernorrbin commented Sep 18, 2025

Uh oh!

kimbarrett left a comment

Uh oh!

jdksjolen Sep 18, 2025

Uh oh!

kimbarrett left a comment

Uh oh!

Uh oh!

8367536: Change RBTree to use C++17 features #27260

Are you sure you want to change the base?

8367536: Change RBTree to use C++17 features #27260

Conversation

caspernorrbin commented Sep 12, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Sep 12, 2025

Uh oh!

openjdk bot commented Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

kimbarrett left a comment

Choose a reason for hiding this comment

Uh oh!

kimbarrett Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

caspernorrbin commented Sep 17, 2025

Uh oh!

kimbarrett commented Sep 17, 2025

Uh oh!

caspernorrbin commented Sep 18, 2025

Uh oh!

kimbarrett left a comment

Choose a reason for hiding this comment

Uh oh!

jdksjolen Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

kimbarrett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

caspernorrbin commented Sep 12, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Sep 12, 2025 •

edited

Loading

openjdk bot commented Sep 12, 2025 •

edited

Loading

mlbridge bot commented Sep 12, 2025 •

edited

Loading