8261445: Use memory_order_relaxed for os::random(). #2484

theRealAph · 2021-02-09T14:49:12Z

os::random() is used a lot (once for every class) during HotSpot initialization, and it defaults to using a compare-and-swap operation with memory_order_conservative. We don't need that: it should be memory_order_relaxed.

Progress

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

Issue

JDK-8261445: Use memory_order_relaxed for os::random().

Reviewers

Andrew Dinn (@adinn - Reviewer)
Erik Österlund (@fisk - Reviewer)
Martin Doerr (@TheRealMDoerr - Reviewer)
David Holmes (@dholmes-ora - Reviewer)

Download

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

bridgekeeper · 2021-02-09T14:49:35Z

👋 Welcome back aph! 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 · 2021-02-09T14:50:25Z

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

hotspot-runtime

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 · 2021-02-09T15:06:09Z

Webrevs

00: Full (5dbc6e0)

adinn

Looks good.

openjdk · 2021-02-09T15:53:05Z

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

8261445: Use memory_order_relaxed for os::random().

Reviewed-by: adinn, eosterlund, mdoerr, dholmes

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

699a3cd: 8223188: Removed unnecessary #ifdef __cplusplus from .cpp sources
05c6009: 8259656: fixpath.sh changes broke _NT_SYMBOL_PATH in RunTests.gmk
ef7ee3f: 8225081: Remove Telia Company CA certificate expiring in April 2021
7c565f8: 8261209: isStandalone property: remove dependency on pretty-print
01d9280: 8261299: Use-after-free on failure path in LinuxPackage.c, getJvmLauncherLibPath
a00b130: 8261356: Clean up enum G1Mark
becee64: 8261279: sun/util/resources/cldr/TimeZoneNamesTest.java timed out
f395ee0: 8261306: ServiceLoader documentation has malformed Unicode escape
8f4c15f: 8198540: Dynalink leaks memory when generating type converters
edd5fc8: 8261096: Convert jlink tool to use Stream.toList()
... and 2 more: https://git.openjdk.java.net/jdk/compare/906facabad69688d549ea9c49dcc1ce73754f4c1...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.

shipilev

It seems superficially fine, given unsynchronized read of _rand_seed prior to that. But I have to wonder if in absence of any memory ordering semantics, this loop is guaranteed to terminate? That is, can't a sufficiently hostile optimizer reduce it to the CAS loop that always expects a constant "oldval" seed?

shipilev · 2021-02-09T18:24:14Z

...and yes, it seems to (borderline imperceptibly) improve startup :)

 Performance counter stats for 'taskset -c 13 build/linux-x86_64-server-release/images/jdk/bin/java -Xms128m -Xmx128m -Xint Hello' (5000 runs):

             23.27 msec task-clock                #    0.940 CPUs utilized            ( +-  0.01% )
        85,860,247      cycles                    #    3.690 GHz                      ( +-  0.01% )
        80,060,520      instructions              #    0.93  insn per cycle           ( +-  0.00% )

        0.02476574 +- 0.00000328 seconds time elapsed  ( +-  0.01% )

 Performance counter stats for 'taskset -c 13 build/linux-x86_64-server-release/images/jdk/bin/java -Xms128m -Xmx128m -Xint Hello' (5000 runs):

             23.22 msec task-clock                #    0.940 CPUs utilized            ( +-  0.01% )
        85,659,272      cycles                    #    3.689 GHz                      ( +-  0.01% )
        80,063,528      instructions              #    0.93  insn per cycle           ( +-  0.00% )

        0.02471087 +- 0.00000271 seconds time elapsed  ( +-  0.01% )

TheRealMDoerr · 2021-02-09T18:49:05Z

It seems superficially fine, given unsynchronized read of _rand_seed prior to that. But I have to wonder if in absence of any memory ordering semantics, this loop is guaranteed to terminate? That is, can't a sufficiently hostile optimizer reduce it to the CAS loop that always expects a constant "oldval" seed?

I can't see any problem here. _rand_seed is volatile so the compiler must preserve all accesses to it. And all processors I'm aware of are single-copy atomic which means that all accesses to the same memory location are ordered.

TheRealMDoerr

Looks like a nice improvement!

dholmes-ora

This seems fine to me too.

Thanks,
David

shipilev · 2021-02-10T07:34:18Z

It seems superficially fine, given unsynchronized read of _rand_seed prior to that. But I have to wonder if in absence of any memory ordering semantics, this loop is guaranteed to terminate? That is, can't a sufficiently hostile optimizer reduce it to the CAS loop that always expects a constant "oldval" seed?

I can't see any problem here. _rand_seed is volatile so the compiler must preserve all accesses to it. And all processors I'm aware of are single-copy atomic which means that all accesses to the same memory location are ordered.

Yes, that looks fine. If we match the C++11 semantics of relaxed, we still maintain the single modification order of the variable. I would suggest we load _rand_seed with Atomic::load(&_rand_seed) as a good style, though.

shipilev · 2021-04-16T08:00:53Z

This one seems to have fallen through the cracks. @theRealAph, would you like to finish it?

theRealAph · 2021-04-20T08:08:19Z

/integrate

openjdk · 2021-04-20T08:09:52Z

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

5136643: 8262725: IGV: crash when removing all graphs in a group
79798c6: 8265136: ZGC: Expose GarbageCollectorMXBeans for both pauses and cycles
f1d4ae6: 8263718: unused-result warning happens at os_linux.cpp
787908c: 8264221: Rewrite confusing stream API chain in SnippetMaps
142edd3: 8265152: jpackage cleanup fails on Windows with IOException deleting msi
ab22407: 8265486: ProblemList javax/sound/midi/Sequencer/Recording.java on macosx-aarch64
e0fd5fc: 8265028: JDWP debug agent thread lookup can be made faster
713483c: 8265373: Change to GCC 10.3 for building on Linux at Oracle
3990713: 8265463: ProblemList vmTestbase/vm/mlvm/mixed/stress/regression/b6969574/INDIFY_Test.java on Win-X64 -Xcomp
5b43b39: 8263154: [macos] DMG builds have finder errors
... and 974 more: https://git.openjdk.java.net/jdk/compare/906facabad69688d549ea9c49dcc1ce73754f4c1...master

Your commit was automatically rebased without conflicts.

Pushed as commit a25bae8.

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

Stage

5dbc6e0

openjdk bot added hotspot-runtime hotspot-runtime-dev@openjdk.org rfr Pull request is ready for review labels Feb 9, 2021

adinn approved these changes Feb 9, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 9, 2021

fisk approved these changes Feb 9, 2021

View reviewed changes

shipilev reviewed Feb 9, 2021

View reviewed changes

TheRealMDoerr approved these changes Feb 9, 2021

View reviewed changes

dholmes-ora approved these changes Feb 9, 2021

View reviewed changes

openjdk bot closed this Apr 20, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 20, 2021

8261445: Use memory_order_relaxed for os::random(). #2484

8261445: Use memory_order_relaxed for os::random(). #2484

theRealAph commented Feb 9, 2021 •

edited by openjdk bot

bridgekeeper bot commented Feb 9, 2021

openjdk bot commented Feb 9, 2021

mlbridge bot commented Feb 9, 2021

adinn left a comment

openjdk bot commented Feb 9, 2021 •

edited

shipilev left a comment

shipilev commented Feb 9, 2021

TheRealMDoerr commented Feb 9, 2021

TheRealMDoerr left a comment

dholmes-ora left a comment

shipilev commented Feb 10, 2021

shipilev commented Apr 16, 2021

theRealAph commented Apr 20, 2021

openjdk bot commented Apr 20, 2021

8261445: Use memory_order_relaxed for os::random(). #2484

8261445: Use memory_order_relaxed for os::random(). #2484

Conversation

theRealAph commented Feb 9, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Feb 9, 2021

openjdk bot commented Feb 9, 2021

mlbridge bot commented Feb 9, 2021

Webrevs

adinn left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 9, 2021 • edited

shipilev left a comment

Choose a reason for hiding this comment

shipilev commented Feb 9, 2021

TheRealMDoerr commented Feb 9, 2021

TheRealMDoerr left a comment

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

shipilev commented Feb 10, 2021

shipilev commented Apr 16, 2021

theRealAph commented Apr 20, 2021

openjdk bot commented Apr 20, 2021

theRealAph commented Feb 9, 2021 •

edited by openjdk bot

openjdk bot commented Feb 9, 2021 •

edited