On 1/22/2016 5:16 PM, serguei.spit...@oracle.com wrote:
Please, review this initial fix for the M4 Jigsaw task:
https://bugs.openjdk.java.net/browse/JDK-8049365
Jdk webrev:
cr.openjdk.java.net/~sspitsyn/webrevs/2016/jdk/8049365-Jigsaw-jdk.3/
Hotspot webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8049365-Jigsaw-hs.3/
Hi Serguei,
I have reviewed the hotspot changes and it looks good. Some minor comments:
src/share/vm/prims/jvmtiEnvBase.hpp
- line #698: I think the do_module() method should have a
"assert_locked_or_safepoint(Module_lock);" statement. The method
shouldn't be invoked unless under the Module_lock.
- line #705: Is there a need for JvmtiModuleClosure to have an empty
constructor definition? Could it be removed?
src/share/vm/prims/jvmtiEnvBase.cpp
- line #1486: With a basic "java -version" command there are 77 modules
defined to the VM, so I think the GrowableArray should be initially
allocated to at least 77 elements.
In addition, JvmtiModuleClosure::get_all_modules() is using the
ClassLoaderDataGraph's modules_do() method that Markus Gronlund had
added for JFR support. Good to see you removed the ModulesTable data
structure that you had added to modules.[c/h]pp in your preliminary
implementation. So at this point, I actually don't think you need
anything further from me, JvmtiModuleClosure implementation looks good
and my initial concern was around the ModulesTable data structure which
is now gone.
Thanks,
Lois
Summary:
This round resolves most of the Alan's comments from previous review
rounds.
Two follow-up tasks were filed:
https://bugs.openjdk.java.net/browse/JDK-8148106
https://bugs.openjdk.java.net/browse/JDK-8148103
Also, please, refer to a related M4 Jigsaw task:
https://bugs.openjdk.java.net/browse/JDK-8049364
Update JVM TI for modules
Thanks,
Serguei
