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

[Yumin Qi]

HI, Ioi

Please check the updated webrev: 
http://cr.openjdk.java.net/~minqi/2020/8240245/webrev-02/
I changed the test case to use  OutputAnalyzer to check test result. 
Using CDSUtil.Result will check "sharing" first, in the last test, CDS 
is turned off so it will cause checking "sharing" failed.

Resubmitted mach5 test.

Thanks
Yumin

On 6/1/20 10:37 AM, Ioi Lam wrote:
> 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.
>
OK, moved the check after this.
> ------------------
>
> 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() {
>        ....
>      }
>
Done.
> 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.
>
The ResourceMark has no real usage here, removed.

Thanks
Yumin
> 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
>>
>