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:
http://cr.openjdk.java.net/~kvn/8182701/webrev.01/ On pátek 18. srpna 2017 21:11:28 CEST Doug Simon wrote: > 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. Mandy already suggested to put the JVMCIMBeans class into its own module with dependencies on jdk.management and jdk.vm.internal.ci modules. I think it is the cleanest solution from the point of modular architecture. The suggested module can be found in the webrev.01 under the name jdk.vm.ci.management. > 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())? These two methods have been replaced by /** * Provides compiler specific platform MBeans. These MBeans will be automatically * exposed once the management system gets initialized. * * @return map from MBean names to their instances */ public Map<String, Object> mbeans() { return Collections.emptyMap(); } in the webrev.01. You are right, the name of the method could be getMBeans(). Let me know if I should rename that and also ... > > 95 /** Name of the {@link MBeanInfo MBean} representing the internals > > This should be: > > /** > * Gets the name of the ... ...please give me a better version of Javadoc, in case the current "Provides..." isn't sufficient. > Same comment for mbean() method. Also {@code null} is used in JVMCI instead > of <code>null</code>. The webrev.01 version never returns null. Instead Collections.emptyMap() is returned by default. Thus this comment no longer applies. Thanks for your comments. -jt > > On 18 Aug 2017, at 20:49, Vladimir Kozlov <vladimir.koz...@oracle.com> > > 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/ca9071941a1be7f1a3725529ecc231ff > >>> 621d5ed0 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
