Round #3: RFR: 8049365 - Update JDI and JDWP for modules
Lois Foltan
lois.foltan at oracle.com
Mon Jan 25 19:42:29 UTC 2016
On 1/22/2016 5:16 PM, serguei.spitsyn at 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
More information about the jigsaw-dev
mailing list