RFR: 8229516: Thread.isInterrupted() always returns false after thread termination
David Holmes
david.holmes at oracle.com
Thu Oct 31 04:30:16 UTC 2019
Hi Doug,
On 30/10/2019 10:06 pm, Doug Simon wrote:
>
>
>> On 30 Oct 2019, at 05:28, David Holmes <david.holmes at oracle.com> 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.
>
> Like this:
Thanks for that! One alteration below ...
> diff --git a/compiler/src/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/HotSpotMethodSubstitutionTest.java b/compiler/src/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/HotSpotMethodSubstitutionTest.java
> index 96e7d979d36..3c928b86961 100644
> --- a/compiler/src/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/HotSpotMethodSubstitutionTest.java
> +++ b/compiler/src/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/HotSpotMethodSubstitutionTest.java
> @@ -28,12 +28,13 @@ import java.lang.invoke.ConstantCallSite;
> import java.lang.invoke.MethodHandles;
> import java.lang.invoke.MethodType;
>
> -import org.graalvm.compiler.nodes.IfNode;
> -import org.junit.Test;
> -
> import org.graalvm.compiler.api.directives.GraalDirectives;
> import org.graalvm.compiler.api.replacements.MethodSubstitution;
> +import org.graalvm.compiler.hotspot.GraalHotSpotVMConfig;
> +import org.graalvm.compiler.hotspot.HotSpotBackend;
> +import org.graalvm.compiler.nodes.IfNode;
> import org.graalvm.compiler.replacements.test.MethodSubstitutionTest;
> +import org.junit.Test;
>
> /**
> * Tests HotSpot specific {@link MethodSubstitution}s.
> @@ -133,13 +134,18 @@ public class HotSpotMethodSubstitutionTest extends MethodSubstitutionTest {
>
> @Test
> public void testThreadSubstitutions() {
> + GraalHotSpotVMConfig config = ((HotSpotBackend) getBackend()).getRuntime().getVMConfig();
> testGraph("currentThread");
> - assertInGraph(testGraph("threadIsInterrupted", "isInterrupted", true), IfNode.class);
> - assertInGraph(testGraph("threadInterrupted", "isInterrupted", true), IfNode.class);
> + if (config.osThreadOffset != Integer.MAX_VALUE) {
s/osThreadOffset/osThreadInterruptedOffset/
This change removes the interrupted field from osThread but not osThread
itself. Though as osThread is only used to then access the interrupted
field, I could both the same. Here's updated webrev showing that:
http://cr.openjdk.java.net/~dholmes/8229516/webrev.v2/
All tests under compiler/graalunit now pass.
Thanks,
David
-----
> + assertInGraph(testGraph("threadIsInterrupted", "isInterrupted", true), IfNode.class);
> + assertInGraph(testGraph("threadInterrupted", "isInterrupted", true), IfNode.class);
> + }
>
> Thread currentThread = Thread.currentThread();
> test("currentThread", currentThread);
> - test("threadIsInterrupted", currentThread);
> + if (config.osThreadOffset != Integer.MAX_VALUE) {
> + test("threadIsInterrupted", currentThread);
> + }
> }
>
> @SuppressWarnings("all")
>
>>
>> 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