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

[David Holmes]

Hi Calvin,

Updated look good - thanks.

One comment below ...

On 30/06/2020 4:17 am, Calvin Cheung wrote:
> Hi David,
> 
> On 6/28/20 8:04 PM, David Holmes wrote:
>> Hi Calvin,
>>
>> Generally looks okay but a few comments/suggestions.
> Thanks for your review.
>>
>> On 25/06/2020 8:53 am, 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.
>>
>> src/hotspot/share/classfile/packageEntry.hpp
>>
>> +   volatile intptr_t _defined_in_class_path; // a Package java object 
>> has been define via CDS
>>
>> The name and description of this field is somewhat lacking. It is a 
>> bit map indicating which CDS classpath entries have defined classes in 
>> this package.
> I've rename the field to _defined_by_cds_in_class_path and expanded the 
> comment.
>>
>> +   bool is_defined_by_cds_in_class_path(intptr_t idx) const {
>> +   void set_defined_in_class_path(intptr_t idx) {
>>
>> These names should be consistent i.e.
>>
>> +   void set_defined_by_cds_in_class_path(intptr_t idx) {
> Rename the 'set' function as you suggested.
>>
>> I agree with Ioi that the idx should be range checked by an assert.
> Added the assert.
>>
>> test/hotspot/jtreg/runtime/cds/appcds/PackageSealing.java
>>
>> I don't really understand what this test was doing previously, but it 
>> is not obvious that it is exercising the new bitmap logic in any 
>> rigorous way. Is this test change really related to the code change?
> 
> The original test was to dump and load the "sealed/pkg/C1" and "pkg/C2" 
> classes from the same jar but from different packages. Only the 
> "sealed/pkg/C1" is from a sealed packaged.
> 
> The added tests is to dump "sealed/pkg/C3", which is from a non-sealed 
> package, from non_sealed.jar.

Okay that is what confused me. The name of the package is "sealed" but 
it's not actually sealed as the jar containing C3 is not marked as sealed.

Thanks,
David
-----

> During runtime, it will load 
> "sealed/pkg/C3" and "sealed/pkg/C1" from the same package but from 
> non_sealed.jar and pkg_seal.jar, respectively. So this makes sure define 
> package is called for each package encountered in each jar.
> 
> As suggested by Ioi, I also added another test to dump "sealed/pkg/C1" 
> from a sealed package from pkg_seal.jar. During runtime, the order of 
> loading the classes and the jars in the classpath are reversed.
> 
>>
>>    * @compile test-classes/C1.java
>>    * @compile test-classes/C2.java
>> +  * @compile test-classes/C3.java
>>    * @compile test-classes/PackageSealingTest.java
>>    * @compile test-classes/Hello.java
>>
>> Unless there is a reason separate compilation is required, the number 
>> of @compile commands can be reduced - perhaps to one.
> I've combined them in to one @compile command but separated into 2 lines 
> to avoid long line.
>>
>> +         classList[1] = "sealed/pkg/C3"; // C3 is from a non-sealed 
>> package
>>
>> The comment seems in contradiction to the path - is it sealed or not?
> 
> C3 is from a non-sealed package. That's why I added a comment.
> 
> Note non_sealed.jar was created as follows:
> 
>   70         String nonSealedJar = 
> ClassFileInstaller.writeJar("non_sealed.jar", "sealed/pkg/C3");
> 
> The pkg_seal.jar was created as follows: (note the package_seal.mf 
> manifest)
> 
>    41         String appJar = ClassFileInstaller.writeJar("pkg_seal.jar",
>    42 
> ClassFileInstaller.Manifest.fromSourceFile("test-classes/package_seal.mf"),
>    43             "PackageSealingTest", "sealed/pkg/C1", "pkg/C2");
> 
>>
>> +         TestCommon.testDump(jars, TestCommon.list(classList2), 
>> "-XX:+TraceExceptions");
>>
>> TraceExceptions is deprecated - use -Xlog:exceptions=info
> 
> It turns out it was leftover for debugging. I've removed it.
> 
> updated webrev:
>      http://cr.openjdk.java.net/~ccheung/jdk16/8243586/webrev.01/
> 
> thanks,
> 
> Calvin
> 
>>
>> Thanks,
>> David
>> -----
>>
>>> 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
>>>