8314644: Change "Rvalue references and move semantics" into an accepted feature

[jdksjolen]

Hi,

I'd like to propose that rvalue references and move semantics are now considered permitted in the style guide. This change would allow for move constructors to be written. This enables more performant code, if the move ctr is less expensive than the copy ctr, but also more correct code. For the latter part, look at "8314571: GrowableArray should move its old data and not copy it". Here we can avoid using copy assignment, instead using move constructors, which more accurately reflects what is happening: The old elements are in fact moved, and not copied.

Two useful std functions will become available to us with this change:

1. std::move, for explicitly moving a value. This is a slightly more powerful static_cast<T&&>(T), in that it also handles T& corectly.
2. std::forward, which simplifies the usage of perfect forwarding. Perfect forwarding is a technique where in copying is minimized. To quote Scott Meyers ( https://cppandbeyond.com/2011/04/25/session-announcement-adventures-in-perfect-forwarding/ ):

Perfecting forwarding is an important C++0x technique built atop rvalue references. It allows move semantics to be automatically applied, even when the source and the destination of a move are separated by intervening function calls. Common examples include constructors and setter functions that forward arguments they receive to the data members of the class they are initializing or setting, as well as standard library functions like make_shared, which “perfect-forwards” its arguments to the class constructor of whatever object the to-be-created shared_ptr is to point to.

Looking forward to your feedback, thank you.
Johan

---

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-8314644: Change "Rvalue references and move semantics" into an accepted feature (Enhancement - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15386

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jsjolen! 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]

@jdksjolen The following labels will be automatically applied to this pull request:

• build
• hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (2cba5f23)
• 00: Full (a85b1d18)

[jdksjolen]

After internal discussion I have come to the conclusion that this probably won't move forward, but if we have any external contributors who would like to voice their opinion I am happy to hear them.

[kimbarrett]

Some thoughts of mine, cribbed from that internal discussion. I'll preface by
saying that I'm not fundamentally opposed to permitting at least some uses of
move semantics in HotSpot code. I've thought about making a proposal in this
area myself. I haven't, in part, because I've seen very bad bugs in both
directions, so I keep waffling on the question. Also, I have to admit that the
size of the Josuttis book on move semantics (https://www.cppmove.com/) has kind
of put me off. I enjoyed reading it. But foisting it on other HotSpot
developers feels a bit much. And most of us will need some understanding of it
if any of us start using it.

A specific problem we currently have is that there are lots of classes with
default copy/assign constructors that really shouldn’t. At least some of those
probably ought to be movable but not copyable. Others probably ought to be
explicitly non-copyable, to prevent accidents.

Move semantics is an excellent tool for some problems, much better than
alternatives. Movable but not copyable objects is one example. Perfect
forwarding is another. Unfortunately, it also seems to be a very sharp tool.
(So totally in keeping with C++ culture. :) ) I think that if we do allow it,
we need to do so with eyes open, and that we will want to provide some well
thought out guidance, guardrails, &etc.

There are several problems we need to address with such a proposal. One is
when to define a class to be movable. I think that’s usually fairly easy to
decide. Another is when to enable use of move, and how. That’s mostly an issue
for holders of movable objects (containers, objects with movable data members,
&etc). That’s less easy. Still another is when to use std::move explicitly. My
experience is that’s often a source of really bad bugs. Though maybe less
often than our current frequently bogus default copy/assign.

[mlbridge]

Mailing list message from daniel.daugherty at oracle.com on hotspot-dev:

A couple of questions:

- Are these C++ features supported in all of the compilers that we
use/support
? in OpenJDK? I mean the Tier1 platforms that Oracle supports in our CI
and the
? other platforms that OpenJDK supports in the GHA setups.
- How do we gauge the quality of support for these C++ features in the
set of
? compilers that we use/support in OpenJDK?
- Is it possible, feasible and desirable to create standalone tests for
these
? C++ features so that we can use the new tests to evaluate compiler
upgrades?

Dan

On 8/22/23 8:23 AM, Johan Sj?len wrote:

[jdksjolen]

Mailing list message from daniel.daugherty at oracle.com on hotspot-dev:

A couple of questions:

• Are these C++ features supported in all of the compilers that we use/support ? in OpenJDK? I mean the Tier1 platforms that Oracle supports in our CI and the ? other platforms that OpenJDK supports in the GHA setups. - How do we gauge the quality of support for these C++ features in the set of ? compilers that we use/support in OpenJDK? - Is it possible, feasible and desirable to create standalone tests for these ? C++ features so that we can use the new tests to evaluate compiler upgrades?

Dan

On 8/22/23 8:23 AM, Johan Sj?len wrote:

Hi Dan, here are my findings:

• Are these C++ features supported in all of the compilers that we use/support

Looking at: https://en.cppreference.com/w/cpp/compiler_support/11

We can confirm that Rvalue references are supported by gcc 4.5, clang 17 and MSVC (no version mentioned). For IBM XL for AIX it is supported according to this url: https://www.ibm.com/docs/en/openxl-c-and-cpp-aix/17.1.1?topic=features-supported-language-levels

To be clear: This is a feature from C++11 and is considered table stakes for C++ compilers today, I'd be surprised if it wasn't supported. But we don't want to be taken by surprise, I get that :-).

How do we gauge the quality of support for these C++ features in the set of compilers that we use/support in OpenJDK?

Is it possible, feasible and desirable to create standalone tests for these C++ features so that we can use the new tests to evaluate compiler upgrades

We may add tests checking that the move constructors are called when std::move is invoked of course, and this would also be some gauge of the quality of implementation in the compilers. I am not sure why this is needed however, we do not have tests checking that the compilers have compiled copy constructors correctly, for example. It would be more natural to write tests that check the capabilities of our classes that use move constructors. For example, in my recent CR 8314571 I added a test that checks that GrowableArray correctly handles a struct with only a move constructor. These tests would hopefully fail if a compiler upgrade breaks rvalue references. Reading the thread for 8274400 (which made usage of alignof acceptable), there were no mentions of adidng tests that ensure that alignof actually functions in our compilers.

[dcubed-ojdk]

Thanks for the info. I appreciate the effort that it takes to dig up the details
on all these tool sets. If we could come up with tests of our classes that rely
on newer C++ features such that the tests are good enough to catch underlying
C++ errors, then I would be happier with those. Thanks for considering the
feedback.

[magicus]

/label -build

[openjdk]

@magicus
The build label was successfully removed.

[bridgekeeper]

@jdksjolen 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!

[jdksjolen]

Hi,

I've expanded the text on this a bit. I'm basically attempting to say that the usage of this should be limited and I give an example. I also use "we" to refer to "HotSpot developers as a whole" and "you" to refer to "the reader of this document." The latter is previously established as a style choice, but the former is not AFAICS.

[bridgekeeper]

@jdksjolen 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!

[bridgekeeper]

@jdksjolen This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

[TheShermanTanker]

Shame that this never made it in, this sounds pretty useful