Quantcast

RFR: 8177845: Need a mechanism to load Graal

classic Classic list List threaded Threaded
25 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR: 8177845: Need a mechanism to load Graal

Doug Simon @ Oracle
Please review these changes that make jdk.internal.vm.compiler an upgradable compiler.
The primary requirement for this is to remove all usage of JDK internals from Graal.
While this most involves changes in Graal, there remain a few capabilities that must
be exposed via JVMCI. Namely:

1. Graal needs a copy of jdk.internal.misc.VM.savedProps so that it can Graal initialize
   can use system properties that are guaranteed not to have been modified by application
   code (Graal initialization is lazy and may occur after application code has been run).

2. Truffle needs to be able to trigger JVMCI initialization so that it can select the Graal
   optimized implementation of the Truffle runtime.

These capabilities have been added to jdk.vm.ci.services.Services. A comment has
also been added to this class to denote the current methods to be removed
in a subsequent bug to update the JDK copy of Graal with the upstream version which
no longer uses the methods. The methods destined for removal are:

exportJVMCITo(Class<?> requestor)
load(Class<S> service)
loadSingle(Class<S> service, boolean required)

The first one is no longer needed as JVMCI exports itself to Graal service providers
it loads via private API and Graal re-exports[1] JVMCI to any module the extends Graal.
The latter 2 are no longer required as Graal uses the standard ServiceLoader. This API
is only useful for JVMCI-8 where a hidden JVMCI class loader is used.

These changes have been tested against upstream Graal. To make the Graal tests pass, 2 extra
minor changes were required:

1. In JDK-8177663, HotSpotMemoryAccessProviderImpl::checkRead was added and throws IllegalArgumentException
   on all error paths except one which throws AssertionError. The latter was a mistake and has been
   fixed to also throw IllegalArgumentException.
2. An invalid assertion has been removed from HotSpotResolvedObjectTypeImpl::isDefinitelyResolvedWithRespectTo.
   Up to JDK 8, it was true that all classes in java.* packages were loaded by the boot class loader.
   This is no longer true in JDK 9 with classes like java.sql being loaded by platform class loader.

As hinted above, a subsequent bug will be required to pull in the latest upstream Graal and
remove use of JDK internal API from the jdk.aot module. The qualified exports to
jdk.vm.internal.compiler and jdk.aot can then be removed.

-Doug

https://bugs.openjdk.java.net/browse/JDK-8177845
http://cr.openjdk.java.net/~dnsimon/8177845/

[1] http://openjdk.java.net/projects/jigsaw/spec/issues/#IndirectQualifiedReflectiveAccess
   


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Mandy Chung

> On Apr 18, 2017, at 3:13 PM, Doug Simon <[hidden email]> wrote:
>
> Please review these changes that make jdk.internal.vm.compiler an upgradable compiler.
> :
> http://cr.openjdk.java.net/~dnsimon/8177845/

Thanks for making this change.  This would simplify the way to replace JDK’s graal with the lab graal.  A couple of comments:

jdk/internal/misc/VM.java

 161      * Note that the saved system properties do not include
 162      * the ones set by sun.misc.Version.init().

sun.misc.Version is no longer present in JDK 9.  Renamed to java.lang.VersionProps

jdk/vm/ci/services/Services.java
  67             Class.forName("jdk.vm.ci.runtime.JVMCI”);

JVMCI class is local in jdk.internal.vm.ci module.  An alternative may be
to provide a static initialize method rather than Class::forName.

jdk/vm/ci/hotspot/HotSpotJVMCIRuntime.java
 211         for (HotSpotJVMCIBackendFactory factory : ServiceLoader.load(HotSpotJVMCIBackendFactory.class)) {

This uses the thread context class loader to load providers.  Services::load uses
the system class loader to load providers.  I suspect you want this to use the
system class loader here too.

jdk/vm/ci/services/JVMCIServiceLocator.java
  78         for (JVMCIServiceLocator access : ServiceLoader.load(JVMCIServiceLocator.class)) {

Same comment as above. I think you want to use system class loader to load providers.

Mandy
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Doug Simon @ Oracle

> On 19 Apr 2017, at 02:02, Mandy Chung <[hidden email]> wrote:
>
>
>> On Apr 18, 2017, at 3:13 PM, Doug Simon <[hidden email]> wrote:
>>
>> Please review these changes that make jdk.internal.vm.compiler an upgradable compiler.
>> :
>> http://cr.openjdk.java.net/~dnsimon/8177845/
>
> Thanks for making this change.  This would simplify the way to replace JDK’s graal with the lab graal.  A couple of comments:
>
> jdk/internal/misc/VM.java
>
> 161      * Note that the saved system properties do not include
> 162      * the ones set by sun.misc.Version.init().
>
> sun.misc.Version is no longer present in JDK 9.  Renamed to java.lang.VersionProps

Sorry, the webrev was out of date. I've updated it and also fixed the comment for VM::getSavedProperty.

> jdk/vm/ci/services/Services.java
>  67             Class.forName("jdk.vm.ci.runtime.JVMCI”);
>
> JVMCI class is local in jdk.internal.vm.ci module.  An alternative may be
> to provide a static initialize method rather than Class::forName.

JVMCI is broken into fine grained "projects" and the jdk.vm.ci.runtime[1] project depends on the jdk.vm.ci.services project[2] so I cannot make a direct reference without introducing a circular dependency. I don't expect the reflection cost to be significant in practice.

> jdk/vm/ci/hotspot/HotSpotJVMCIRuntime.java
> 211         for (HotSpotJVMCIBackendFactory factory : ServiceLoader.load(HotSpotJVMCIBackendFactory.class)) {
>
> This uses the thread context class loader to load providers.  Services::load uses
> the system class loader to load providers.  I suspect you want this to use the
> system class loader here too.
>
> jdk/vm/ci/services/JVMCIServiceLocator.java
>  78         for (JVMCIServiceLocator access : ServiceLoader.load(JVMCIServiceLocator.class)) {
>
> Same comment as above. I think you want to use system class loader to load providers.

Yes, I think you're right. I've updated the webrev with that change.

Thanks for the review.

-Doug

[1] http://hg.openjdk.java.net/jdk9/dev/hotspot/file/560d7aa083a2/.mx.jvmci/suite.py#l94
[2] http://hg.openjdk.java.net/jdk9/dev/hotspot/file/560d7aa083a2/.mx.jvmci/suite.py#l45

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Peter Levart
In reply to this post by Mandy Chung
Hi Doug, Mandy,

Just a minor comment on the VM class changes...

Note that Properties class is thread-safe, while HashMap isn't. But I
think this should not be a problem here, as during initPhase1(), as far
as I know, no threads apart from main thread are started yet (is this
true?), so anyone accessing getSavedProperty(ies) will either be the
main thread itself or a thread started afterwards, so there is a
happens-before relationship.

If this is true, then we could wrap the copy of saved properties with an
unmodifiable map view before setting the savedProps field so that
getSavedProperties() could always return the same instance. Like for
example in the following alternative change:

http://cr.openjdk.java.net/~plevart/jdk9-dev/8177845_VM.getSavedProperties/webrev.01/

What do you think?

Regards, Peter

On 04/19/2017 02:02 AM, Mandy Chung wrote:

>> On Apr 18, 2017, at 3:13 PM, Doug Simon <[hidden email]> wrote:
>>
>> Please review these changes that make jdk.internal.vm.compiler an upgradable compiler.
>> :
>> http://cr.openjdk.java.net/~dnsimon/8177845/
> Thanks for making this change.  This would simplify the way to replace JDK’s graal with the lab graal.  A couple of comments:
>
> jdk/internal/misc/VM.java
>
>   161      * Note that the saved system properties do not include
>   162      * the ones set by sun.misc.Version.init().
>
> sun.misc.Version is no longer present in JDK 9.  Renamed to java.lang.VersionProps
>
> jdk/vm/ci/services/Services.java
>    67             Class.forName("jdk.vm.ci.runtime.JVMCI”);
>
> JVMCI class is local in jdk.internal.vm.ci module.  An alternative may be
> to provide a static initialize method rather than Class::forName.
>
> jdk/vm/ci/hotspot/HotSpotJVMCIRuntime.java
>   211         for (HotSpotJVMCIBackendFactory factory : ServiceLoader.load(HotSpotJVMCIBackendFactory.class)) {
>
> This uses the thread context class loader to load providers.  Services::load uses
> the system class loader to load providers.  I suspect you want this to use the
> system class loader here too.
>
> jdk/vm/ci/services/JVMCIServiceLocator.java
>    78         for (JVMCIServiceLocator access : ServiceLoader.load(JVMCIServiceLocator.class)) {
>
> Same comment as above. I think you want to use system class loader to load providers.
>
> Mandy

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Peter Levart


On 04/19/2017 09:37 AM, Peter Levart wrote:
>
>
> http://cr.openjdk.java.net/~plevart/jdk9-dev/8177845_VM.getSavedProperties/webrev.01/ 
>

Also, while we are at it, the following javadocs in the
getSavedProperty() do not apply any more:

  138      * It accesses a private copy of the system properties so
  139      * that user's locking of the system properties object will not
  140      * cause the library to deadlock.

In JDK 9, Properties class does not use locking any more on the
Properties instance for get()/getProperty() methods...

Regards, Peter

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Peter Levart


On 04/19/2017 09:42 AM, Peter Levart wrote:

>
>
> On 04/19/2017 09:37 AM, Peter Levart wrote:
>>
>>
>> http://cr.openjdk.java.net/~plevart/jdk9-dev/8177845_VM.getSavedProperties/webrev.01/ 
>>
>
> Also, while we are at it, the following javadocs in the
> getSavedProperty() do not apply any more:
>
>  138      * It accesses a private copy of the system properties so
>  139      * that user's locking of the system properties object will not
>  140      * cause the library to deadlock.
>
> In JDK 9, Properties class does not use locking any more on the
> Properties instance for get()/getProperty() methods...
>
> Regards, Peter
>

I also noticed the following comment:

     // TODO: the Property Management needs to be refactored and
     // the appropriate prop keys need to be accessible to the
     // calling classes to avoid duplication of keys.
     private static Map<String, String> savedProps;

...which is not entirely true. Neither keys nor values are duplicated
(they are just referenced in the new copy of the Properties/Map object).
What is duplicated is an excessive amount of internal objects, such as
array slots and Map.Entry objects. If this is a concern, then we could
use the new immutable Map implementation that is available in JDK 9, so
the following lines in my webrev:

  181         @SuppressWarnings("unchecked")
  182         Map<String, String> sp = new HashMap<>((Map)props);
  183         // only main thread is running at this time, so savedProps and
  184         // its content will be correctly published to threads
started later
  185         savedProps = Collections.unmodifiableMap(sp);

Could be changed into:

         @SuppressWarnings("unchecked")
         Map<String, String> sp =
             Map.ofEntries(props.entrySet().toArray(new Map.Entry[0]));
         // only main thread is running at this time, so savedProps
         // will be correctly published to threads started later
         savedProps = sp;

...to save some excessive space (the implementation is a linear-probe
hashtable which keeps keys and values directly in an array without
wrapping them with  Map.Entry objects).

Regards, Peter

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Doug Simon @ Oracle
Hi Peter,

All of your suggestions look good. I've updated http://cr.openjdk.java.net/~dnsimon/8177845/jdk/src/java.base/share/classes/jdk/internal/misc/VM.java.udiff.html to include them (please check I didn't make any copy errors in the process).

I was not aware of the new Map.ofEntries method. Nice to see more space efficient map implementations being added to the JDK.

Thanks!

-Doug

> On 19 Apr 2017, at 10:12, Peter Levart <[hidden email]> wrote:
>
>
>
> On 04/19/2017 09:42 AM, Peter Levart wrote:
>>
>>
>> On 04/19/2017 09:37 AM, Peter Levart wrote:
>>>
>>>
>>> http://cr.openjdk.java.net/~plevart/jdk9-dev/8177845_VM.getSavedProperties/webrev.01/ 
>>
>> Also, while we are at it, the following javadocs in the getSavedProperty() do not apply any more:
>>
>> 138      * It accesses a private copy of the system properties so
>> 139      * that user's locking of the system properties object will not
>> 140      * cause the library to deadlock.
>>
>> In JDK 9, Properties class does not use locking any more on the Properties instance for get()/getProperty() methods...
>>
>> Regards, Peter
>>
>
> I also noticed the following comment:
>
>    // TODO: the Property Management needs to be refactored and
>    // the appropriate prop keys need to be accessible to the
>    // calling classes to avoid duplication of keys.
>    private static Map<String, String> savedProps;
>
> ...which is not entirely true. Neither keys nor values are duplicated (they are just referenced in the new copy of the Properties/Map object). What is duplicated is an excessive amount of internal objects, such as array slots and Map.Entry objects. If this is a concern, then we could use the new immutable Map implementation that is available in JDK 9, so the following lines in my webrev:
>
> 181         @SuppressWarnings("unchecked")
> 182         Map<String, String> sp = new HashMap<>((Map)props);
> 183         // only main thread is running at this time, so savedProps and
> 184         // its content will be correctly published to threads started later
> 185         savedProps = Collections.unmodifiableMap(sp);
>
> Could be changed into:
>
>        @SuppressWarnings("unchecked")
>        Map<String, String> sp =
>            Map.ofEntries(props.entrySet().toArray(new Map.Entry[0]));
>        // only main thread is running at this time, so savedProps
>        // will be correctly published to threads started later
>        savedProps = sp;
>
> ...to save some excessive space (the implementation is a linear-probe hashtable which keeps keys and values directly in an array without wrapping them with  Map.Entry objects).
>
> Regards, Peter
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Peter Levart
Hi Simon,

On 04/19/2017 11:25 AM, Doug Simon wrote:
> Hi Peter,
>
> All of your suggestions look good. I've updated http://cr.openjdk.java.net/~dnsimon/8177845/jdk/src/java.base/share/classes/jdk/internal/misc/VM.java.udiff.html to include them (please check I didn't make any copy errors in the process).

Looks good.

>
> I was not aware of the new Map.ofEntries method. Nice to see more space efficient map implementations being added to the JDK.

Admittedly, I used this method in a way not envisioned by the author.
Maybe there's a reason there is no Map.copyOf(Map) method there, which
would make this even simpler. If there was one, it would be too easy to
(mis)use it instead of Collections.unmodifiableMap(Map), albeit with a
slightly different semantics, and force re-hashing-copying of big maps
where there is no need to do that. But it would be a pretty nice
replacement for the following idiom:

Collections.unmodifiableMap(new HashMap<>(someMap))

Regards, Peter

>
> Thanks!
>
> -Doug
>
>> On 19 Apr 2017, at 10:12, Peter Levart <[hidden email]> wrote:
>>
>>
>>
>> On 04/19/2017 09:42 AM, Peter Levart wrote:
>>>
>>> On 04/19/2017 09:37 AM, Peter Levart wrote:
>>>>
>>>> http://cr.openjdk.java.net/~plevart/jdk9-dev/8177845_VM.getSavedProperties/webrev.01/
>>> Also, while we are at it, the following javadocs in the getSavedProperty() do not apply any more:
>>>
>>> 138      * It accesses a private copy of the system properties so
>>> 139      * that user's locking of the system properties object will not
>>> 140      * cause the library to deadlock.
>>>
>>> In JDK 9, Properties class does not use locking any more on the Properties instance for get()/getProperty() methods...
>>>
>>> Regards, Peter
>>>
>> I also noticed the following comment:
>>
>>     // TODO: the Property Management needs to be refactored and
>>     // the appropriate prop keys need to be accessible to the
>>     // calling classes to avoid duplication of keys.
>>     private static Map<String, String> savedProps;
>>
>> ...which is not entirely true. Neither keys nor values are duplicated (they are just referenced in the new copy of the Properties/Map object). What is duplicated is an excessive amount of internal objects, such as array slots and Map.Entry objects. If this is a concern, then we could use the new immutable Map implementation that is available in JDK 9, so the following lines in my webrev:
>>
>> 181         @SuppressWarnings("unchecked")
>> 182         Map<String, String> sp = new HashMap<>((Map)props);
>> 183         // only main thread is running at this time, so savedProps and
>> 184         // its content will be correctly published to threads started later
>> 185         savedProps = Collections.unmodifiableMap(sp);
>>
>> Could be changed into:
>>
>>         @SuppressWarnings("unchecked")
>>         Map<String, String> sp =
>>             Map.ofEntries(props.entrySet().toArray(new Map.Entry[0]));
>>         // only main thread is running at this time, so savedProps
>>         // will be correctly published to threads started later
>>         savedProps = sp;
>>
>> ...to save some excessive space (the implementation is a linear-probe hashtable which keeps keys and values directly in an array without wrapping them with  Map.Entry objects).
>>
>> Regards, Peter
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Peter Levart

On 04/19/2017 11:49 AM, Peter Levart wrote:

>> I was not aware of the new Map.ofEntries method. Nice to see more
>> space efficient map implementations being added to the JDK.
>
> Admittedly, I used this method in a way not envisioned by the author.
> Maybe there's a reason there is no Map.copyOf(Map) method there, which
> would make this even simpler. If there was one, it would be too easy
> to (mis)use it instead of Collections.unmodifiableMap(Map), albeit
> with a slightly different semantics, and force re-hashing-copying of
> big maps where there is no need to do that. But it would be a pretty
> nice replacement for the following idiom:
>
> Collections.unmodifiableMap(new HashMap<>(someMap))

Sometimes I wish that besides public, protected, "package-private" and
private, Java also had an "expert protected" access modifier ;-)

Regards, Peter

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Alan Bateman
In reply to this post by Peter Levart
On 19/04/2017 08:37, Peter Levart wrote:

> :
>
> Note that Properties class is thread-safe, while HashMap isn't. But I
> think this should not be a problem here, as during initPhase1(), as
> far as I know, no threads apart from main thread are started yet (is
> this true?), so anyone accessing getSavedProperty(ies) will either be
> the main thread itself or a thread started afterwards, so there is a
> happens-before relationship.
The ReferenceHandler and Finalizer threads are started early, before
initPhase1. However, both should immediately block and in the case of
the Finalizer, await the completion of initPhase1 before polling.

-Alan
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Alan Bateman
In reply to this post by Doug Simon @ Oracle
On 18/04/2017 23:13, Doug Simon wrote:

> :
>
> https://bugs.openjdk.java.net/browse/JDK-8177845
> http://cr.openjdk.java.net/~dnsimon/8177845/
>
I'm happy to see jdk.internal.vm.compiler changing to be an upgradable
module and the patching foo going away.

If I read the changes correctly then I can extend JVMCIServiceLocator
and the construction of my sub-class will open up all packages in
jdk.internal.vm.ci to me. It is there any to tie this to -XX:+EnableJVMCI?

-Alan
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Peter Levart
In reply to this post by Alan Bateman
Hi Alan,

On 04/19/2017 03:41 PM, Alan Bateman wrote:

> On 19/04/2017 08:37, Peter Levart wrote:
>
>> :
>>
>> Note that Properties class is thread-safe, while HashMap isn't. But I
>> think this should not be a problem here, as during initPhase1(), as
>> far as I know, no threads apart from main thread are started yet (is
>> this true?), so anyone accessing getSavedProperty(ies) will either be
>> the main thread itself or a thread started afterwards, so there is a
>> happens-before relationship.
> The ReferenceHandler and Finalizer threads are started early, before
> initPhase1. However, both should immediately block and in the case of
> the Finalizer, await the completion of initPhase1 before polling.
>
> -Alan

Just out of curiosity, what guarantees are there that no code before
VM.saveAndRemoveProperties() ever executes Integer.valueOf(int) method
for example? Note that this call is implicitly hidden in autoboxing of
int(s)...

If this happens, then class initialization of Integer.IntegerCache may
fail, because it is using VM.getSavedProperty():

     public static String getSavedProperty(String key) {
         if (savedProps.isEmpty())
             throw new IllegalStateException("Should be non-empty if
initialized");

         return savedProps.getProperty(key);
     }

Hm....

Peter

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Mandy Chung

> On Apr 19, 2017, at 8:03 AM, Peter Levart <[hidden email]> wrote:
>
> Hi Alan,
>
> On 04/19/2017 03:41 PM, Alan Bateman wrote:
>> On 19/04/2017 08:37, Peter Levart wrote:
>>
>>> :
>>>
>>> Note that Properties class is thread-safe, while HashMap isn't. But I think this should not be a problem here, as during initPhase1(), as far as I know, no threads apart from main thread are started yet (is this true?), so anyone accessing getSavedProperty(ies) will either be the main thread itself or a thread started afterwards, so there is a happens-before relationship.
>> The ReferenceHandler and Finalizer threads are started early, before initPhase1. However, both should immediately block and in the case of the Finalizer, await the completion of initPhase1 before polling.
>>
>> -Alan
>
> Just out of curiosity, what guarantees are there that no code before VM.saveAndRemoveProperties() ever executes Integer.valueOf(int) method for example? Note that this call is implicitly hidden in autoboxing of int(s)...
>
> If this happens, then class initialization of Integer.IntegerCache may fail, because it is using VM.getSavedProperty():
>
>    public static String getSavedProperty(String key) {
>        if (savedProps.isEmpty())
>            throw new IllegalStateException("Should be non-empty if initialized");
>
>        return savedProps.getProperty(key);
>    }
>
> Hm....

We should catch this issue and detect if VM.getSavedProperty is called when init level < 1 during early startup.

Mandy
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Doug Simon @ Oracle

> On 19 Apr 2017, at 20:01, Mandy Chung <[hidden email]> wrote:
>
>>
>> On Apr 19, 2017, at 8:03 AM, Peter Levart <[hidden email]> wrote:
>>
>> Hi Alan,
>>
>> On 04/19/2017 03:41 PM, Alan Bateman wrote:
>>> On 19/04/2017 08:37, Peter Levart wrote:
>>>
>>>> :
>>>>
>>>> Note that Properties class is thread-safe, while HashMap isn't. But I think this should not be a problem here, as during initPhase1(), as far as I know, no threads apart from main thread are started yet (is this true?), so anyone accessing getSavedProperty(ies) will either be the main thread itself or a thread started afterwards, so there is a happens-before relationship.
>>> The ReferenceHandler and Finalizer threads are started early, before initPhase1. However, both should immediately block and in the case of the Finalizer, await the completion of initPhase1 before polling.
>>>
>>> -Alan
>>
>> Just out of curiosity, what guarantees are there that no code before VM.saveAndRemoveProperties() ever executes Integer.valueOf(int) method for example? Note that this call is implicitly hidden in autoboxing of int(s)...
>>
>> If this happens, then class initialization of Integer.IntegerCache may fail, because it is using VM.getSavedProperty():
>>
>>   public static String getSavedProperty(String key) {
>>       if (savedProps.isEmpty())
>>           throw new IllegalStateException("Should be non-empty if initialized");
>>
>>       return savedProps.getProperty(key);
>>   }
>>
>> Hm....
>
> We should catch this issue and detect if VM.getSavedProperty is called when init level < 1 during early startup.

What are you proposing? Some extra testing of VM startup to see if this is an issue? If so, can you please suggest what testing is to be performed.

-Doug
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Mandy Chung

> On Apr 19, 2017, at 12:07 PM, Doug Simon <[hidden email]> wrote:
>
>
>> On 19 Apr 2017, at 20:01, Mandy Chung <[hidden email]> wrote:
>>
>> We should catch this issue and detect if VM.getSavedProperty is called when init level < 1 during early startup.
>
> What are you proposing? Some extra testing of VM startup to see if this is an issue? If so, can you please suggest what testing is to be performed.

This is an existing issue, not related to your change.  I will create JBS issue to track it.

What you have is fine.

Mandy

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Vladimir Kozlov
In reply to this post by Doug Simon @ Oracle
Hi Doug,

Can you consider using other name and not JDK9 for new JVMCI class? It
will be used in JDK 10 too:

jdk.vm.ci.services.internal.JDK9;

Thanks,
Vladimir

On 4/18/17 3:13 PM, Doug Simon wrote:

> Please review these changes that make jdk.internal.vm.compiler an upgradable compiler.
> The primary requirement for this is to remove all usage of JDK internals from Graal.
> While this most involves changes in Graal, there remain a few capabilities that must
> be exposed via JVMCI. Namely:
>
> 1. Graal needs a copy of jdk.internal.misc.VM.savedProps so that it can Graal initialize
>    can use system properties that are guaranteed not to have been modified by application
>    code (Graal initialization is lazy and may occur after application code has been run).
>
> 2. Truffle needs to be able to trigger JVMCI initialization so that it can select the Graal
>    optimized implementation of the Truffle runtime.
>
> These capabilities have been added to jdk.vm.ci.services.Services. A comment has
> also been added to this class to denote the current methods to be removed
> in a subsequent bug to update the JDK copy of Graal with the upstream version which
> no longer uses the methods. The methods destined for removal are:
>
> exportJVMCITo(Class<?> requestor)
> load(Class<S> service)
> loadSingle(Class<S> service, boolean required)
>
> The first one is no longer needed as JVMCI exports itself to Graal service providers
> it loads via private API and Graal re-exports[1] JVMCI to any module the extends Graal.
> The latter 2 are no longer required as Graal uses the standard ServiceLoader. This API
> is only useful for JVMCI-8 where a hidden JVMCI class loader is used.
>
> These changes have been tested against upstream Graal. To make the Graal tests pass, 2 extra
> minor changes were required:
>
> 1. In JDK-8177663, HotSpotMemoryAccessProviderImpl::checkRead was added and throws IllegalArgumentException
>    on all error paths except one which throws AssertionError. The latter was a mistake and has been
>    fixed to also throw IllegalArgumentException.
> 2. An invalid assertion has been removed from HotSpotResolvedObjectTypeImpl::isDefinitelyResolvedWithRespectTo.
>    Up to JDK 8, it was true that all classes in java.* packages were loaded by the boot class loader.
>    This is no longer true in JDK 9 with classes like java.sql being loaded by platform class loader.
>
> As hinted above, a subsequent bug will be required to pull in the latest upstream Graal and
> remove use of JDK internal API from the jdk.aot module. The qualified exports to
> jdk.vm.internal.compiler and jdk.aot can then be removed.
>
> -Doug
>
> https://bugs.openjdk.java.net/browse/JDK-8177845
> http://cr.openjdk.java.net/~dnsimon/8177845/
>
> [1] http://openjdk.java.net/projects/jigsaw/spec/issues/#IndirectQualifiedReflectiveAccess
>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Vladimir Kozlov
ReflectionAccessJDK ? Based on your comment in the file.

Vladimir

On 4/19/17 1:02 PM, Doug Simon wrote:

> Sure - how about good old Util? ;-) I'm open to other suggestions.
>
> Sent from my iPhone
>
>> On Apr 19, 2017, at 9:46 PM, Vladimir Kozlov <[hidden email]> wrote:
>>
>> Hi Doug,
>>
>> Can you consider using other name and not JDK9 for new JVMCI class? It will be used in JDK 10 too:
>>
>> jdk.vm.ci.services.internal.JDK9;
>>
>> Thanks,
>> Vladimir
>>
>>> On 4/18/17 3:13 PM, Doug Simon wrote:
>>> Please review these changes that make jdk.internal.vm.compiler an upgradable compiler.
>>> The primary requirement for this is to remove all usage of JDK internals from Graal.
>>> While this most involves changes in Graal, there remain a few capabilities that must
>>> be exposed via JVMCI. Namely:
>>>
>>> 1. Graal needs a copy of jdk.internal.misc.VM.savedProps so that it can Graal initialize
>>>   can use system properties that are guaranteed not to have been modified by application
>>>   code (Graal initialization is lazy and may occur after application code has been run).
>>>
>>> 2. Truffle needs to be able to trigger JVMCI initialization so that it can select the Graal
>>>   optimized implementation of the Truffle runtime.
>>>
>>> These capabilities have been added to jdk.vm.ci.services.Services. A comment has
>>> also been added to this class to denote the current methods to be removed
>>> in a subsequent bug to update the JDK copy of Graal with the upstream version which
>>> no longer uses the methods. The methods destined for removal are:
>>>
>>> exportJVMCITo(Class<?> requestor)
>>> load(Class<S> service)
>>> loadSingle(Class<S> service, boolean required)
>>>
>>> The first one is no longer needed as JVMCI exports itself to Graal service providers
>>> it loads via private API and Graal re-exports[1] JVMCI to any module the extends Graal.
>>> The latter 2 are no longer required as Graal uses the standard ServiceLoader. This API
>>> is only useful for JVMCI-8 where a hidden JVMCI class loader is used.
>>>
>>> These changes have been tested against upstream Graal. To make the Graal tests pass, 2 extra
>>> minor changes were required:
>>>
>>> 1. In JDK-8177663, HotSpotMemoryAccessProviderImpl::checkRead was added and throws IllegalArgumentException
>>>   on all error paths except one which throws AssertionError. The latter was a mistake and has been
>>>   fixed to also throw IllegalArgumentException.
>>> 2. An invalid assertion has been removed from HotSpotResolvedObjectTypeImpl::isDefinitelyResolvedWithRespectTo.
>>>   Up to JDK 8, it was true that all classes in java.* packages were loaded by the boot class loader.
>>>   This is no longer true in JDK 9 with classes like java.sql being loaded by platform class loader.
>>>
>>> As hinted above, a subsequent bug will be required to pull in the latest upstream Graal and
>>> remove use of JDK internal API from the jdk.aot module. The qualified exports to
>>> jdk.vm.internal.compiler and jdk.aot can then be removed.
>>>
>>> -Doug
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8177845
>>> http://cr.openjdk.java.net/~dnsimon/8177845/
>>>
>>> [1] http://openjdk.java.net/projects/jigsaw/spec/issues/#IndirectQualifiedReflectiveAccess
>>>
>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Doug Simon @ Oracle
In reply to this post by Doug Simon @ Oracle

> On 19 Apr 2017, at 21:40, Christian Thalinger <[hidden email]> wrote:
>
>>
>> On Apr 19, 2017, at 9:27 AM, Doug Simon <[hidden email]> wrote:
>>
>>>
>>> On 19 Apr 2017, at 21:04, Mandy Chung <[hidden email]> wrote:
>>>
>>>
>>>> On Apr 19, 2017, at 11:55 AM, Christian Thalinger <[hidden email]> wrote:
>>>>
>>>>
>>>>> On Apr 19, 2017, at 8:38 AM, Mandy Chung <[hidden email]> wrote:
>>>>>
>>>>> Since jdk.internal.vm.compiler becomes an upgradeable module, it is not hashed with java.base to allow it to be upgraded and there is no integrity check.  Such qualified export will be granted to any module named jdk.internal.vm.compiler at runtime.  The goal is for upgradeable modules not to use any internal APIs and eliminate the qualified exports.
>>>>>
>>>>> The main thing is that jdk.vm.ci.services API would need to be guarded if it’s used by non-Graal modules.
>>>>
>>>> This all makes sense but where is the restriction that only jdk.internal.vm.compiler can use jdk.vm.ci.services?  
>>>
>>> It's unqualified and no restriction in this change.
>>
>> The public methods currently in jdk.vm.ci.services are:
>>
>> 1. JVMCIServiceLocator.getProvider(Class<S>)
>> 2. JVMCIServiceLocator.getProviders(Class<S>)
>> 3. Services.initializeJVMCI()
>> 4. Services.getSavedProperties()
>> 5. Services.exportJVMCITo(Class<?>)
>> 6. Services.load(Class<S>)
>> 7. Services.loadSingle(Class<S>, boolean)
>>
>> 1 should be made protected. I'll update the webrev with this change.
>
> Good.
>
>>
>> 2 should check for JVMCIPermission. I'll update the webrev with this change.
>
> Good.
>
>>
>> 3 is harmless from a security perspective in my opinion.
>
> Would be good if one of Oracle’s security engineers could take a quick look just to be sure.

Vladimir, can you please bring this to the attention of the relevant engineer.

>>
>> 4 checks for JVMCIPermission.
>
> Ok.
>
>>
>> 5, 6 and 7 will be removed in a follow bug that updates Graal from upstream (and removes its usage of these methods).
>
> About this, will this Graal update happen for JDK 9?

Yes.

>  It’s awfully late in the cycle...

These are jigsaw related changes and I've been told jigsaw has an FC exception (although I don't exactly know what that is).

-Doug
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Doug Simon @ Oracle
I've updated http://cr.openjdk.java.net/~dnsimon/8177845/hotspot/ with these changes:

1. JVMCIServiceLocator.getProvider(Class<S>) is now protected
2. JVMCIServiceLocator.getProviders(Class<S>) now checks JVMCIPermission
3. Rename: jdk.vm.ci.services.internal.JDK9 -> jdk.vm.ci.services.internal.ReflectionAccessJDK

-Doug

> On 19 Apr 2017, at 23:12, Doug Simon <[hidden email]> wrote:
>
>>
>> On 19 Apr 2017, at 21:40, Christian Thalinger <[hidden email]> wrote:
>>
>>>
>>> On Apr 19, 2017, at 9:27 AM, Doug Simon <[hidden email]> wrote:
>>>
>>>>
>>>> On 19 Apr 2017, at 21:04, Mandy Chung <[hidden email]> wrote:
>>>>
>>>>
>>>>> On Apr 19, 2017, at 11:55 AM, Christian Thalinger <[hidden email]> wrote:
>>>>>
>>>>>
>>>>>> On Apr 19, 2017, at 8:38 AM, Mandy Chung <[hidden email]> wrote:
>>>>>>
>>>>>> Since jdk.internal.vm.compiler becomes an upgradeable module, it is not hashed with java.base to allow it to be upgraded and there is no integrity check.  Such qualified export will be granted to any module named jdk.internal.vm.compiler at runtime.  The goal is for upgradeable modules not to use any internal APIs and eliminate the qualified exports.
>>>>>>
>>>>>> The main thing is that jdk.vm.ci.services API would need to be guarded if it’s used by non-Graal modules.
>>>>>
>>>>> This all makes sense but where is the restriction that only jdk.internal.vm.compiler can use jdk.vm.ci.services?  
>>>>
>>>> It's unqualified and no restriction in this change.
>>>
>>> The public methods currently in jdk.vm.ci.services are:
>>>
>>> 1. JVMCIServiceLocator.getProvider(Class<S>)
>>> 2. JVMCIServiceLocator.getProviders(Class<S>)
>>> 3. Services.initializeJVMCI()
>>> 4. Services.getSavedProperties()
>>> 5. Services.exportJVMCITo(Class<?>)
>>> 6. Services.load(Class<S>)
>>> 7. Services.loadSingle(Class<S>, boolean)
>>>
>>> 1 should be made protected. I'll update the webrev with this change.
>>
>> Good.
>>
>>>
>>> 2 should check for JVMCIPermission. I'll update the webrev with this change.
>>
>> Good.
>>
>>>
>>> 3 is harmless from a security perspective in my opinion.
>>
>> Would be good if one of Oracle’s security engineers could take a quick look just to be sure.
>
> Vladimir, can you please bring this to the attention of the relevant engineer.
>
>>>
>>> 4 checks for JVMCIPermission.
>>
>> Ok.
>>
>>>
>>> 5, 6 and 7 will be removed in a follow bug that updates Graal from upstream (and removes its usage of these methods).
>>
>> About this, will this Graal update happen for JDK 9?
>
> Yes.
>
>> It’s awfully late in the cycle...
>
> These are jigsaw related changes and I've been told jigsaw has an FC exception (although I don't exactly know what that is).
>
> -Doug

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8177845: Need a mechanism to load Graal

Vladimir Kozlov
Hotspot changes looks good to me.

Thanks,
Vladimir

On 4/19/17 2:26 PM, Doug Simon wrote:

> I've updated http://cr.openjdk.java.net/~dnsimon/8177845/hotspot/ with these changes:
>
> 1. JVMCIServiceLocator.getProvider(Class<S>) is now protected
> 2. JVMCIServiceLocator.getProviders(Class<S>) now checks JVMCIPermission
> 3. Rename: jdk.vm.ci.services.internal.JDK9 -> jdk.vm.ci.services.internal.ReflectionAccessJDK
>
> -Doug
>
>> On 19 Apr 2017, at 23:12, Doug Simon <[hidden email]> wrote:
>>
>>>
>>> On 19 Apr 2017, at 21:40, Christian Thalinger <[hidden email]> wrote:
>>>
>>>>
>>>> On Apr 19, 2017, at 9:27 AM, Doug Simon <[hidden email]> wrote:
>>>>
>>>>>
>>>>> On 19 Apr 2017, at 21:04, Mandy Chung <[hidden email]> wrote:
>>>>>
>>>>>
>>>>>> On Apr 19, 2017, at 11:55 AM, Christian Thalinger <[hidden email]> wrote:
>>>>>>
>>>>>>
>>>>>>> On Apr 19, 2017, at 8:38 AM, Mandy Chung <[hidden email]> wrote:
>>>>>>>
>>>>>>> Since jdk.internal.vm.compiler becomes an upgradeable module, it is not hashed with java.base to allow it to be upgraded and there is no integrity check.  Such qualified export will be granted to any module named jdk.internal.vm.compiler at runtime.  The goal is for upgradeable modules not to use any internal APIs and eliminate the qualified exports.
>>>>>>>
>>>>>>> The main thing is that jdk.vm.ci.services API would need to be guarded if it’s used by non-Graal modules.
>>>>>>
>>>>>> This all makes sense but where is the restriction that only jdk.internal.vm.compiler can use jdk.vm.ci.services?
>>>>>
>>>>> It's unqualified and no restriction in this change.
>>>>
>>>> The public methods currently in jdk.vm.ci.services are:
>>>>
>>>> 1. JVMCIServiceLocator.getProvider(Class<S>)
>>>> 2. JVMCIServiceLocator.getProviders(Class<S>)
>>>> 3. Services.initializeJVMCI()
>>>> 4. Services.getSavedProperties()
>>>> 5. Services.exportJVMCITo(Class<?>)
>>>> 6. Services.load(Class<S>)
>>>> 7. Services.loadSingle(Class<S>, boolean)
>>>>
>>>> 1 should be made protected. I'll update the webrev with this change.
>>>
>>> Good.
>>>
>>>>
>>>> 2 should check for JVMCIPermission. I'll update the webrev with this change.
>>>
>>> Good.
>>>
>>>>
>>>> 3 is harmless from a security perspective in my opinion.
>>>
>>> Would be good if one of Oracle’s security engineers could take a quick look just to be sure.
>>
>> Vladimir, can you please bring this to the attention of the relevant engineer.
>>
>>>>
>>>> 4 checks for JVMCIPermission.
>>>
>>> Ok.
>>>
>>>>
>>>> 5, 6 and 7 will be removed in a follow bug that updates Graal from upstream (and removes its usage of these methods).
>>>
>>> About this, will this Graal update happen for JDK 9?
>>
>> Yes.
>>
>>> It’s awfully late in the cycle...
>>
>> These are jigsaw related changes and I've been told jigsaw has an FC exception (although I don't exactly know what that is).
>>
>> -Doug
>
12
Loading...