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

[Calvin Cheung]

Hi Ioi,

I've renamed the package name as you suggested.

Updated webrev:
     http://cr.openjdk.java.net/~ccheung/jdk16/8243586/webrev.02/

Only test related files have been changed.

I'll push after the current round of testing look good.

thanks,

Calvin

On 6/30/20 10:53 PM, Ioi Lam wrote:
>
>
> 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.
>
>
> For clarity, maybe we can renamed the "sealed/pkg" package to 
> something neutral like "foo", and then have
>
>    foo-sealed.jar    -- contains foo/C1, and specifies that "foo" is a 
> sealed package
>    foo-unsealed.jar   -- contains foo/C3; does not specify sealed 
> packages.
>
> Thanks
> - Ioi
>
>>
>> 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
>>>>>
>