Round #3: RFR: 8049365 - Update JDI and JDWP for modules

[Lois Foltan]

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