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

[harold seigel]

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
>>>
>