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

[serguei.spitsyn at oracle.com]

Hi David,

Thank you for the comments!
I agree with you.

Updated Hotspot webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.4/


Thanks,
Serguei



On 6/27/16 22:06, David Holmes wrote:
> 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
>>