(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