Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v2]
On Mon, 27 Sep 2021 23:50:38 GMT, Phil Race wrote: >> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set >> the name of the application in the system menu bar. >> >> Because this set shortly after the VM is running, it causes a thread safety >> issue described in https://bugs.openjdk.java.net/browse/JDK-8270549 >> >> Since the AWT already looks for the system property >> "apple.awt.application.name" for this same purpose, >> we can set that instead of the env. var and avoid the threading issue. >> >> This trades the JAVA_MAIN_CLASS_ pollution of the environment for >> setting a system property which is visible to apps as well but it seems a >> reasonable trade-off since we already (implicitly) support this variable >> anyway in preference to the env. var. > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher > code src/java.base/macosx/native/libjli/java_md_macosx.m line 842: > 840: * of the app as it appears in the system menu bar. > 841: * > 842: * Not idea if how much external code ever sets it, but use it if > set, else "Not idea if" -> "No idea of" - PR: https://git.openjdk.java.net/jdk/pull/5724
Re: RFR: JDK-8267936: PreserveAllAnnotations option doesn't expose the annotation to Java code [v4]
On Mon, 31 May 2021 06:06:37 GMT, Jaroslav Tulach wrote: >> This PR exposes runtime invisible annotations via `Class.getAnnotation` when >> `-XX:+PreserveAllAnnotations` option is passed to the JVM. >> >> Existing `-XX:+PreserveAllAnnotations` option can be very useful for code >> that needs to access annotations with `RetentionPolicy.CLASS` without the >> need to parse the .class files manually. While the >> RuntimeInvisibleAnnotations are kept in the runtime, they are not visible >> via java.lang.reflect API. I assume that's just an omission. >> >> This PR provides a new test and a fix to make `Class.getAnnotation(...)` >> useful when `-XX:+PreserveAllAnnotations` option is on. > > Jaroslav Tulach has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. There is [support for parsing class files in Graal](https://github.com/oracle/graal/blob/89e4cfc7ae/compiler/src/org.graalvm.compiler.replacements/src/org/graalvm/compiler/replacements/classfile/ClassfileBytecodeProvider.java#L45-L47) which exists to avoids all bytecode preprocessing and instrumentation that may be performed on the VM internal bytecode representation. This is only done for trusted bytecode that comes from Graal classes (e.g. [Java source code snippets](https://github.com/oracle/graal/blob/89e4cfc7ae/compiler/src/org.graalvm.compiler.replacements/src/org/graalvm/compiler/replacements/AllocationSnippets.java#L50-L72) used to express compiler (sub)graphs as Java source code). - PR: https://git.openjdk.java.net/jdk/pull/4245
Re: RFR: 8264806: Remove the experimental JIT compiler [v2]
On Tue, 27 Apr 2021 19:07:45 GMT, Igor Ignatyev wrote: > > I guess this should really be named `isJVMCICompilerEnabled` now and the > > `vm.graal.enabled` predicate renamed to `vm.jvmcicompiler.enabled` but > > maybe that's too big a change (or can be done later). > > @dougxc, I don't think that we should (or even can) rename it to > `vm.jvmcicompiler.enabled`. although the value of the property is indeed > `true` whenever a jvmci compiler is enabled, it is used to filter out tests > that are incompatible with a particular compiler -- graal. so what we can do > is to change `requires/VMProps.java` to set `vm.graal.enabled` only if the > jvmci compiler is indeed graal, but on the other hand, we haven't heard > anyone complaining that the test coverage for their jvmci compiler has been > reduced, so I don't see much of the benefit here. > > -- Igor Yes, we should just it as is until a second JVMCI compiler shows up. - PR: https://git.openjdk.java.net/jdk/pull/3421
Re: RFR: 8264806: Remove the experimental JIT compiler [v3]
On Mon, 12 Apr 2021 22:10:06 GMT, Vladimir Kozlov wrote: >> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related >> to Java-based JIT compiler (Graal) from JDK: >> >> - `jdk.internal.vm.compiler` — the Graal compiler >> - `jdk.internal.vm.compiler.management` — Graal's `MBean` >> - `test/hotspot/jtreg/compiler/graalunit` — Graal's unit tests >> >> Remove Graal related code in makefiles. >> >> Note, next two `module-info.java` files are preserved so that the JVMCI >> module `jdk.internal.vm.ci` continues to build: >> >> src/jdk.internal.vm.compiler/share/classes/module-info.java >> src/jdk.internal.vm.compiler.management/share/classes/module-info.java >> >> >> @AlanBateman suggested that we can avoid it by using Module API to export >> packages at runtime . It requires changes in GraalVM's JVMCI too so I will >> file followup RFE to implement it. >> >> Tested hs-tier1-4 > > Vladimir Kozlov has updated the pull request incrementally with one > additional commit since the last revision: > > Restore Compiler::isGraalEnabled() Approved. - PR: https://git.openjdk.java.net/jdk/pull/3421
Re: RFR: 8264806: Remove the experimental JIT compiler [v2]
On Sun, 11 Apr 2021 10:25:47 GMT, Doug Simon wrote: >> Vladimir Kozlov has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains three additional >> commits since the last revision: >> >> - Restore Graal Builder image makefile >> - Merge latest changes from branch 'JDK-8264805' into JDK-8264806 >> - 8264806: Remove the experimental JIT compiler > > We would definitely like to be able to continue testing of GraalVM with the > JDK set of jtreg tests. So keeping `Compiler::isGraalEnabled()` working like > it does today is important. > @dougxc I restored Compiler::isGraalEnabled(). Thanks. I guess this should really be named `isJVMCICompilerEnabled` now and the `vm.graal.enabled` predicate renamed to `vm.jvmcicompiler.enabled` but maybe that's too big a change (or can be done later). - PR: https://git.openjdk.java.net/jdk/pull/3421
Re: RFR: 8264806: Remove the experimental JIT compiler [v2]
On Sat, 10 Apr 2021 17:41:05 GMT, Vladimir Kozlov wrote: >> Marked as reviewed by iignatyev (Reviewer). > > Thank you, Igor. I filed https://bugs.openjdk.java.net/browse/JDK-8265032 We would definitely like to be able to continue testing of GraalVM with the JDK set of jtreg tests. So keeping `Compiler::isGraalEnabled()` working like it does today is important. - PR: https://git.openjdk.java.net/jdk/pull/3421
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v15]
On Wed, 11 Nov 2020 13:31:11 GMT, Jorn Vernee wrote: >> Where's the logic for the native thread to detach? We have a similar problem >> in libgraal. We have a [utility >> class](https://github.com/oracle/graal/blob/e4b9ab931940e1946f96f2015b937ba100384573/compiler/src/org.graalvm.compiler.core/src/org/graalvm/compiler/core/GraalServiceThread.java#L27-L32) >> for libgraal created threads (as opposed to VM created threads that call >> into libgraal) that call into the VM. The utility class takes care of >> [attaching](https://github.com/oracle/graal/blob/a913944a06425c25ccd6e4a90379938fcf7ea2cf/substratevm/src/com.oracle.svm.graal.hotspot.libgraal/src/com/oracle/svm/graal/hotspot/libgraal/LibGraalFeature.java#L749) >> and >> [detaching](https://github.com/oracle/graal/blob/a913944a06425c25ccd6e4a90379938fcf7ea2cf/substratevm/src/com.oracle.svm.graal.hotspot.libgraal/src/com/oracle/svm/graal/hotspot/libgraal/LibGraalFeature.java#L757) >> to/from the VM. > > I added a call to DetachCurrentThread here: > https://github.com/openjdk/jdk/pull/634/commits/719224ca9dc70fce6d28885acfb362fee715ebbd#diff-c084afc373a6ce95010a480ddc5ab79d3cb759b80e46102c212c2cbc948e2303R65 Ok. That makes for high overhead for each upcall on a non-attached native thread. I assume that's an edge case not worth optimizing? - PR: https://git.openjdk.java.net/jdk/pull/634
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v15]
On Mon, 9 Nov 2020 11:50:54 GMT, Jorn Vernee wrote:
>> src/hotspot/cpu/aarch64/universalUpcallHandler_aarch64.cpp line 99:
>>
>>> 97: if (thread == NULL) {
>>> 98: JavaVM_ *vm = (JavaVM *)(&main_vm);
>>> 99: vm -> functions -> AttachCurrentThreadAsDaemon(vm, &p_env, NULL);
>>
>> Style nit: don't put spaces around `->` operator.
>>
>> What is the context for this being called? It looks highly suspicious to
>> just attach the current thread to the VM this way.
>
> The context is a thread that is spawned by native code doing an upcall. We
> need to attach the thread to the VM first in that case. Normally this would
> be handled by the calling code, but in our case the calling code doesn't know
> it's calling into Java.
Where's the logic for the native thread to detach? We have a similar problem in
libgraal. We have a [utility
class](https://github.com/oracle/graal/blob/e4b9ab931940e1946f96f2015b937ba100384573/compiler/src/org.graalvm.compiler.core/src/org/graalvm/compiler/core/GraalServiceThread.java#L27-L32)
for libgraal created threads (as opposed to VM created threads that call into
libgraal) that call into the VM. The utility class takes care of
[attaching](https://github.com/oracle/graal/blob/a913944a06425c25ccd6e4a90379938fcf7ea2cf/substratevm/src/com.oracle.svm.graal.hotspot.libgraal/src/com/oracle/svm/graal/hotspot/libgraal/LibGraalFeature.java#L749)
and
[detaching](https://github.com/oracle/graal/blob/a913944a06425c25ccd6e4a90379938fcf7ea2cf/substratevm/src/com.oracle.svm.graal.hotspot.libgraal/src/com/oracle/svm/graal/hotspot/libgraal/LibGraalFeature.java#L757)
to/from the VM.
-
PR: https://git.openjdk.java.net/jdk/pull/634
Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination
> On 31 Oct 2019, at 05:30, David Holmes wrote:
>
> Hi Doug,
>
> On 30/10/2019 10:06 pm, Doug Simon wrote:
>>> On 30 Oct 2019, at 05:28, David Holmes >> <mailto:david.hol...@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/
Good catch. Sorry about that.
>
> 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/
> <http://cr.openjdk.java.net/~dholmes/8229516/webrev.v2/>
Looks good.
-Doug
Re: RFR: 8229516: Thread.isInterrupted() always returns false after thread termination
> On 30 Oct 2019, at 05:28, 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.
Like this:
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) {
+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 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 f
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:
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: [10] (S) RFR 8191788: add jdk.internal.vm.compiler to --limit-modules if -Djvmci.Compiler=graal is in the command line
> On 29 Nov 2017, at 10:05, Alan Bateman wrote: > > On 28/11/2017 23:51, Vladimir Kozlov wrote: >> http://cr.openjdk.java.net/~kvn/8191788/webrev.00/ >> https://bugs.openjdk.java.net/browse/JDK-8191788 >> >> This is redo of JDK-8190975 [1] fix which added jdk.internal.vm.compiler to >> tests which have --limit-modules option. Unfortunately tests start failing >> on SPARC where Graal (jdk.internal.vm.compiler) is not available. >> JDK-8191653 [2] reversed 8190975 changes to fix that problem. >> >> This fix adds jdk.internal.vm.compiler to --limit-modules list in JVM only >> when Graal is used: when it is explicitly specified with >> -Djvmci.Compiler=graal or in default case and when UseJVMCICompiler is true. >> >> I tested the fix with failed tests from JDK-8190975 which are mostly AppCDS >> tests in test/runtime/appcds/jigsaw and also >> test/jdk/java/lang/String/concat/WithSecurityManager.java >> >> I think this fix may break upgradeable status of Graal (for Oracle Labs >> version of Graal). >> But it should be fine since it is only used with --limit-modules which is >> not used by Labs. > If jdk.internal.vm.compiler is not observable on SPARC then shouldn't the > tests have `@requires jdk.internal.vm.compiler` and jtreg will skip the test > on that platform? > > Just asking as augmenting the value passed to --limit-modules is very > strange. It's normal for XX options to augment the set of modules that > resolved (+EnableJVMCI implies `--add-modules jdk.internal.vm.ci` for > example) but doing this for --limit-modules suggests the VM is doing > something to mask an issue with the way that the tests are run. As much as I understand the issue, I agree. This seems like something that should be addressed in the test(s) instead of in the VM. -Doug
Re: [10] RFR(M) 8182701: Modify JVMCI to allow Graal Compiler to expose platform MBean
> On 15 Sep 2017, at 14:32, Jaroslav Tulach wrote:
>
> Thanks for the review Mandy, Daniel. Now, that the consolidated JDK10
> repository is available, I have updated my webrev to its structure. In
> addition to that I addressed your comments:
>
> On úterý 12. září 2017 11:19:45 CEST mandy chung wrote:
>> ./make/common/Modules.gmk
>> Nit: can you move jdk.internal.vm.compiler.management to keep the
>> list in alphabetical order
>
> Inserted at appropriate place.
>
>> 199 # Filter out Graal specific modules if Graal build is disabled
>> 200
>> 201 ifeq ($(INCLUDE_GRAAL), false)
>> 202 MODULES_FILTER += jdk.internal.vm.compiler
>> 203 endif
>>
>> When will INCLUDE_GRAAL be set to false? I think
>> jdk.internal.vm.compiler.management should also be filtered if
>> jdk.internal.vm.compiler is disabled.
>
> That is probably true. Fixed.
>
>>
>> Is jdk.internal.vm.compiler and jdk.internal.vm.compiler.management
>> built for all platforms in JDK 10? If not,
>>jdk/src/java.management/share/classes/module-info.java may fail to
>> compile when jdk.internal.vm.compiler.management is not present. We
>> can consult with the build team when you find out what configuration
>> that jdk.internal.vm.compiler is not built.
>
> I haven't found configuration where jdk.internal.vm.compiler wouldn't be
> built.
> However I wasn't looking very extensively...
>
>> hotspot/src/jdk.internal.vm.compiler/share/classes/module-info.java 29
>> requires transitive jdk.internal.vm.ci;
>> do you get any error without this requires transitive?
>> jdk.internal.vm.compiler.management already requires
>> jdk.internal.vm.ci. I would think this requires transitive is not
>> necessary.
>
> Looks like this change isn't necessary. I am not sure what was the problem
> before, when I introduced it.
>
>> Is HotSpotGraalCompiler::mbean method necessary? In GraalMBeans.java
>>
>> 53 public static Object findGraalRuntimeBean() {
>> 54 JVMCIRuntime r = JVMCI.getRuntime();
>> 55 JVMCICompiler c = r.getCompiler();
>> 56 if (c instanceof HotSpotGraalCompiler) {
>> 57 return ((HotSpotGraalCompiler) c).mbean();
>> 58 }
>> 59 return null;
>> 60 }
>>
>> It seems that you can call HotspotGraalRuntime::mbean directly.
>
> I don't think I can. There is no way to get to HotspotGraalRuntime except
> asking the HotSpotGraalCompiler. The HotspotGraalRuntime isn't
> JVMCIRuntime...
> At least I think so
That is correct - you have to obtain a HotspotGraalRuntime from a
HotSpotGraalCompiler. There can be other HotspotGraalRuntime instances (e.g.,
Truffle has it's own HotSpotGraalCompiler/HotSpotGraalRuntime).
-Doug
Re: [10] RFR(M) 8182701: Modify JVMCI to allow Graal Compiler to expose platform MBean
> On 21 Aug 2017, at 10:48, Jaroslav Tulach wrote: > > Hello Doug & co., > I think your comments are about the old webrev. I hope most of them have been > (by a chance) addressed in the newest webrev 01: Indeed they are. Sorry again for missing the latest link when do the review ;-) -Doug
Re: [10] RFR(M) 8182701: Modify JVMCI to allow Graal Compiler to expose platform MBean
Hi Jaroslav,
In general, thanks for pushing through with this change.
I don't think JVMCIMXBeans should be in the hotspot-agnostic
jdk.vm.ci.services.internal package since it has a direct reference to
HotSpotJVMCIRuntime. I would suggest moving it to jdk.vm.ci.hotspot.
In HotSpotJVMCRuntime.java:
558 public String mbeanName() {
568 public Object mbean() {
Any reason these methods don't follow the conventions of other getter methods
in this class (i.e. getMBeanName() and getMBean())?
95 /** Name of the {@link MBeanInfo MBean} representing the internals
This should be:
/**
* Gets the name of the ...
Same comment for mbean() method. Also {@code null} is used in JVMCI instead of
null.
-Doug
> On 18 Aug 2017, at 20:49, Vladimir Kozlov wrote:
>
> Updated changes in all repos:
> http://cr.openjdk.java.net/~kvn/8182701/webrev.01/
>
> On 8/18/17 7:12 AM, Jaroslav Tulach wrote:
>
> Thanks for pushing me forward, Vladimir. Yes, the changes are still needed if
> we want the Graal compiler to expose its MBean in a lazy way. I am offering
> new webrev for review. It contains following changes:
>
> Per Mandy's suggestion I created new module jdk.vm.ci.management to bridge
> between
> JVMCI and jdk.management. Adding new module was a bit tricky, but with great
> help of Jan
> Lahoda I even managed to register it as a boot module.
>
> I renamed the JVMCIMXBean class and dropped X per Vladimir's advice. I fixed
> the non-standard location of JVMCIMXBean class.
>
> I changed the interface to use Map, so the compiler is able to expose more
> than a single bean.
>
> That's it. I am looking forward to your review comments.
> -jt
>
> Here are original changes for reference:
>
> http://cr.openjdk.java.net/~kvn/8182701/webrev.jdk/
> http://cr.openjdk.java.net/~kvn/8182701/webrev.hs/
>
> On 8/17/17 11:54 AM, Vladimir Kozlov wrote:
>> Hi Jaroslav,
>> What we should do with 8182701? Do you still need JVMCI changes?
>> Note, your changes to Graal [GR-5435] were integrated recently into JDK
>> (jdk10/hs):
>> https://bugs.openjdk.java.net/browse/JDK-8186158
>> Thanks,
>> Vladimir
>> On 8/14/17 10:06 AM, Jaroslav Tulach wrote:
>>> On čtvrtek 3. srpna 2017 17:03:39 CEST Jaroslav Tulach wrote:
On čtvrtek 27. července 2017 15:01:17 CEST Alan Bateman wrote:
> On 27/07/2017 10:07, Jaroslav Tulach wrote:
>> Yes, it seems like a desirable goal to let Graal compiler work with just
>> java.base. Is there a description how to build JDK9/10 with just
>> java.base
>> that I could follow and test against?
>
> You can use jlink to create a run-time image that only contains
> java.base (jlink --module-path $JAVA_HOME/jmods --add-modules java.base
> --output smalljre).
Status update: I've just tried to run Graal compiler against JDK9 with only
java.base and jdk.internal.vm.ci modules, and there are some problems. I
need to resolve them first before I provide updated version of my patch.
>>>
>>> FYI: As of
>>> https://github.com/graalvm/graal/commit/ca9071941a1be7f1a3725529ecc231ff621d5ed0
>>> the Graal compiler can run with java.base, jdk.unsupported and jdk.vm.ci
>>> only
>>> modules. But it wasn't easy, especially the
>>> http://wiki.apidesign.org/wiki/PropertyChangeListener required a bit of
>>> work.
>>>
>>> -jt
>>>
>>>
Re: [10] RFR(M) 8182701: Modify JVMCI to allow Graal Compiler to expose platform MBean
> On 25 Jul 2017, at 01:37, Mandy Chung wrote: > > Vladimir, > > I believe you don’t want to add the dependency from JVMCI to java.management. > Otherwise, JVMCI can’t run with only java.base. The dependency is unfortunate. Can you suggest how JVMCI platform beans could participate in platform bean registration without the dependency? > Is the MBean in jdk.internal.vm.compiler or in Lab’s Graal compiler? Anything in Lab’s Graal compiler is destined for JDK Graal so the distinction is only temporary at best. > > src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/services/src/jdk/vm/ci/services/internal/JVMCIMXBeans.java > - I suspect this file meant to be in > src/jdk.internal.vm.ci/share/classes/src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/services/internal/JVMCIMXBeans.java Not sure I follow - there is currently no such directory src/jdk.internal.vm.ci/share/classes/src -Doug >> On Jul 12, 2017, at 2:20 PM, Vladimir Kozlov >> wrote: >> >> https://bugs.openjdk.java.net/browse/JDK-8182701 >> webrev: >> http://cr.openjdk.java.net/~kvn/8182701/webrev.jdk/ >> http://cr.openjdk.java.net/~kvn/8182701/webrev.hs/ >> >> Contributed by Jaroslav Tulach. >> >> JDK itself contains quite a lot of platform MBeans which get registered "on >> demand". Graal compiler (jdk.internal.vm.compiler) provides its own MBean(s) >> - however currently there is no way to register it "on demand". JDK9 already >> contains support for collecting platform MBeans from various modules. We >> just need to expose Graal MBean through JVMCI. >> >> Thanks, >> Vladimir >
Re: [9] RFR(M): 8076112: Add @HotSpotIntrinsicCandidate annotation to indicate methods for which Java Runtime has intrinsics
> On Jun 29, 2015, at 10:47 PM, John Rose wrote: > > On Jun 29, 2015, at 10:48 AM, Doug Simon wrote: >> >> As I understand it, part of this change is to split intrinsification into >> one method that does the checks that then calls a second method which the VM >> may intrinsify on the assumption all checks have been performed by the first >> method. What’s to prevent a direct call to the latter via reflection that >> by-passes the first method? > > The answer is simple: We mark the dangerous method private. > > I assume you are thinking about setAccessible, but if that's allowed there's > nothing to prevent people from taking whole system down in a hundred > different ways. At that point having a surprising intrinsic is the least of > our problems. Ok, thanks for the clarification John. I’ll admit to occasionally forgetting that setAccessible is as unsafe as, well, Unsafe. >> I understand that writing the checking logic in Java is desirable. > > Indeed, that is the key compromise here. Coding the required checks in > assembly code or compiler IR is (as we all know) a losing battle. You need > Maxine/Graal snippets or real Java code to get it right. Snippets certainly make it easier. I’m surprised though that existing C2 IR logic for expressing runtime checks cannot be easily leveraged for some intrinsics. My perspective may shift rapidly though were I tasked with doing it ;-) -Doug
Re: [9] RFR(M): 8076112: Add @HotSpotIntrinsicCandidate annotation to indicate methods for which Java Runtime has intrinsics
> On Jun 29, 2015, at 3:01 PM, Andrew Haley wrote: > > On 06/29/2015 01:38 PM, Doug Simon wrote: > >> I seems just plain wrong for an intrinsic to not implement the same >> semantics as the intrinsified method. I would expect an intrinsic to >> perform all necessary runtime checks and only have the compiler omit >> them if it can prove they are unnecessary. If all intrinsics obeyed >> this contract, then there’s no need for the >> @HotSpotIntrinsicCandidate annotation from a semantics >> perspective. And in terms of the keeping HotSpot in sync with the >> JDK, the responsibility should fall entirely on HotSpot to check >> that its intrinsics correspond to existing methods. > > Don't you think you're being rather idealistic? The other side of the > argument is that it's much easier to write and maintain the > arg-checking code if it's written in Java, and it also has the > advantage that it benefits from profile data to guide the JIT. As I understand it, part of this change is to split intrinsification into one method that does the checks that then calls a second method which the VM may intrinsify on the assumption all checks have been performed by the first method. What’s to prevent a direct call to the latter via reflection that by-passes the first method? I understand that writing the checking logic in Java is desirable. Indeed, it’s possible: http://hg.openjdk.java.net/graal/graal/file/tip/graal/com.oracle.graal.hotspot/src/com/oracle/graal/hotspot/replacements/AESCryptSubstitutions.java#l95 Adding these checks did not impact the score for the SPECjvm2008 crypto.aes benchmark on Graal. I’m not sure if this performance non-impact holds for all existing intrinsics but unless one can guarantee an intrinsified method will only be executed after the necessary checks have been performed, one is asking for trouble. Maybe hiding the intrinsifiable methods from reflection is sufficient? -Doug
Re: [9] RFR(M): 8076112: Add @HotSpotIntrinsicCandidate annotation to indicate methods for which Java Runtime has intrinsics
> On Jun 29, 2015, at 12:41 PM, Zoltán Majó wrote: > > Hi, > > > On 06/29/2015 11:45 AM, Andrew Haley wrote: >> Hi, >> >> On 29/06/15 10:41, Zoltán Majó wrote: >>> On 06/27/2015 10:05 AM, Andrew Haley wrote: On 25/06/15 12:49, Zoltán Majó wrote: > Problem: There is need to indicate Java methods that are potentially > intrinsified by JVM. It's a great idea but is it a good name? HotSpot is not the only Java VM. Do we expect people from to come along and add J9IntrinsicCandidate, CACAOIntrinsicCandidate, and so on? >>> thank you bringing up this issue. >>> >>> The name HotSpotIntrinsicCandidate resulted from a private discussion >>> with Joe Darcy, Brian Goetz, and John Rose. The reason this name was >>> picked is to make clear that a marked method's interaction with the VM >>> (specifically with the HotSpot VM implementation) needs special attention. >> OK, cool. So has any thought been given to the other VMs? Do you >> expect that, say, J9 will use the HotSpotIntrinsicCandidate >> annotattion, or do you expect we will have similar annotations for >> each VM which uses OpenJDK libraries? Or is the need for this >> annotation totally HotSpot-specific? > > the need for this annotation resulted from the way HotSpot handles > intrinsics. Here are the two main reasons: > > (1) Intrinsics in the HotSpot VM omit some checks (typically null checks and > array bounds checks) that are instead performed in the JDK code. If HotSpot > intrinsic code is changed, the matching JDK code must be changed as well (and > vice versa). Otherwise we might run into correctness problems (e.g., the > HotSpot intrinsic and the JDK method have different semantics) and/or > performance problems (HotSpot suddenly not intrinsify a method because, e.g., > the method's signature has changed and HotSpot's intrinsic list was not > updated accordingly). Annotating intrinsified methods makes it less likely > that a "mismatch" between a JDK method and its HotSpot-level intrinsic > counterpart can be introduced. I seems just plain wrong for an intrinsic to not implement the same semantics as the intrinsified method. I would expect an intrinsic to perform all necessary runtime checks and only have the compiler omit them if it can prove they are unnecessary. If all intrinsics obeyed this contract, then there’s no need for the @HotSpotIntrinsicCandidate annotation from a semantics perspective. And in terms of the keeping HotSpot in sync with the JDK, the responsibility should fall entirely on HotSpot to check that its intrinsics correspond to existing methods. > (2) With the newly added CheckIntrinsic flag, HotSpot verifies if all > annotated methods are backed by intrinsics at the VM level and that all > intrinsics are marked appropriately in the JDK. > > Other VM implementations will most likely intrinsify a different set of > methods. So, if those methods were marked with the same annotation as HotSpot > is looking for, it would be difficult for HotSpot to check the match between > intrinsics and the JDK code they replace (Reason 2 from above). Also, if a > JDK method is updated for which VM_A but not VM_B defines and intrinsic, only > VM A's intrinsic code must be updated to match the JDK code, so it is maybe > better to mark clearly which VM implementation intrinsifies an annotated > method. > > So, the current design would require introducing a similar annotation for > every VM that decided to implement what we just proposed for HotSpot with the > current changeset. That is true which is a great reason to avoid an annotation altogether if possible. -Doug
