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

[Calvin Cheung]

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