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

[Robbin Ehn]

On Fri, 9 Sep 2022 16:21:40 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> Robbin Ehn has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fixed ws
>
> 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?

Fixed

> 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".

Fixed

> 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".

Fixed

> 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

Fixed

> 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

Fixed

> 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

Fixed

> 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

Fixed

> 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/

Fixed

> 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???

Yes, thanks, fixed.

> 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/

Fixed

> 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?

Fixed

> 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.

Yes it does.

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

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