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 <doug.si...@oracle.com> 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
