RFR: 8292591: Experimentally add back barrier-less Java thread transitions [v5]

Fri Sep 9 16:54:58 UTC 2022

On Fri, 9 Sep 2022 10:05:11 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

>> Please consider, only implemented on x64/aarch64 linux/windows. (@TheRealMDoerr have now contributed PPC64)
>> 
>> On my box calling clock_gettime via JNI goes from 35ns to 28ns when enabled.
>> 
>> Passes t1-7 with option forced on, also passes t1-4 as is in this PR.
>
> Robbin Ehn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fixed ws

Changes requested by dcubed (Reviewer).

src/hotspot/cpu/x86/templateInterpreterGenerator_x86.cpp line 1086:

> 1084: 
> 1085:   if (!UseSystemMemoryBarrier) {
> 1086:     // Force this write out before the read below

In other places where this comment exists and you added the
`if (!UseSystemMemoryBarrier) {`, you left the comment above
the if-statement. Why move it this time?

src/hotspot/os/linux/systemMemoryBarrier_linux.cpp line 22:

> 20:  * or visit www.oracle.com if you need additional information or have any
> 21:  * questions.
> 22:  *

Nit: extra "*" blank line in the header should be deleted.

src/hotspot/os/linux/systemMemoryBarrier_linux.cpp line 29:

> 27: #include "logging/log.hpp"
> 28: #include "utilities/debug.hpp"
> 29: #include "utilities/systemMemoryBarrier.hpp"

"runtime/os.hpp" should be after "logging/log.hpp".

src/hotspot/os/linux/systemMemoryBarrier_linux.cpp line 51:

> 49:   MEMBARRIER_CMD_PRIVATE_EXPEDITED          = (1 << 3),
> 50:   MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED = (1 << 4),
> 51: };

Does there need to be some form of "#ifndef XXX" ... "#endif" bracketing
for when we eventually reach systems running kernel 4.14?

src/hotspot/os/linux/systemMemoryBarrier_linux.cpp line 63:

> 61: 
> 62: static int membarrier(int cmd, unsigned int flags, int cpu_id) {
> 63:   return syscall(SYS_membarrier, cmd, flags, cpu_id); // cpu_id only on >=5.10

nit: need space before "5.10".

src/hotspot/os/linux/systemMemoryBarrier_linux.hpp line 22:

> 20:  * or visit www.oracle.com if you need additional information or have any
> 21:  * questions.
> 22:  *

Nit: extra "*" blank line in the header should be deleted.

src/hotspot/os/linux/systemMemoryBarrier_linux.hpp line 26:

> 24: 
> 25: #ifndef SHARE_UTILITIES_SYSTEMMEMORYBARRIER_LINUX_HPP
> 26: #define SHARE_UTILITIES_SYSTEMMEMORYBARRIER_LINUX_HPP

This file is not in "share/utilities" so this should be:

#ifndef OS_LINUX_SYSTEMMEMORYBARRIER_LINUX_HPP
#define OS_LINUX_SYSTEMMEMORYBARRIER_LINUX_HPP

src/hotspot/os/linux/systemMemoryBarrier_linux.hpp line 36:

> 34: };
> 35: 
> 36: #endif // SHARE_UTILITIES_SYSTEMMEMORYBARRIER_LINUX_HPP

This should be:

#endif // OS_LINUX_SYSTEMMEMORYBARRIER_LINUX_HPP

src/hotspot/os/windows/systemMemoryBarrier_windows.cpp line 22:

> 20:  * or visit www.oracle.com if you need additional information or have any
> 21:  * questions.
> 22:  *

Nit: extra "*" blank line in the header should be deleted.

src/hotspot/os/windows/systemMemoryBarrier_windows.cpp line 29:

> 27: 
> 28: #include <windows.h>
> 29: #include <processthreadsapi.h>

Should these be alpha sorted?

src/hotspot/os/windows/systemMemoryBarrier_windows.hpp line 22:

> 20:  * or visit www.oracle.com if you need additional information or have any
> 21:  * questions.
> 22:  *

Nit: extra "*" blank line in the header should be deleted.

src/hotspot/os/windows/systemMemoryBarrier_windows.hpp line 26:

> 24: 
> 25: #ifndef SHARE_UTILITIES_SYSTEMMEMORYBARRIER_WINDOWS_HPP
> 26: #define SHARE_UTILITIES_SYSTEMMEMORYBARRIER_WINDOWS_HPP

This file is not in "share/utilities" so this should be:

#ifndef OS_WINDOWS_SYSTEMMEMORYBARRIER_WINDOWS_HPP
#define OS_WINDOWS_SYSTEMMEMORYBARRIER_WINDOWS_HPP

src/hotspot/os/windows/systemMemoryBarrier_windows.hpp line 36:

> 34: };
> 35: 
> 36: #endif // SHARE_UTILITIES_SYSTEMMEMORYBARRIER_WINDOWS_HPP

This should be:

#endif // OS_WINDOWS_SYSTEMMEMORYBARRIER_WINDOWS_HPP

src/hotspot/share/runtime/globals.hpp line 1298:

> 1296:                                                                             \
> 1297:   product(bool, UseSystemMemoryBarrier, false, EXPERIMENTAL,                \
> 1298:           "Try enable system memory barrier")                               \

typo: s/Try enable/Try to enable/

src/hotspot/share/runtime/handshake.cpp line 380:

> 378:   if (UseSystemMemoryBarrier) {
> 379:     SystemMemoryBarrier::emit();
> 380:   }

It's not clear to me why this emit() is here.

src/hotspot/share/runtime/threads.cpp line 559:

> 557:   if (UseSystemMemoryBarrier) {
> 558:     if (!SystemMemoryBarrier::initialize()) {
> 559:       // UseSystemMemoryBarrier = false;

Delete the commented out line? Or was this done for testing? Or???

src/hotspot/share/runtime/threads.cpp line 560:

> 558:     if (!SystemMemoryBarrier::initialize()) {
> 559:       // UseSystemMemoryBarrier = false;
> 560:       vm_shutdown_during_initialization("Failed to initialize request system memory barrier synchronization.");

typo: s/initialize request system/initialize the requested system/

src/hotspot/share/utilities/systemMemoryBarrier.hpp line 22:

> 20:  * or visit www.oracle.com if you need additional information or have any
> 21:  * questions.
> 22:  *

Nit: extra "*" blank line in the header should be deleted.

test/hotspot/jtreg/runtime/handshake/HandshakeTransitionTest.java line 32:

> 30:  * @test HandshakeTransitionTest
> 31:  * @summary This does a sanity test of the poll in the native wrapper.
> 32:  * @requires vm.debug

Why was this deleted?
Have you tested this with release bits?

test/hotspot/jtreg/runtime/handshake/SystemMembarHandshakeTransitionTest.java line 22:

> 20:  * or visit www.oracle.com if you need additional information or have any
> 21:  * questions.
> 22:  *

Nit: extra "*" blank line in the header should be deleted.

test/hotspot/jtreg/runtime/handshake/SystemMembarHandshakeTransitionTest.java line 27:

> 25: import jdk.test.lib.Utils;
> 26: import jdk.test.lib.process.ProcessTools;
> 27: import jdk.test.lib.process.OutputAnalyzer;

Should these be in alpha sort order?

test/hotspot/jtreg/runtime/handshake/SystemMembarHandshakeTransitionTest.java line 57:

> 55:         commands.addAll(Arrays.asList(args));
> 56:         commands.add("HandshakeTransitionTest$Test");
> 57:         ProcessBuilder pb = ProcessTools.createTestJvm(commands);

Please make sure the new test is run with 'release' bits.

-------------

PR: https://git.openjdk.org/jdk/pull/10123