(RFR) 8240245: Avoid calling is_shared_class_visible() in SystemDictionary::load_shared_class()

[Ioi Lam]

Hi Yumin,

I think this check in the old code will be unintentionally skipped by 
your change:

   int path_index = ik->shared_classpath_index();
   ClassLoaderData* loader_data = class_loader_data(class_loader);
   if (path_index < 0) {
     // path_index < 0 indicates that the class is intended for a custom 
loader
     // and should not be loaded by boot/platform/app loaders
     if (loader_data->is_builtin_class_loader_data()) {
       return false;
     } else {
       return true;
     }
   }

I think your optimization should be done after this check.

------------------

1239 // Check if
1240 //   1) -Xbootclasspath/a: specified or
1241 //   2) --module-path at runtime
1242 // so avoid checking class visibility for builtin loaders.
1243 bool continue_check_shared_class_visibility() {

How about changing the function names and comments to:

1239 // Need to do the expensive visibility check for builtin loader only if
1240 //   1) -Xbootclasspath/a: specified or
1241 //   2) --module-path at runtime
1243 bool need_to_check_builtin_shared_class_visibility() {
        ....
      }

      bool SystemDictionary::check_builtin_shared_class_visibility(...) {
        assert(ik->shared_classpath_index() >= 0, "must be built-in class");
        assert(loader_data->is_builtin_class_loader_data(), "must be 
built-in loader");
        ...
------------------

Also, the "ResourceMark rm(THREAD);" can be moved to 
check_builtin_shared_class_visibility.

Thanks
- Ioi



On 5/29/20 9:50 PM, Yumin Qi wrote:
> Hi,
>
>   Please check the updated webrev at: 
> http://cr.openjdk.java.net/~minqi/2020/8240245/webrev-01/
>   In this version, test case 
> test/hotspot/jtreg/runtime/cds/appcds/jigsaw/modulepath/ModulePathAndCP.java
>   modfied/added cases to reflect --module-path used at runtime.
>
>   Thanks
>   Yumin*
> *
> On 5/22/20 8:45 AM, Yumin Qi wrote:
>> Hi, Please review
>>
>>   bug: 8240245: https://bugs.openjdk.java.net/browse/JDK-8240245
>>   Webrev: http://cr.openjdk.java.net/~minqi/2020/8240245/webrev-00/
>>
>> Summary: When -Xbootclasspath/a: and --module-path are not specified, 
>> for bultin loaders, is_shared_class_visible will always return true 
>> so we can skip such check. Another optimization is guarding the call 
>> to load_shared_class with UseSharedSpaces, save unnecessary calls for 
>> non-shared run.
>>
>> For java -version, the performance data:
>>
>> Results of " perf stat -r 40 bin/java -Xshare:on 
>> -XX:SharedArchiveFile=jdk2.jsa -Xint -version "
>>    1:     59008853    59008564 (  -289)                41.100 40.342 
>> ( -0.758)      --
>>    2:     58983285    59026645 ( 43360)  ++++          39.841 40.708 
>> (  0.867)    ++
>>    3:     59008801    59005425 ( -3376)                39.903 40.881 
>> (  0.978)   +++
>>    4:     59032045    58990500 (-41545)      ----      39.809 40.443 
>> (  0.634)    ++
>>    5:     59029813    58976124 (-53689)      -----     40.121 39.238 
>> ( -0.883)      --
>>    6:     59036617    58998279 (-38338)      ----      40.644 39.875 
>> ( -0.769)      --
>>    7:     59003768    59005109 (  1341)                39.416 38.991 
>> ( -0.425)      -
>>    8:     58972545    58985824 ( 13279)     +          40.811 40.001 
>> ( -0.810)      --
>>    9:     59007110    58981883 (-25227)      --        40.969 39.090 
>> ( -1.879)      -----
>>   10:     58992934    58987333 ( -5601)      -         40.521 40.371 
>> ( -0.150)
>> ============================================================
>>           59007573    58996566 (-11006)      -         40.310 39.989 
>> ( -0.321)      -
>> instr delta =       -11006    -0.0187%
>> time  delta =       -0.321 ms -0.7966%
>>
>> Tests:  tier1,tier2
>>
>> Thanks
>> Yumin
>