RFR(S): 8243586: Optimize calls to SystemDictionaryShared::define_shared_package for classpath

[Calvin Cheung]

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?

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