[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR(S): 8240204: Optimize package handling for archived classes
From:       Yumin Qi <yumin.qi () oracle ! com>
Date:       2020-04-20 21:25:54
Message-ID: 1a0c1254-5e8d-274d-8d2b-0f31dca95b95 () oracle ! com
[Download RAW message or body]

Thanks for review it.

Yumin

On 4/20/20 1:17 PM, Ioi Lam wrote:
> Looks good to me, too. Thanks!
>
> - Ioi
>
> 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
>>>>
>>>
>

[prev in list] [next in list] [prev in thread] [next in thread]