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

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR(S): 8243586: Optimize calls to SystemDictionaryShared::define_shared_package for classpath
From:       Yumin Qi <yumin.qi () oracle ! com>
Date:       2020-06-29 19:19:03
Message-ID: 49964f3c-6954-0b5c-d29e-b21faf751822 () oracle ! com
[Download RAW message or body]


On 6/29/20 11:36 AM, Calvin Cheung wrote:
> Hi Yumin,
>
> On 6/29/20 11:21 AM, Yumin Qi wrote:
>>
>> Hi, Calvin
>>
>>   Looks good to me.  A minor comment:
>>
> Thanks for your review.
>>
>>   test/hotspot/jtreg/runtime/cds/appcds/test-classes/C3.java":
>>
>>     1 /*
>>     2  * Copyright (c) 2014, 2020, Oracle and/or its affiliates. All 
>> rights reserved.
>>     3  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>     4  *
>>
>>   This is a new file, the year should be year 2020. Is this a copy 
>> problem?
>>
>>
> I've corrected the copyright year in my local repo. I assume that you 
> don't need to see another webrev?
>
No, thanks.


Yumin

> thanks,
>
> Calvin
>
>> Thanks
>>
>> Yumin
>>
>> On 6/29/20 10:59 AM, Calvin Cheung wrote:
>>> Hi Ioi,
>>>
>>> Thanks for your review.
>>>
>>> On 6/28/20 6:01 PM, Ioi Lam wrote:
>>>> Hi Calvin,
>>>>
>>>> [1]
>>>> I think the following name is too generic and also shouldn't be in 
>>>> the global scope. How about moving it into PackageEntry as:
>>>>
>>>> class PackageEntry ... {
>>>>   static int max_index_for_defined_in_class_path() {
>>>>     return sizeof(intptr_t) * BitsPerByte;
>>>>   }
>>>> }
>>>>
>>>> Also, I think it's better to use "int" as the type of "idx" for 
>>>> is_defined_by_cds_in_class_path and set_defined_in_class_path.
>>> Added the above static function. I've also renamed the variable to 
>>> _defined_by_cds_in_class_path and changed its type to int.
>>>>
>>>> Both of the above two functions should assert(index < 
>>>> max_index_for_defined_in_class_path()).
>>> Added the assert to both functions.
>>>>
>>>>  875       if (index_offset < MAX_BITMAP_BITS) {
>>>>  876         if (pkg_entry == NULL || 
>>>> !pkg_entry->is_defined_by_cds_in_class_path(index_offset)) {
>>>>   >>> add comments about the optimization?
>>> Added a comment.
>>>>
>>>> [2] In the test case:
>>>>
>>>>   66         // Test loading of two classes from the same package 
>>>> from different jars.
>>>>   67         // First loaded class is from a non-sealed package, 
>>>> the second loaded
>>>>   68         // class is from the same package but sealed.
>>>>   69         // The expected result is a SecurityException with a 
>>>> "sealing violation".
>>>>
>>>> How about
>>>>
>>>>   69         // The expected result is a SecurityException with a 
>>>> "sealing violation"
>>>>              // for the second class.
>>>>
>>>> and then, check in the output that the first class was successfully 
>>>> loaded.
>>>>
>>>> I think we should also have another test case for:
>>>>
>>>>          output = TestCommon.exec(jars, "PackageSealingTest",
>>>>                                   "sealed/pkg/C1", "sealed", 
>>>> "sealed/pkg/C3", "notSealed");
>>>>          TestCommon.checkExec(output, ".....");
>>>>
>>>> in this case, C3 should fail to load.
>>>
>>> For the above added test, I've used the jar with the sealed package 
>>> during dump time so that we can verify the sealed/pkg/C1 class is 
>>> loaded from the archive during runtime.
>>>
>>> updated webrev:
>>> http://cr.openjdk.java.net/~ccheung/jdk16/8243586/webrev.01/
>>>
>>> thanks,
>>>
>>> Calvin
>>>
>>>
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>>>
>>>>
>>>> On 6/24/20 3:53 PM, Calvin Cheung wrote:
>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8243586
>>>>>
>>>>> webrev: http://cr.openjdk.java.net/~ccheung/jdk16/8243586/webrev.00/
>>>>>
>>>>> The proposed change is to reduce the calls to 
>>>>> SystemDictionaryShared::define_shared_package which does a java 
>>>>> call to AppClassLoader.defineOrCheckPackage. Currently, 
>>>>> define_shared_package is called for every shared class but it is 
>>>>> needed only for each package in each jar specified in the classpath.
>>>>>
>>>>> zprint perf results summary:
>>>>>
>>>>> instr delta = -104380126 -2.2487%
>>>>> time delta = -23.779 ms -3.3640%
>>>>>
>>>>> Testing: running mach5 tier1 and 2 tests.
>>>>>
>>>>> thanks,
>>>>>
>>>>> Calvin
>>>>>
>>>>
[prev in list] [next in list] [prev in thread] [next in thread]