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

[serguei.spitsyn at oracle.com]

Hi Harold,

Thank you again for the comments!
This is the updated webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2016/hotspot/8159145-jigsaw-jvmti-pkg.5/

Thanks,
Serguei


On 6/28/16 07:32, harold seigel wrote:
>
> Hi Serguei,
>
> Looks good.
>
> 1. In modules.cpp, the first assert in get_named_module() has the 
> wrong function name:
>
> 825 assert(ModuleEntryTable::javabase_defined(),
> 826 "Attempt to call *get_module_by_package_name* before java.base is 
> defined");
>
> 2. Also, is a check needed to ensure that package_str is not null?
>
> 3. Is the ResourceMark(THREAD) needed at line 824?
>
> Thanks, Harold
>
> On 6/28/2016 4:54 AM, serguei.spitsyn at oracle.com wrote:
>> 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
>>>>
>>
>