RFR: 8229516: Thread.isInterrupted() always returns false after thread termination
Bug: https://bugs.openjdk.java.net/browse/JDK-8229516 CSR: https://bugs.openjdk.java.net/browse/JDK-8232676 (already approved) webrev: http://cr.openjdk.java.net/~dholmes/8229516/webrev/ This cross-cuts core-libs, hotspot-runtime and the JIT compilers, but only in small pieces each. There is also a small touch to serviceability code. This change is "simply" moving the "interrupted" field out of the osThread and into the java.lang.Thread so that it can be set independently of whether the thread is alive (and to make things easier for loom in the near future). It is very straightforward: - old scheme - interrupted field is in osThread - VM can read/write directly - Java code calls into VM to read/write - new scheme - interrupted field is in java.lang.Thread - VM has to use javaCalls to read/write "directly" - Java code can read/write directly No changes to any of the semantics regarding the actual interrupt mechanism. Special thanks to Patricio for tracking down a bug I had introduced in that regard! Special Note (Hi Roger!): on windows we still need to set/clear the _interrupt_event used by the Process.waitFor logic. To facilitate clearing via Thread.interrupted() I had to introduce a native method that is a no-op except on Windows. This seemed the cheapest and least intrusive means to achieve this. Other changes revolve around the fact we used to have an intrinsic for Thread.isInterrupted and that is not needed any more. So we strip some code out of C1/C2. The changes in the JVMCI/Graal code are a bit more far reaching as entire classes disappear. I've cc'd Doug and Tom at Vladimir's request so that they can comment on the JVMCI changes and whether I have gone too far or not far enough. There are a bunch of tests for interruption in JVMCI that could potentially be deleted if they are only intended to test the JVMCI handling of interrupt: ./jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.jtt/src/org/graalvm/compiler/jtt/threads/Thread_isInterrupted*.java Testing: - Tiers 1-3 on all Oracle platforms - Focused testing on Linux x64: - Stress runs of JSR166TestCase - Anything that seems to use interrupt(): - JDK - java/lang/Thread - java/util/concurrent - jdk/internal/loader/InterruptedClassLoad.java - javax/management - java/nio/file/Files - java/nio/channels - java/net/Socket/Timeouts.java - java/lang/Runtime/shutdown/ - java/lang/ProcessBuilder/Basic.java - com/sun/jdi/ - Hotspot - vmTestbase/nsk/monitoring/ - vmTestbase/nsk/jdwp - vmTestbase/nsk/jdb/ - vmTestbase/nsk/jdi/ - vmTestbase/nsk/jvmti/ - runtime/Thread - serviceability/jvmti/ - serviceability/jdwp - serviceability/sa Thanks, David -
Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination
Hi David, Since Graal needs to work against multiple JVMCI versions (and VM versions!), the Graal changes should only disable the intrinsic when the relevant VM config is missing: diff --git a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java index 0752a621215..baa2136a6ba 100644 --- a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java +++ b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java @@ -315,7 +315,7 @@ public class GraalHotSpotVMConfig extends GraalHotSpotVMConfigBase { public final int javaThreadAnchorOffset = getFieldOffset("JavaThread::_anchor", Integer.class, "JavaFrameAnchor"); public final int javaThreadShouldPostOnExceptionsFlagOffset = getFieldOffset("JavaThread::_should_post_on_exceptions_flag", Integer.class, "int", Integer.MIN_VALUE); public final int threadObjectOffset = getFieldOffset("JavaThread::_threadObj", Integer.class, "oop"); -public final int osThreadOffset = getFieldOffset("JavaThread::_osthread", Integer.class, "OSThread*"); +public final int osThreadOffset = getFieldOffset("JavaThread::_osthread", Integer.class, "OSThread*", Integer.MAX_VALUE); public final int threadIsMethodHandleReturnOffset = getFieldOffset("JavaThread::_is_method_handle_return", Integer.class, "int"); public final int threadObjectResultOffset = getFieldOffset("JavaThread::_vm_result", Integer.class, "oop"); public final int jvmciCountersThreadOffset = getFieldOffset("JavaThread::_jvmci_counters", Integer.class, "jlong*"); diff --git a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java index 6b37fff083d..ffc8032d2b0 100644 --- a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java +++ b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java @@ -441,7 +441,9 @@ public class HotSpotGraphBuilderPlugins { } }); -r.registerMethodSubstitution(ThreadSubstitutions.class, "isInterrupted", Receiver.class, boolean.class); +if (config.osThreadOffset != Integer.MAX_VALUE) { +r.registerMethodSubstitution(ThreadSubstitutions.class, "isInterrupted", Receiver.class, boolean.class); +} } public static final String reflectionClass; diff --git a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java index 1d694fae2a4..8500c4de115 100644 --- a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java +++ b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java @@ -308,6 +308,7 @@ public class HotSpotReplacementsUtil { @Fold public static int osThreadOffset(@InjectedParameter GraalHotSpotVMConfig config) { +assert config.instanceKlassInitThreadOffset != Integer.MAX_VALUE; return config.osThreadOffset; } All the other JVMCI changes look good to me. -Doug > On 29 Oct 2019, at 08:42, David Holmes wrote: > > Bug: https://bugs.openjdk.java.net/browse/JDK-8229516 > CSR: https://bugs.openjdk.java.net/browse/JDK-8232676 (already approved) > webrev: http://cr.openjdk.java.net/~dholmes/8229516/webrev/ > > This cross-cuts core-libs, hotspot-runtime and the JIT compilers, but only in > small pieces each. There is also a small touch to serviceability code. > > This change is "simply" moving the "interrupted" field out of the osThread > and into the java.lang.Thread so that it can be set independently of whether > the thread is alive (and to make things easier for loom in the near future). > It is very straightforward: > > - old scheme > - interrupted field is in osThread > - VM can read/write directly > - Java code calls into VM to read/write > > - new scheme > - interrupted field is in java.lang.Thread > - VM has to use javaCalls to read/write "directly" > - Java code can read/write directly > > No changes to any of the semantics regarding the actual interrupt mechanism. > Special thanks to Patricio for tracking down a bug I had introduced in that > regard! > > Special Note (Hi Roger!): on windows we still need to set/clear the > _interrupt_event used by the Process.waitFor logic. To facilitate clearing > via Thread.interrupted() I had to introduce a native method that is a no-op > except on Windows. This se
Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination
Hi Doug, Thanks for taking a look so quickly! :) On 29/10/2019 6:55 pm, Doug Simon wrote: Hi David, Since Graal needs to work against multiple JVMCI versions (and VM versions!), the Graal changes should only disable the intrinsic when the relevant VM config is missing: So to be clear I should revert all the Graal file changes I made and just apply the patch below? Thanks, David - diff --git a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java index 0752a621215..baa2136a6ba 100644 --- a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java +++ b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java @@ -315,7 +315,7 @@ public class GraalHotSpotVMConfig extends GraalHotSpotVMConfigBase { public final int javaThreadAnchorOffset = getFieldOffset("JavaThread::_anchor", Integer.class, "JavaFrameAnchor"); public final int javaThreadShouldPostOnExceptionsFlagOffset = getFieldOffset("JavaThread::_should_post_on_exceptions_flag", Integer.class, "int", Integer.MIN_VALUE); public final int threadObjectOffset = getFieldOffset("JavaThread::_threadObj", Integer.class, "oop"); -public final int osThreadOffset = getFieldOffset("JavaThread::_osthread", Integer.class, "OSThread*"); +public final int osThreadOffset = getFieldOffset("JavaThread::_osthread", Integer.class, "OSThread*", Integer.MAX_VALUE); public final int threadIsMethodHandleReturnOffset = getFieldOffset("JavaThread::_is_method_handle_return", Integer.class, "int"); public final int threadObjectResultOffset = getFieldOffset("JavaThread::_vm_result", Integer.class, "oop"); public final int jvmciCountersThreadOffset = getFieldOffset("JavaThread::_jvmci_counters", Integer.class, "jlong*"); diff --git a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java index 6b37fff083d..ffc8032d2b0 100644 --- a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java +++ b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java @@ -441,7 +441,9 @@ public class HotSpotGraphBuilderPlugins { } }); -r.registerMethodSubstitution(ThreadSubstitutions.class, "isInterrupted", Receiver.class, boolean.class); +if (config.osThreadOffset != Integer.MAX_VALUE) { +r.registerMethodSubstitution(ThreadSubstitutions.class, "isInterrupted", Receiver.class, boolean.class); +} } public static final String reflectionClass; diff --git a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java index 1d694fae2a4..8500c4de115 100644 --- a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java +++ b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java @@ -308,6 +308,7 @@ public class HotSpotReplacementsUtil { @Fold public static int osThreadOffset(@InjectedParameter GraalHotSpotVMConfig config) { +assert config.instanceKlassInitThreadOffset != Integer.MAX_VALUE; return config.osThreadOffset; } All the other JVMCI changes look good to me. -Doug On 29 Oct 2019, at 08:42, David Holmes wrote: Bug: https://bugs.openjdk.java.net/browse/JDK-8229516 CSR: https://bugs.openjdk.java.net/browse/JDK-8232676 (already approved) webrev: http://cr.openjdk.java.net/~dholmes/8229516/webrev/ This cross-cuts core-libs, hotspot-runtime and the JIT compilers, but only in small pieces each. There is also a small touch to serviceability code. This change is "simply" moving the "interrupted" field out of the osThread and into the java.lang.Thread so that it can be set independently of whether the thread is alive (and to make things easier for loom in the near future). It is very straightforward: - old scheme - interrupted field is in osThread - VM can read/write directly - Java code calls into VM to read/write - new scheme - interrupted field is in java.lang.Thread - VM has to use javaCalls to read/write "directly" - Java code can read/write directly No changes to any of the semantics regarding the actual interrupt mechanism. Special thanks to Patricio for tracking down a bug I had introduced in that regard! Special Note (Hi Roger!): on windows we still need to set/clear the
Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination
> On 29 Oct 2019, at 10:12, David Holmes wrote: > > Hi Doug, > > Thanks for taking a look so quickly! :) > > On 29/10/2019 6:55 pm, Doug Simon wrote: >> Hi David, >> Since Graal needs to work against multiple JVMCI versions (and VM >> versions!), the Graal changes should only disable the intrinsic when the >> relevant VM config is missing: > > So to be clear I should revert all the Graal file changes I made and just > apply the patch below? Yes please. -Doug > >> diff --git >> a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java >> >> b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java >> index 0752a621215..baa2136a6ba 100644 >> --- >> a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java >> +++ >> b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java >> @@ -315,7 +315,7 @@ public class GraalHotSpotVMConfig extends >> GraalHotSpotVMConfigBase { >> public final int javaThreadAnchorOffset = >> getFieldOffset("JavaThread::_anchor", Integer.class, "JavaFrameAnchor"); >> public final int javaThreadShouldPostOnExceptionsFlagOffset = >> getFieldOffset("JavaThread::_should_post_on_exceptions_flag", Integer.class, >> "int", Integer.MIN_VALUE); >> public final int threadObjectOffset = >> getFieldOffset("JavaThread::_threadObj", Integer.class, "oop"); >> -public final int osThreadOffset = >> getFieldOffset("JavaThread::_osthread", Integer.class, "OSThread*"); >> +public final int osThreadOffset = >> getFieldOffset("JavaThread::_osthread", Integer.class, "OSThread*", >> Integer.MAX_VALUE); >> public final int threadIsMethodHandleReturnOffset = >> getFieldOffset("JavaThread::_is_method_handle_return", Integer.class, "int"); >> public final int threadObjectResultOffset = >> getFieldOffset("JavaThread::_vm_result", Integer.class, "oop"); >> public final int jvmciCountersThreadOffset = >> getFieldOffset("JavaThread::_jvmci_counters", Integer.class, "jlong*"); >> diff --git >> a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java >> >> b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java >> index 6b37fff083d..ffc8032d2b0 100644 >> --- >> a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java >> +++ >> b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java >> @@ -441,7 +441,9 @@ public class HotSpotGraphBuilderPlugins { >> } >> }); >> -r.registerMethodSubstitution(ThreadSubstitutions.class, >> "isInterrupted", Receiver.class, boolean.class); >> +if (config.osThreadOffset != Integer.MAX_VALUE) { >> +r.registerMethodSubstitution(ThreadSubstitutions.class, >> "isInterrupted", Receiver.class, boolean.class); >> +} >> } >>public static final String reflectionClass; >> diff --git >> a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java >> >> b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java >> index 1d694fae2a4..8500c4de115 100644 >> --- >> a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java >> +++ >> b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java >> @@ -308,6 +308,7 @@ public class HotSpotReplacementsUtil { >>@Fold >> public static int osThreadOffset(@InjectedParameter >> GraalHotSpotVMConfig config) { >> +assert config.instanceKlassInitThreadOffset != Integer.MAX_VALUE; >> return config.osThreadOffset; >> } >> All the other JVMCI changes look good to me. >> -Doug >>> On 29 Oct 2019, at 08:42, David Holmes wrote: >>> >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8229516 >>> CSR: https://bugs.openjdk.java.net/browse/JDK-8232676 (already approved) >>> webrev: http://cr.openjdk.java.net/~dholmes/8229516/webrev/ >>> >>> This cross-cuts core-libs, hotspot-runtime and the JIT compilers, but only >>> in small pieces each. There is also a small touch to serviceability code. >>> >>> This change is "simply" moving the "interrupted" field out of the osThread >>> and into the java.lang.Thread so that it can be set independently of >>> whether the thread is alive (and to make things easier for loom in the near >>> future). It is very straightforward: >>> >>> - old scheme >>> - interrupted field is in osThread >>> - VM can read/write directly >>> - Java code calls into VM to read/write >>> >>> - new scheme >>> - interrupted field is in java.
Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination
Hi David, The fix looks good to me. I did not pay much attention to the Graal related changes though. The test coverage for Serviceability is complete. Running java/lang/instrument tests is not necessary. Thanks, Serguei On 10/29/19 00:42, David Holmes wrote: Bug: https://bugs.openjdk.java.net/browse/JDK-8229516 CSR: https://bugs.openjdk.java.net/browse/JDK-8232676 (already approved) webrev: http://cr.openjdk.java.net/~dholmes/8229516/webrev/ This cross-cuts core-libs, hotspot-runtime and the JIT compilers, but only in small pieces each. There is also a small touch to serviceability code. This change is "simply" moving the "interrupted" field out of the osThread and into the java.lang.Thread so that it can be set independently of whether the thread is alive (and to make things easier for loom in the near future). It is very straightforward: - old scheme - interrupted field is in osThread - VM can read/write directly - Java code calls into VM to read/write - new scheme - interrupted field is in java.lang.Thread - VM has to use javaCalls to read/write "directly" - Java code can read/write directly No changes to any of the semantics regarding the actual interrupt mechanism. Special thanks to Patricio for tracking down a bug I had introduced in that regard! Special Note (Hi Roger!): on windows we still need to set/clear the _interrupt_event used by the Process.waitFor logic. To facilitate clearing via Thread.interrupted() I had to introduce a native method that is a no-op except on Windows. This seemed the cheapest and least intrusive means to achieve this. Other changes revolve around the fact we used to have an intrinsic for Thread.isInterrupted and that is not needed any more. So we strip some code out of C1/C2. The changes in the JVMCI/Graal code are a bit more far reaching as entire classes disappear. I've cc'd Doug and Tom at Vladimir's request so that they can comment on the JVMCI changes and whether I have gone too far or not far enough. There are a bunch of tests for interruption in JVMCI that could potentially be deleted if they are only intended to test the JVMCI handling of interrupt: ./jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.jtt/src/org/graalvm/compiler/jtt/threads/Thread_isInterrupted*.java Testing: - Tiers 1-3 on all Oracle platforms - Focused testing on Linux x64: - Stress runs of JSR166TestCase - Anything that seems to use interrupt(): - JDK - java/lang/Thread - java/util/concurrent - jdk/internal/loader/InterruptedClassLoad.java - javax/management - java/nio/file/Files - java/nio/channels - java/net/Socket/Timeouts.java - java/lang/Runtime/shutdown/ - java/lang/ProcessBuilder/Basic.java - com/sun/jdi/ - Hotspot - vmTestbase/nsk/monitoring/ - vmTestbase/nsk/jdwp - vmTestbase/nsk/jdb/ - vmTestbase/nsk/jdi/ - vmTestbase/nsk/jvmti/ - runtime/Thread - serviceability/jvmti/ - serviceability/jdwp - serviceability/sa Thanks, David -
Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination
On 10/29/19 3:42 AM, David Holmes wrote: Bug: https://bugs.openjdk.java.net/browse/JDK-8229516 CSR: https://bugs.openjdk.java.net/browse/JDK-8232676 (already approved) webrev: http://cr.openjdk.java.net/~dholmes/8229516/webrev/ make/hotspot/symbols/symbols-unix No comments. src/hotspot/os/windows/osThread_windows.cpp No comments. src/hotspot/os/windows/osThread_windows.hpp No comments. src/hotspot/share/aot/aotCodeHeap.cpp Skipped. src/hotspot/share/classfile/javaClasses.cpp No comments. src/hotspot/share/classfile/javaClasses.hpp No comments. src/hotspot/share/classfile/vmSymbols.cpp No comments. src/hotspot/share/classfile/vmSymbols.hpp No comments. src/hotspot/share/include/jvm.h No comments. src/hotspot/share/jvmci/jvmciRuntime.cpp src/hotspot/share/jvmci/jvmciRuntime.hpp src/hotspot/share/jvmci/vmStructs_jvmci.cpp Skipped these three. src/hotspot/share/oops/oop.hpp No comments. src/hotspot/share/oops/oop.inline.hpp No comments. src/hotspot/share/opto/c2compiler.cpp No comments. src/hotspot/share/opto/library_call.cpp No comments. src/hotspot/share/prims/jvm.cpp No comments. src/hotspot/share/prims/jvmtiEnv.cpp No comments. src/hotspot/share/prims/jvmtiEnvBase.cpp No comments. src/hotspot/share/runtime/osThread.cpp No comments. src/hotspot/share/runtime/osThread.hpp No comments. src/hotspot/share/runtime/thread.cpp No comments. src/hotspot/share/runtime/vmStructs.cpp No comments. src/java.base/share/classes/java/lang/Thread.java No comments. src/java.base/share/native/libjava/Thread.c L76: // Need to reset the interrupt event used by Process.waitFor L77: ResetEvent((HANDLE) JVM_GetThreadInterruptEvent()); nit - the indent in JDK .c files is four spaces. src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/OSThread.java No comments. src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/HotSpotMethodSubstitutionTest.java src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/ThreadSubstitutions.java Skipped these 5 graal files. Thumbs up! I only have one nit above and don't need to see another webrev if you decide to fix it. Thanks for being so thorough on your testing. Dan This cross-cuts core-libs, hotspot-runtime and the JIT compilers, but only in small pieces each. There is also a small touch to serviceability code. This change is "simply" moving the "interrupted" field out of the osThread and into the java.lang.Thread so that it can be set independently of whether the thread is alive (and to make things easier for loom in the near future). It is very straightforward: - old scheme - interrupted field is in osThread - VM can read/write directly - Java code calls into VM to read/write - new scheme - interrupted field is in java.lang.Thread - VM has to use javaCalls to read/write "directly" - Java code can read/write directly No changes to any of the semantics regarding the actual interrupt mechanism. Special thanks to Patricio for tracking down a bug I had introduced in that regard! Special Note (Hi Roger!): on windows we still need to set/clear the _interrupt_event used by the Process.waitFor logic. To facilitate clearing via Thread.interrupted() I had to introduce a native method that is a no-op except on Windows. This seemed the cheapest and least intrusive means to achieve this. Other changes revolve around the fact we used to have an intrinsic for Thread.isInterrupted and that is not needed any more. So we strip some code out of C1/C2. The changes in the JVMCI/Graal code are a bit more far reaching as entire classes disappear. I've cc'd Doug and Tom at Vladimir's request so that they can comment on the JVMCI changes and whether I have gone too far or not far enough. There are a bunch of tests for interruption in JVMCI that could potentially be deleted if they are only intended to test the JVMCI handling of interrupt: ./jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.jtt/src/org/graalvm/compiler/jtt/threads/Thread_isInterrupted*.java Testing: - Tiers 1-3 on all Oracle platforms - Focused testing on Linux x64: - Stress runs of JSR166TestCase - Anything that seems to use interrupt(): - JDK - java/lang/Thread - java/util/concurrent - jdk/internal/loader/InterruptedClassLoad.java - javax/management -
Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination
Hi David, in java.lang.Thread interrupt0 should be renamed to setInterruptEvent, because what it does now and I don't really understand the comment in interrupted(), if a thread is interrupted by two other threads calling interrupt(), you will loose an interrupt anyway. Rémi - Mail original - > De: "David Holmes" > À: "hotspot-runtime-dev" , "hotspot > compiler" > , "core-libs-dev" > , "Doug Simon" > , "TOM.RODRIGUEZ" , > "serviceability-dev" > , "Roger Riggs" > Envoyé: Mardi 29 Octobre 2019 08:42:08 > Objet: RFR: 8229516: Thread.isInterrupted() always returns false after thread > termination > Bug: https://bugs.openjdk.java.net/browse/JDK-8229516 > CSR: https://bugs.openjdk.java.net/browse/JDK-8232676 (already approved) > webrev: http://cr.openjdk.java.net/~dholmes/8229516/webrev/ > > This cross-cuts core-libs, hotspot-runtime and the JIT compilers, but > only in small pieces each. There is also a small touch to serviceability > code. > > This change is "simply" moving the "interrupted" field out of the > osThread and into the java.lang.Thread so that it can be set > independently of whether the thread is alive (and to make things easier > for loom in the near future). It is very straightforward: > > - old scheme > - interrupted field is in osThread > - VM can read/write directly > - Java code calls into VM to read/write > > - new scheme > - interrupted field is in java.lang.Thread > - VM has to use javaCalls to read/write "directly" > - Java code can read/write directly > > No changes to any of the semantics regarding the actual interrupt > mechanism. Special thanks to Patricio for tracking down a bug I had > introduced in that regard! > > Special Note (Hi Roger!): on windows we still need to set/clear the > _interrupt_event used by the Process.waitFor logic. To facilitate > clearing via Thread.interrupted() I had to introduce a native method > that is a no-op except on Windows. This seemed the cheapest and least > intrusive means to achieve this. > > Other changes revolve around the fact we used to have an intrinsic for > Thread.isInterrupted and that is not needed any more. So we strip some > code out of C1/C2. > > The changes in the JVMCI/Graal code are a bit more far reaching as > entire classes disappear. I've cc'd Doug and Tom at Vladimir's request > so that they can comment on the JVMCI changes and whether I have gone > too far or not far enough. There are a bunch of tests for interruption > in JVMCI that could potentially be deleted if they are only intended to > test the JVMCI handling of interrupt: > > ./jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.jtt/src/org/graalvm/compiler/jtt/threads/Thread_isInterrupted*.java > > Testing: > - Tiers 1-3 on all Oracle platforms > - Focused testing on Linux x64: > - Stress runs of JSR166TestCase > - Anything that seems to use interrupt(): > - JDK > - java/lang/Thread > - java/util/concurrent > - jdk/internal/loader/InterruptedClassLoad.java > - javax/management > - java/nio/file/Files > - java/nio/channels > - java/net/Socket/Timeouts.java > - java/lang/Runtime/shutdown/ > - java/lang/ProcessBuilder/Basic.java > - com/sun/jdi/ > - Hotspot > - vmTestbase/nsk/monitoring/ > - vmTestbase/nsk/jdwp > - vmTestbase/nsk/jdb/ > - vmTestbase/nsk/jdi/ > - vmTestbase/nsk/jvmti/ > - runtime/Thread > - serviceability/jvmti/ > - serviceability/jdwp > - serviceability/sa > > Thanks, > David > -
Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination
Thanks for the review Dan! Fixed the nit in thread.c Thanks, David On 30/10/2019 6:51 am, Daniel D. Daugherty wrote: On 10/29/19 3:42 AM, David Holmes wrote: Bug: https://bugs.openjdk.java.net/browse/JDK-8229516 CSR: https://bugs.openjdk.java.net/browse/JDK-8232676 (already approved) webrev: http://cr.openjdk.java.net/~dholmes/8229516/webrev/ make/hotspot/symbols/symbols-unix No comments. src/hotspot/os/windows/osThread_windows.cpp No comments. src/hotspot/os/windows/osThread_windows.hpp No comments. src/hotspot/share/aot/aotCodeHeap.cpp Skipped. src/hotspot/share/classfile/javaClasses.cpp No comments. src/hotspot/share/classfile/javaClasses.hpp No comments. src/hotspot/share/classfile/vmSymbols.cpp No comments. src/hotspot/share/classfile/vmSymbols.hpp No comments. src/hotspot/share/include/jvm.h No comments. src/hotspot/share/jvmci/jvmciRuntime.cpp src/hotspot/share/jvmci/jvmciRuntime.hpp src/hotspot/share/jvmci/vmStructs_jvmci.cpp Skipped these three. src/hotspot/share/oops/oop.hpp No comments. src/hotspot/share/oops/oop.inline.hpp No comments. src/hotspot/share/opto/c2compiler.cpp No comments. src/hotspot/share/opto/library_call.cpp No comments. src/hotspot/share/prims/jvm.cpp No comments. src/hotspot/share/prims/jvmtiEnv.cpp No comments. src/hotspot/share/prims/jvmtiEnvBase.cpp No comments. src/hotspot/share/runtime/osThread.cpp No comments. src/hotspot/share/runtime/osThread.hpp No comments. src/hotspot/share/runtime/thread.cpp No comments. src/hotspot/share/runtime/vmStructs.cpp No comments. src/java.base/share/classes/java/lang/Thread.java No comments. src/java.base/share/native/libjava/Thread.c L76: // Need to reset the interrupt event used by Process.waitFor L77: ResetEvent((HANDLE) JVM_GetThreadInterruptEvent()); nit - the indent in JDK .c files is four spaces. src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/OSThread.java No comments. src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/HotSpotMethodSubstitutionTest.java src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/ThreadSubstitutions.java Skipped these 5 graal files. Thumbs up! I only have one nit above and don't need to see another webrev if you decide to fix it. Thanks for being so thorough on your testing. Dan This cross-cuts core-libs, hotspot-runtime and the JIT compilers, but only in small pieces each. There is also a small touch to serviceability code. This change is "simply" moving the "interrupted" field out of the osThread and into the java.lang.Thread so that it can be set independently of whether the thread is alive (and to make things easier for loom in the near future). It is very straightforward: - old scheme - interrupted field is in osThread - VM can read/write directly - Java code calls into VM to read/write - new scheme - interrupted field is in java.lang.Thread - VM has to use javaCalls to read/write "directly" - Java code can read/write directly No changes to any of the semantics regarding the actual interrupt mechanism. Special thanks to Patricio for tracking down a bug I had introduced in that regard! Special Note (Hi Roger!): on windows we still need to set/clear the _interrupt_event used by the Process.waitFor logic. To facilitate clearing via Thread.interrupted() I had to introduce a native method that is a no-op except on Windows. This seemed the cheapest and least intrusive means to achieve this. Other changes revolve around the fact we used to have an intrinsic for Thread.isInterrupted and that is not needed any more. So we strip some code out of C1/C2. The changes in the JVMCI/Graal code are a bit more far reaching as entire classes disappear. I've cc'd Doug and Tom at Vladimir's request so that they can comment on the JVMCI changes and whether I have gone too far or not far enough. There are a bunch of tests for interruption in JVMCI that could potentially be deleted if they are only intended to test the JVMCI handling of interrupt: ./jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.jtt/src/org/graalvm/compiler/jtt/threads/Thread_isInterrupted*.java Testing: - Tiers 1-3 on all Oracle platforms - Focused testing on Linux x64: - Stress runs of JSR166TestCase - Anything that seems to use interrupt():
Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination
On 30/10/2019 4:05 am, serguei.spit...@oracle.com wrote: Hi David, The fix looks good to me. Thanks Serguei! David I did not pay much attention to the Graal related changes though. The test coverage for Serviceability is complete. Running java/lang/instrument tests is not necessary. Thanks, Serguei On 10/29/19 00:42, David Holmes wrote: Bug: https://bugs.openjdk.java.net/browse/JDK-8229516 CSR: https://bugs.openjdk.java.net/browse/JDK-8232676 (already approved) webrev: http://cr.openjdk.java.net/~dholmes/8229516/webrev/ This cross-cuts core-libs, hotspot-runtime and the JIT compilers, but only in small pieces each. There is also a small touch to serviceability code. This change is "simply" moving the "interrupted" field out of the osThread and into the java.lang.Thread so that it can be set independently of whether the thread is alive (and to make things easier for loom in the near future). It is very straightforward: - old scheme - interrupted field is in osThread - VM can read/write directly - Java code calls into VM to read/write - new scheme - interrupted field is in java.lang.Thread - VM has to use javaCalls to read/write "directly" - Java code can read/write directly No changes to any of the semantics regarding the actual interrupt mechanism. Special thanks to Patricio for tracking down a bug I had introduced in that regard! Special Note (Hi Roger!): on windows we still need to set/clear the _interrupt_event used by the Process.waitFor logic. To facilitate clearing via Thread.interrupted() I had to introduce a native method that is a no-op except on Windows. This seemed the cheapest and least intrusive means to achieve this. Other changes revolve around the fact we used to have an intrinsic for Thread.isInterrupted and that is not needed any more. So we strip some code out of C1/C2. The changes in the JVMCI/Graal code are a bit more far reaching as entire classes disappear. I've cc'd Doug and Tom at Vladimir's request so that they can comment on the JVMCI changes and whether I have gone too far or not far enough. There are a bunch of tests for interruption in JVMCI that could potentially be deleted if they are only intended to test the JVMCI handling of interrupt: ./jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.jtt/src/org/graalvm/compiler/jtt/threads/Thread_isInterrupted*.java Testing: - Tiers 1-3 on all Oracle platforms - Focused testing on Linux x64: - Stress runs of JSR166TestCase - Anything that seems to use interrupt(): - JDK - java/lang/Thread - java/util/concurrent - jdk/internal/loader/InterruptedClassLoad.java - javax/management - java/nio/file/Files - java/nio/channels - java/net/Socket/Timeouts.java - java/lang/Runtime/shutdown/ - java/lang/ProcessBuilder/Basic.java - com/sun/jdi/ - Hotspot - vmTestbase/nsk/monitoring/ - vmTestbase/nsk/jdwp - vmTestbase/nsk/jdb/ - vmTestbase/nsk/jdi/ - vmTestbase/nsk/jvmti/ - runtime/Thread - serviceability/jvmti/ - serviceability/jdwp - serviceability/sa Thanks, David -
Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination
Hi Remi, On 30/10/2019 7:38 am, Remi Forax wrote: Hi David, in java.lang.Thread interrupt0 should be renamed to setInterruptEvent, because what it does now There is only an "interrupt event" on Windows. What interrupt0 does is issue all the necessary unparks so that blocked threads will wake up and recheck if they were interrupted. So I don't think a rename is needed. and I don't really understand the comment in interrupted(), if a thread is interrupted by two other threads calling interrupt(), you will loose an interrupt anyway. Suppose a Thread is checking interrupted() ahead of a blocking operation, it reads the interrupted field and sees that it is false. Then another thread interrupts it, setting the field to true and issuing the unparks. The interrupted() call continues and sets the interrupted field to false, and then returns false so the blocking operation proceeds. Because of the unparks the blocking call returns immediately. The thread then checks again to see if it was interrupted, but the field is false and so it re-parks (treating this as a spurious wakeup). But the thread actually was interrupted and should have thrown InterruptedException. The interrupt has been lost. By only clearing the interrupted field if it was set we avoid this problem. This is the logic the VM code has been implementing for 20 years, so the same logic is needed in the Java code. I added the additional comment to the VM code to make it clearer. Hope that clarifies things. Thanks, David - Rémi - Mail original - De: "David Holmes" À: "hotspot-runtime-dev" , "hotspot compiler" , "core-libs-dev" , "Doug Simon" , "TOM.RODRIGUEZ" , "serviceability-dev" , "Roger Riggs" Envoyé: Mardi 29 Octobre 2019 08:42:08 Objet: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination Bug: https://bugs.openjdk.java.net/browse/JDK-8229516 CSR: https://bugs.openjdk.java.net/browse/JDK-8232676 (already approved) webrev: http://cr.openjdk.java.net/~dholmes/8229516/webrev/ This cross-cuts core-libs, hotspot-runtime and the JIT compilers, but only in small pieces each. There is also a small touch to serviceability code. This change is "simply" moving the "interrupted" field out of the osThread and into the java.lang.Thread so that it can be set independently of whether the thread is alive (and to make things easier for loom in the near future). It is very straightforward: - old scheme - interrupted field is in osThread - VM can read/write directly - Java code calls into VM to read/write - new scheme - interrupted field is in java.lang.Thread - VM has to use javaCalls to read/write "directly" - Java code can read/write directly No changes to any of the semantics regarding the actual interrupt mechanism. Special thanks to Patricio for tracking down a bug I had introduced in that regard! Special Note (Hi Roger!): on windows we still need to set/clear the _interrupt_event used by the Process.waitFor logic. To facilitate clearing via Thread.interrupted() I had to introduce a native method that is a no-op except on Windows. This seemed the cheapest and least intrusive means to achieve this. Other changes revolve around the fact we used to have an intrinsic for Thread.isInterrupted and that is not needed any more. So we strip some code out of C1/C2. The changes in the JVMCI/Graal code are a bit more far reaching as entire classes disappear. I've cc'd Doug and Tom at Vladimir's request so that they can comment on the JVMCI changes and whether I have gone too far or not far enough. There are a bunch of tests for interruption in JVMCI that could potentially be deleted if they are only intended to test the JVMCI handling of interrupt: ./jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.jtt/src/org/graalvm/compiler/jtt/threads/Thread_isInterrupted*.java Testing: - Tiers 1-3 on all Oracle platforms - Focused testing on Linux x64: - Stress runs of JSR166TestCase - Anything that seems to use interrupt(): - JDK - java/lang/Thread - java/util/concurrent - jdk/internal/loader/InterruptedClassLoad.java - javax/management - java/nio/file/Files - java/nio/channels - java/net/Socket/Timeouts.java - java/lang/Runtime/shutdown/ - java/lang/ProcessBuilder/Basic.java - com/sun/jdi/ - Hotspot - vmTestbase/nsk/monitoring/ - vmTestbase/nsk/jdwp - vmTestbase/nsk/jdb/ - vmTestbase/nsk/jdi/ - vmTestbase/nsk/jvmti/ - runtime/Thread - serviceability/jvmti/ - serviceability/jdwp - serviceability/sa Thanks, David -
Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination
Hi Doug, Your proposed patch wasn't quite right. I made some adjustments but I'm still having issues with the test, HotSpotMethodSubstitutionTest.testThreadSubstitutions, as I don't see how to make the test execution conditional on the VM config. Updated webrev: http://cr.openjdk.java.net/~dholmes/8229516/webrev.v2/ Thanks, David - On 29/10/2019 7:14 pm, Doug Simon wrote: On 29 Oct 2019, at 10:12, David Holmes wrote: Hi Doug, Thanks for taking a look so quickly! :) On 29/10/2019 6:55 pm, Doug Simon wrote: Hi David, Since Graal needs to work against multiple JVMCI versions (and VM versions!), the Graal changes should only disable the intrinsic when the relevant VM config is missing: So to be clear I should revert all the Graal file changes I made and just apply the patch below? Yes please. -Doug diff --git a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java index 0752a621215..baa2136a6ba 100644 --- a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java +++ b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java @@ -315,7 +315,7 @@ public class GraalHotSpotVMConfig extends GraalHotSpotVMConfigBase { public final int javaThreadAnchorOffset = getFieldOffset("JavaThread::_anchor", Integer.class, "JavaFrameAnchor"); public final int javaThreadShouldPostOnExceptionsFlagOffset = getFieldOffset("JavaThread::_should_post_on_exceptions_flag", Integer.class, "int", Integer.MIN_VALUE); public final int threadObjectOffset = getFieldOffset("JavaThread::_threadObj", Integer.class, "oop"); -public final int osThreadOffset = getFieldOffset("JavaThread::_osthread", Integer.class, "OSThread*"); +public final int osThreadOffset = getFieldOffset("JavaThread::_osthread", Integer.class, "OSThread*", Integer.MAX_VALUE); public final int threadIsMethodHandleReturnOffset = getFieldOffset("JavaThread::_is_method_handle_return", Integer.class, "int"); public final int threadObjectResultOffset = getFieldOffset("JavaThread::_vm_result", Integer.class, "oop"); public final int jvmciCountersThreadOffset = getFieldOffset("JavaThread::_jvmci_counters", Integer.class, "jlong*"); diff --git a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java index 6b37fff083d..ffc8032d2b0 100644 --- a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java +++ b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java @@ -441,7 +441,9 @@ public class HotSpotGraphBuilderPlugins { } }); -r.registerMethodSubstitution(ThreadSubstitutions.class, "isInterrupted", Receiver.class, boolean.class); +if (config.osThreadOffset != Integer.MAX_VALUE) { +r.registerMethodSubstitution(ThreadSubstitutions.class, "isInterrupted", Receiver.class, boolean.class); +} } public static final String reflectionClass; diff --git a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java index 1d694fae2a4..8500c4de115 100644 --- a/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java +++ b/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/HotSpotReplacementsUtil.java @@ -308,6 +308,7 @@ public class HotSpotReplacementsUtil { @Fold public static int osThreadOffset(@InjectedParameter GraalHotSpotVMConfig config) { +assert config.instanceKlassInitThreadOffset != Integer.MAX_VALUE; return config.osThreadOffset; } All the other JVMCI changes look good to me. -Doug On 29 Oct 2019, at 08:42, David Holmes wrote: Bug: https://bugs.openjdk.java.net/browse/JDK-8229516 CSR: https://bugs.openjdk.java.net/browse/JDK-8232676 (already approved) webrev: http://cr.openjdk.java.net/~dholmes/8229516/webrev/ This cross-cuts core-libs, hotspot-runtime and the JIT compilers, but only in small pieces each. There is also a small touch to serviceability code. This change is "simply" moving the "interrupted" field out of the osThread and into the java.lang.Thread so that it can be set independently of whether the thread is alive (and to make things easier for loom in the near future). It is very straightforward: - old scheme - interrupted field is in osThread - VM can read/write di