RFR: 8229516: Thread.isInterrupted() always returns false after thread termination
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Thu Oct 31 00:36:29 UTC 2019
Hi David,
The update looks good.
Thanks,
Serguei
On 10/29/19 9:28 PM, David Holmes wrote:
> 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 <david.holmes at oracle.com> 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 <david.holmes at oracle.com>
>>>>> 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
>>>>> -----
>>
More information about the hotspot-runtime-dev
mailing list