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

[Yumin Qi]

Hi, Calvin

   Looks good to me.  A minor comment:

   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?


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