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

[Ioi Lam]

Hi Yumin,

I think this latest version is still not correct. Sorry I missed this in 
my previous review:

Let's say Foo.jar contains foo.Test:

+ create the archive with --module-path Foo.jar, and archive foo.Test
+ run without --module-path

Then at run time, foo.Test should not be visible. E.g., 
Class.forName("foo.Test") should throw a ClassNotFoundException.

Can you add a new test case for this?

In need_to_check_shared_class_visibility(), I think you should also 
check that

A: dump time bootclasspath was not append
B: dump time module path was not specified

The checks are kind of messy:

     FileMapInfo* mapinfo = FileMapInfo::current_info();
     if (FileMapInfo::dynamic_info() != NULL) {
       mapinfo = FileMapInfo::dynamic_info();
     }

B:  mapinfo->header()->_num_module_paths == 0
A:  we should add a new field _num_boot_classpath_appeneds

Thanks
- Ioi



On 6/1/20 4:23 PM, Yumin Qi wrote:
> 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
>>>
>>
>