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

[serguei.spitsyn at oracle.com]

Hi Lois,

Some comments below.


On 1/25/16 11:42, Lois Foltan wrote:
>
> 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.

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
>