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

Calvin Cheung calvin.cheung at oracle.com
Wed Jul 1 18:02:10 UTC 2020


Hi David,

Thanks for taking another look.

thanks,

Calvin

On 6/30/20 9:53 PM, David Holmes wrote:
> 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
>>>>


More information about the hotspot-runtime-dev mailing list