RFR: 8229516: Thread.isInterrupted() always returns false after thread termination
David Holmes
david.holmes at oracle.com
Thu Oct 31 01:34:06 UTC 2019
Thanks Serguei!
David
On 31/10/2019 10:36 am, serguei.spitsyn at oracle.com wrote:
> 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 core-libs-dev
mailing list