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

[Harold Seigel]

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