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

[Calvin Cheung]

Hi Yumin,

In additional to Ioi's comments:

1272 #ifdef ASSERT
1273   if (!cont) {
1274 assert(SystemDictionary::is_shared_class_visible_impl(class_name, 
ik, pkg_entry, class_loader, THREAD), "Sanity check");
1275   }
1276 #endif
1277   if (!cont) {
1278     return true;
1279   }

Can you combine the above into a single "if (!cont)" block?

-----

In the test:

201                    .shouldNotContain("--module-path = " + mjars);

Why the "--module-path" is not in the output if CDS is disabled?

thanks,

Calvin

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