RFR(S): 8240204: Optimize package handling for archived classes

[Yumin Qi]

Hi, Harold

   Thanks for the review.

   Yumin

On 4/20/20 5:53 AM, Harold Seigel wrote:
> Hi Yumin,
>
> It looks good!
>
> Thanks, Harold
>
> On 4/18/2020 12:04 AM, Yumin Qi wrote:
>> HI, Harold and all
>>
>> Updated webrev: http://cr.openjdk.java.net/~minqi/8240204/webrev-02/
>> Please check embedded description below.
>>
>> On 4/17/20 10:54 AM, Yumin Qi wrote:
>>> Hi, Harold
>>>
>>>   Thanks for the review.
>>>
>>> On 4/17/20 8:30 AM, Harold Seigel wrote:
>>>> Hi Yumin,
>>>>
>>>> The changes look good.  Just one question.
>>>>
>>>> InstanceKlass::set_classpath_index() returns a value. That value is 
>>>> only checked in one of the places from where it's called.  Is it 
>>>> worth checking the return value in the other places and asserting 
>>>> if it is false?
>>>>
>>> Let me investigate this issue first. If so, we should put the assert 
>>> inside set_classpath_index and change return type to 'void'.
>>>
>> After checked the code, came up with a new version. The package entry 
>> is generated by its name and there would be no dup in 
>> PackageEntryTable for same name. The new version for 
>> set_classpath_index(s2 classpath_index, TRAPS):
>>
>> +void InstanceKlass::set_classpath_index(s2 path_index, TRAPS) {
>> +  if (_package_entry != NULL) {
>> +    DEBUG_ONLY(PackageEntryTable* pkg_entry_tbl = 
>> ClassLoaderData::the_null_class_loader_data()->packages();)
>> + assert(pkg_entry_tbl->lookup_only(_package_entry->name()) == 
>> _package_entry, "Should be same");
>> +    assert(path_index != -1, "Unexpected classpath_index");
>> +    _package_entry->set_classpath_index(path_index);
>> +  }
>> +}
>>
>> If class has no package, its _package_entry is NULL, and we don't 
>> need care about setting classpath_index for it. We save time in 
>> product for not looking up entry from PackageEntryTable with locking 
>> on Moudle_lock.
>> The assert split into two lines, since it will be too long fit into 
>> one line.
>>
>> Also update copyright for files: classLoader.hpp, klassFactory.cpp.
>>
>> Re-tested mach5 hs-tier1-4. Passed.
>> Local test using slowdebug on runtime. No assertion caught.
>> Performance on javac and zprint showed a little more improvement.
>>
>> Thanks
>> Yumin
>>
>>> Thanks
>>> Yumin
>>>> Thanks, Harold
>>>>
>>>> On 4/17/2020 1:56 AM, Yumin Qi wrote:
>>>>> Hi,
>>>>>
>>>>>   Please review:
>>>>>   bug: 8240204: https://bugs.openjdk.java.net/browse/JDK-8240204
>>>>>   webrev: http://cr.openjdk.java.net/~minqi/8240204/webrev-01/
>>>>>
>>>>>   Summary: Move ClassLoader::add_package to InstanceKlass since 
>>>>> what it does is calling pkg->set_classpath_index. Also a minor 
>>>>> optimization for InstanceKlass::set_package, for shared class, 
>>>>> avoid call check_prohibited_package since CDS does not archive 
>>>>> prohibited classes.
>>>>>
>>>>>   Tests: hs-tier1-4
>>>>>   Performance data: javac and zprint showed a little improvement.
>>>>>
>>>>>   Thanks
>>>>>   Yumin
>>>
>>