8247910: Improve alignment and power-of-2 utilities using C++14

[kimbarrett]

Please review this change which updates the alignment and power-of-2
utilities (utilities/align.hpp and utilities/powerOfTwo.hpp) to use
C++14 features.

Where possible, the alignment functions are now constexpr. This
eliminates the need for alternate macros that needed to be used in
constexpr contexts.

Use <type_traits> and rather than HotSpot workalikes.
We no longer need max_value(), as the problematic platform for
std::numeric_limits::max() was Solaris.

Testing: tier1

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issues

• JDK-8247910: Improve alignment and power-of-2 utilities using C++14
• JDK-8238956: Replace powerOfTwo::max_value with std::numeric_limits

Reviewers

• Thomas Schatzl (@tschatzl - Reviewer)
• Stefan Karlsson (@stefank - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/126/head:pull/126
$ git checkout pull/126

[kimbarrett]

/reviewers 2

[bridgekeeper]

👋 Welcome back kbarrett! 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.

[kimbarrett]

/label hotspot

[openjdk]

@kimbarrett
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

[openjdk]

@kimbarrett
The hotspot label was successfully added.

[openjdk]

@kimbarrett To determine the appropriate audience for reviewing this pull request, one or more labels corresponding to different subsystems will normally be applied automatically. However, no automatic labelling rule matches the changes in this pull request.

In order to have an RFR email automatically sent to the correct mailing list, you will need to add one or more labels manually using the /label add "label" command. The following labels are valid: 2d awt beans build compiler core-libs hotspot hotspot-compiler hotspot-gc hotspot-jfr hotspot-runtime i18n javadoc jdk jmx kulla net nio security serviceability shenandoah sound swing.

[mlbridge]

Webrevs

• 00: Full (a82d4e8)

[mlbridge]

Mailing list message from Thomas Schatzl on hotspot-dev:

Hi,

On 11.09.20 10:50, Kim Barrett wrote:

Please review this change which updates the alignment and power-of-2
utilities (utilities/align.hpp and utilities/powerOfTwo.hpp) to use
C++14 features.

Where possible, the alignment functions are now constexpr. This
eliminates the need for alternate macros that needed to be used in
constexpr contexts.

Use <type_traits> and <limits> rather than HotSpot workalikes.
We no longer need max_value<T>(), as the problematic platform for
std::numeric_limits<T>::max() was Solaris.

Testing: tier1

in utilities/align.hpp:63:

62 constexpr T align_down(T size, A alignment) {

The comment mentions "lognot" twice, at least it took me a while to
understand that it is an abbreviation for "logical not". Probably it's
common in LISP context?. I still think it might be worth just typing it
out for easier reading.

If you think it's better to keep lognot, fine with me too, I'll mark
this as reviewed.

Thanks,
Thomas

[mlbridge]

Mailing list message from Kim Barrett on hotspot-dev:

On Sep 11, 2020, at 5:16 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:

Hi,

On 11.09.20 10:50, Kim Barrett wrote:

Please review this change which updates the alignment and power-of-2
utilities (utilities/align.hpp and utilities/powerOfTwo.hpp) to use
C++14 features.
Where possible, the alignment functions are now constexpr. This
eliminates the need for alternate macros that needed to be used in
constexpr contexts.
Use <type_traits> and <limits> rather than HotSpot workalikes.
We no longer need max_value<T>(), as the problematic platform for
std::numeric_limits<T>::max() was Solaris.
Testing: tier1

in utilities/align.hpp:63:

62 constexpr T align_down(T size, A alignment) {

The comment mentions "lognot" twice, at least it took me a while to understand that it is an abbreviation for "logical not". Probably it's common in LISP context?. I still think it might be worth just typing it out for easier reading.

I thought that was widespread usage; I guess my roots are showing. I'll
expand it to logical_not (which is the name of the corresponding C++
function object class).

If you think it's better to keep lognot, fine with me too, I'll mark this as reviewed.

Thanks,
Thomas

Thanks for reviewing.

[kimbarrett]

/issue add 8238956

[openjdk]

@kimbarrett
Adding additional issue to issue list: 8238956: Replace powerOfTwo::max_value with std::numeric_limits.

[stefank]

Looks good!

[openjdk]

@kimbarrett This change now passes all automated pre-integration checks. In addition to the automated checks, the change must also fulfill all project specific requirements

After integration, the commit message will be:

8247910: Improve alignment and power-of-2 utilities using C++14
8238956: Replace powerOfTwo::max_value with std::numeric_limits

Reviewed-by: tschatzl, stefank

• If you would like to add a summary, use the /summary command.
• To credit additional contributors, use the /contributor command.
• To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 20 commits pushed to the master branch:

• 70cc7fc: 8253098: Archived full module graph should be disabled if CDS heap cannot be mapped
• ac9d1b0: 8223187: Remove setLocale() call in jpackage native launcher
• 9c24a56: 8253029: [PPC64] Remove obsolete Power6 code
• e6a493a: 8252882: Clean up jdk.javadoc and the related parts of jdk.compiler
• 68da63d: 8240658: Code completion not working for lambdas in method invocations that require type inference
• b05290a: 8252898: remove bulk registration of JFR CompilerPhaseType names
• 779d2c3: 8253084: Zero VM is broken after JDK-8252689
• 07da3a1: 8253030: ZGC: Change ZMarkCompleteTimeout unit to microseconds
• f978f6f: 8252689: Classes are loaded from jrt:/java.base even when CDS is used
• c5e63b6: 8248186: Move CDS C++ vtable code to cppVtables.cpp
• ... and 10 more: https://git.openjdk.java.net/jdk/compare/8777ded1238ad195c6b1b63f54fa51984f8c95e4...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 70cc7fc198a68345345e963a0252f0f3f9f1cf73.

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

[mlbridge]

Mailing list message from Kim Barrett on hotspot-dev:

Thanks.

[kimbarrett]

/integrate

[openjdk]

@kimbarrett Since your change was applied there have been 20 commits pushed to the master branch:

• 70cc7fc: 8253098: Archived full module graph should be disabled if CDS heap cannot be mapped
• ac9d1b0: 8223187: Remove setLocale() call in jpackage native launcher
• 9c24a56: 8253029: [PPC64] Remove obsolete Power6 code
• e6a493a: 8252882: Clean up jdk.javadoc and the related parts of jdk.compiler
• 68da63d: 8240658: Code completion not working for lambdas in method invocations that require type inference
• b05290a: 8252898: remove bulk registration of JFR CompilerPhaseType names
• 779d2c3: 8253084: Zero VM is broken after JDK-8252689
• 07da3a1: 8253030: ZGC: Change ZMarkCompleteTimeout unit to microseconds
• f978f6f: 8252689: Classes are loaded from jrt:/java.base even when CDS is used
• c5e63b6: 8248186: Move CDS C++ vtable code to cppVtables.cpp
• ... and 10 more: https://git.openjdk.java.net/jdk/compare/8777ded1238ad195c6b1b63f54fa51984f8c95e4...master

Your commit was automatically rebased without conflicts.

Pushed as commit af8c678.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[mlbridge]

Mailing list message from Thomas St��fe on hotspot-dev:

Hi,

unfortunately this breaks my builds on Ubuntu 16.4. I am using gcc 5.4.0
which according to https://openjdk.java.net/groups/build/doc/building.html
should still be an accepted compiler?

Creating hotspot/variant-server/libjvm/gtest/gtestLauncher from 1 file(s)
In file included from
/shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/services/memTracker.hpp:85:0,
from
/shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/precompiled/precompiled.hpp:60:
/shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/services/mallocTracker.hpp:190:78:
error: size of array '_snapshot' is not an integral constant-expression
static size_t _snapshot[CALC_OBJ_SIZE_IN_TYPE(MallocMemorySnapshot,
size_t)];

^
In file included from
/shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/services/memTracker.hpp:87:0,
from
/shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/precompiled/precompiled.hpp:60:
/shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/services/virtualMemoryTracker.hpp:170:79:
error: size of array '_snapshot' is not an integral constant-expression
static size_t _snapshot[CALC_OBJ_SIZE_IN_TYPE(VirtualMemorySnapshot,
size_t)];

^
In file included from
/shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/services/memTracker.hpp:85:0,
from
/shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/precompiled/precompiled.hpp:60:
/shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/services/mallocTracker.hpp:190:78:
error: size of array '_snapshot' is not an integral constant-expression
static size_t _snapshot[CALC_OBJ_SIZE_IN_TYPE(MallocMemorySnapshot,
size_t)];

^
In file included from
/shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/services/memTracker.hpp:87:0,
from
/shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/precompiled/precompiled.hpp:60:
/shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/services/virtualMemoryTracker.hpp:170:79:
error: size of array '_snapshot' is not an integral constant-expression
static size_t _snapshot[CALC_OBJ_SIZE_IN_TYPE(VirtualMemorySnapshot,
size_t)];

^
cc1plus: error: unrecognized command line option
'-Wno-shift-negative-value' [-Werror]
cc1plus: error: unrecognized command line option '-Wno-cast-function-type'
[-Werror]

Cheers, Thomas

On Tue, Sep 15, 2020 at 12:09 AM Kim Barrett <kim.barrett at oracle.com> wrote:

On Sep 14, 2020, at 3:43 AM, Stefan Karlsson <stefank at openjdk.java.net>
wrote:

On Fri, 11 Sep 2020 08:44:49 GMT, Kim Barrett <kbarrett at openjdk.org>
wrote:

Thanks.

[mlbridge]

Mailing list message from Thomas St��fe on hotspot-dev:

... Okay I worked around the problem by upgrading to gcc 7.

However, should we not make sure that we are buildable on the lowest
required compiler level?

Thanks, Thomas

On Wed, Sep 23, 2020 at 8:14 AM Thomas St?fe <thomas.stuefe at gmail.com>
wrote:

Hi,

unfortunately this breaks my builds on Ubuntu 16.4. I am using gcc 5.4.0
which according to https://openjdk.java.net/groups/build/doc/building.html
should still be an accepted compiler?

Creating hotspot/variant-server/libjvm/gtest/gtestLauncher from 1 file(s)
In file included from
/shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/services/memTracker.hpp:85:0,
from
/shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/precompiled/precompiled.hpp:60:
/shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/services/mallocTracker.hpp:190:78:
error: size of array '_snapshot' is not an integral constant-expression
static size_t _snapshot[CALC_OBJ_SIZE_IN_TYPE(MallocMemorySnapshot,
size_t)];

\^

In file included from
/shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/services/memTracker.hpp:87:0,
from
/shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/precompiled/precompiled.hpp:60:
/shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/services/virtualMemoryTracker.hpp:170:79:
error: size of array '_snapshot' is not an integral constant-expression
static size_t _snapshot[CALC_OBJ_SIZE_IN_TYPE(VirtualMemorySnapshot,
size_t)];

 \^

In file included from
/shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/services/memTracker.hpp:85:0,
from
/shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/precompiled/precompiled.hpp:60:
/shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/services/mallocTracker.hpp:190:78:
error: size of array '_snapshot' is not an integral constant-expression
static size_t _snapshot[CALC_OBJ_SIZE_IN_TYPE(MallocMemorySnapshot,
size_t)];

\^

In file included from
/shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/services/memTracker.hpp:87:0,
from
/shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/precompiled/precompiled.hpp:60:
/shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/services/virtualMemoryTracker.hpp:170:79:
error: size of array '_snapshot' is not an integral constant-expression
static size_t _snapshot[CALC_OBJ_SIZE_IN_TYPE(VirtualMemorySnapshot,
size_t)];

 \^

cc1plus: error: unrecognized command line option
'-Wno-shift-negative-value' [-Werror]
cc1plus: error: unrecognized command line option '-Wno-cast-function-type'
[-Werror]

Cheers, Thomas

On Tue, Sep 15, 2020 at 12:09 AM Kim Barrett <kim.barrett at oracle.com>
wrote:

On Sep 14, 2020, at 3:43 AM, Stefan Karlsson <stefank at openjdk.java.net>
wrote:

On Fri, 11 Sep 2020 08:44:49 GMT, Kim Barrett <kbarrett at openjdk.org>
wrote:

Thanks.

[mlbridge]

Mailing list message from Kim Barrett on hotspot-dev:

On Sep 23, 2020, at 3:00 AM, Thomas St?fe <thomas.stuefe at gmail.com> wrote:

... Okay I worked around the problem by upgrading to gcc 7.

However, should we not make sure that we are buildable on the lowest required compiler level?

gcc5 is the first version of gcc that *claims* to completely support C++14. That?s the only reason
for setting it as the lower limit; it wasn?t based on data.

Rather than potentially contort code to support what is now a pretty old version, I?d prefer we
moved forward the minimum required version.

[mlbridge]

Mailing list message from Andrew Haley on hotspot-dev:

On 23/09/2020 09:24, Kim Barrett wrote:

gcc5 is the first version of gcc that *claims* to completely support
C++14. That?s the only reason for setting it as the lower limit; it
wasn?t based on data.

Rather than potentially contort code to support what is now a pretty
old version, I?d prefer we moved forward the minimum required
version.

I'd prefer that too, but it's not just about us developers: there are
many distros building newer OpenJDK on older systems, and they may not
have the freedom to change the system-wide C compiler. Given that we
want to encourage people to use modern OpenJDK releases we should at
least try to accommodate older compilers.

I'd say that if we can use GCC 5 with minor changes we should do so,
but not if it makes a mess of the code.

--
Andrew Haley (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

[mlbridge]

Mailing list message from Kim Barrett on hotspot-dev:

On Sep 23, 2020, at 5:39 AM, Andrew Haley <aph at redhat.com> wrote:

On 23/09/2020 09:24, Kim Barrett wrote:

gcc5 is the first version of gcc that *claims* to completely support
C++14. That?s the only reason for setting it as the lower limit; it
wasn?t based on data.

Rather than potentially contort code to support what is now a pretty
old version, I?d prefer we moved forward the minimum required
version.

I'd prefer that too, but it's not just about us developers: there are
many distros building newer OpenJDK on older systems, and they may not
have the freedom to change the system-wide C compiler. Given that we
want to encourage people to use modern OpenJDK releases we should at
least try to accommodate older compilers.

I'd say that if we can use GCC 5 with minor changes we should do so,
but not if it makes a mess of the code.

Sure, but the code that?s failing is nothing out of the ordinary for C++14. I?m
pretty surprised there?s a failure for anything like that.

I?m also not in a good position to try to track down the problem; if Thomas or
someone else wants to, I?m game for kibitzing, but I?m reluctant to try to drive
the investigation.

--
Andrew Haley (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. <https://urldefense.com/v3/__https://www.redhat.com__;!!GqivPVa7Brio!I1YAmf55J8AxadNeOeEq0g8zvKdUQ0d5zgrC66QQ4kdY23Ldo-WTx6Ww7U55232l$ >
https://urldefense.com/v3/__https://keybase.io/andrewhaley__;!!GqivPVa7Brio!I1YAmf55J8AxadNeOeEq0g8zvKdUQ0d5zgrC66QQ4kdY23Ldo-WTx6Ww7TU1JXe-$
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

[mlbridge]

Mailing list message from Kim Barrett on hotspot-dev:

On Sep 23, 2020, at 2:14 AM, Thomas St?fe <thomas.stuefe at gmail.com> wrote:

Hi,

unfortunately this breaks my builds on Ubuntu 16.4. I am using gcc 5.4.0 which according to https://openjdk.java.net/groups/build/doc/building.html should still be an accepted compiler?

Creating hotspot/variant-server/libjvm/gtest/gtestLauncher from 1 file(s)
In file included from /shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/services/memTracker.hpp:85:0,
from /shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/precompiled/precompiled.hpp:60:
/shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/services/mallocTracker.hpp:190:78: error: size of array '_snapshot' is not an integral constant-expression
static size_t _snapshot[CALC_OBJ_SIZE_IN_TYPE(MallocMemorySnapshot, size_t)];

I don?t see anything here to indicate what platform this is for. It?s not, by any chance, arm32?

[mlbridge]

Mailing list message from Thomas St��fe on hotspot-dev:

On Wed, Sep 23, 2020, 16:43 Kim Barrett <kim.barrett at oracle.com> wrote:

On Sep 23, 2020, at 2:14 AM, Thomas St?fe <thomas.stuefe at gmail.com>
wrote:

Hi,

unfortunately this breaks my builds on Ubuntu 16.4. I am using gcc 5.4.0
which according to https://openjdk.java.net/groups/build/doc/building.html
should still be an accepted compiler?

Creating hotspot/variant-server/libjvm/gtest/gtestLauncher from 1 file(s)
In file included from
/shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/services/memTracker.hpp:85:0,
from
/shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/precompiled/precompiled.hpp:60:

/shared/projects/openjdk/jdk-jep387/source/src/hotspot/share/services/mallocTracker.hpp:190:78:
error: size of array '_snapshot' is not an integral constant-expression

static size_t _snapshot[CALC_OBJ_SIZE_IN_TYPE(MallocMemorySnapshot,
size_t)];

I don?t see anything here to indicate what platform this is for. It?s
not, by any chance, arm32?

Sorry. No, plain old x64.

Upgrading to gcc 7 also fixed another problem I had where the gtests would
not be buildable for non-pch builds. So I guess gcc 5 support slowly
bitrots.

Unfortunately I do not have time to follow up on this. Maybe later.

That is, Thomas