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

[Calvin Cheung]

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