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

Yumin Qi yumin.qi at oracle.com
Mon Jun 1 22:51:12 UTC 2020


Hi, Calvin


On 6/1/20 11:21 AM, Calvin Cheung wrote:
> 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?
>
Yes, will merge.
> -----
>
> In the test:
>
> 201                    .shouldNotContain("--module-path = " + mjars);
>
> Why the "--module-path" is not in the output if CDS is disabled?
>
The function only called with CDS so for non CDS, it wont be called so 
there is no output for it.


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



More information about the hotspot-runtime-dev mailing list