Jigsaw Enhancement RFR round #3: 8159145 Add JVMTI function GetNamedModule

[David Holmes]

Hi Serguei,

Overall this looks a lot clearer/simpler.

On 28/06/2016 2:20 PM, serguei.spitsyn at oracle.com wrote:
> On 6/27/16 21:08, serguei.spitsyn at oracle.com wrote:
>> Please, review the Jigsaw fix for the enhancement:
>>   https://bugs.openjdk.java.net/browse/JDK-8159145
>>
>>
>> The Hotspot webrev:
>>
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.3/

Are there intended to be other callers of Module::get_named_module? If 
not it seems odd to go to all the trouble of throwing exceptions, just 
to have the caller clear them and return an error code. Or you could 
move all that argument checking code into the JVMTI function directly - 
if called internally you would just assert that arguments are as expected.


src/share/vm/prims/jvmti.xml

+         otherwise the <code>NULL</code> is returned.

Delete "the".

Otherwise all looks good to me.

>>
>>
>> The Jdk webrev is the same:
>>
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.jdk1/
>
> Sorry, the Jdk webrev changed as well:
>
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.jdk3/

Looks fine.

Thanks,
David

>
> Thanks,
> Serguei
>
>
>>
>>
>> Summary:
>>
>>   This is the review round #3.
>>   Alan suggested to replace the function GetModuleByPackageName with
>> the GetNamedModule.
>>   New function will return NULL if the package is not in a module
>> defined to the class loader.
>>   It simplifies the API and makes it easier to specify.
>>   JVM TI agents that instrument code in named modules need the Module
>> at class load time.
>>
>>   One question that came from the function semantics change.
>>   I had to implement the Modules::get_named_module() from scratch
>> independently of the existing
>>   Modules::get_module_by_package_name() and Modules::get_module().
>>   The issue is that the Modules::get_module() can return the unnamed
>> module whereas the
>>   JVMTI helper Modules::get_named_module() should return NULL instead
>> of the unnamed module.
>>   Please, let me know if it is Ok or if you have better ideas how to
>> share the code.
>>
>>   This is the Summary from review round #1:
>>
>>   One way to do this is by introducing a new ClassFileLoadHook that
>> takes an additional parameter but this approach is disruptive.
>>   The alternative option is a JVM TI function that maps a classloader
>> + package name to a module.
>>   We were initially not confident with this approach so we introduced
>> it as JVM function JVM_GetModuleByPackageName.
>>   Based on experience to date then this approach seems okay and so
>> this function needs to be promoted to a JVMTI function.
>>
>>   It includes new jtreg test with native JVMTI agent.
>>
>>
>> Testing:
>>    Run newly developed jtreg test.
>>
>> Thanks,
>> Serguei
>