Hi Lois,
Some comments below.
On 1/25/16 11:42, Lois Foltan wrote:
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.
Thank you for the review!
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.
Good suggestions, all fixed.
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.
Great.
I'm resolving the review comments from Alan now and hope to send new
webrevs today after the testing.
Thanks!
Serguei
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
