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

Yumin Qi yumin.qi at oracle.com
Fri Jun 5 23:02:46 UTC 2020


Hi, Ioi

   Please check updated webrev 
http://cr.openjdk.java.net/~minqi/2020/8240245/webrev-03/

   The code change based on using optimizing module handing saved in 
MetaspaceShared so removed function added to SystemDictionary in last 
version.Test case  added for checking module/jar path with the new change.


Thanks

Yumin


On 6/2/20 7:03 PM, Ioi Lam wrote:
>
>
> On 6/2/20 11:48 AM, Yumin Qi wrote:
>> Hi, Ioi
>>
>>   For adding test case:
>>
>>   I have tested following scenario:
>>
>>  i) module com.hello:
>>
>>   module-info.java:
>>   <code>
>>   module com.hello {
>>       requires com.foo;
>>   }
>>   </code>
>>
>>   Main.java:
>>
>>   <code>
>>   package com.hello;
>>   import com.foo.Test;
>>   public class Main {
>>       public static void main(String... args) {
>>           // System.out.println("Hello, " + Test.getString());
>>           Class<?> k = null;
>>           try {
>>                k = Class.forName("com.foo.Test");
>>                System.out.println("Test found!");
>>           } catch (ClassNotFoundException e) {
>>                System.out.println("ClassNotFoundException " + e);
>>           }
>>       }
>>   }
>>   </code>
>>
>>   ii) module com.foo:
>>
>>   module-info.java:
>>   <code>
>>   module com.foo {
>>     exports com.foo;
>>   }
>>   </code>
>>
>>   Test.java:
>>   <code>
>>   package com.foo;
>>   public class Test {
>>       public static String getString() { return "Test"; }
>>   }
>>   </code>
>>
>>   The two jars are com.hello.jar and com.foo.jar which contain the 
>> two modules respectively.
>>
>>   create .jsa:
>>   java -Xshare:dump -XX:ShareArchiveFile=test.jsa 
>> -XX:SharedClassListFile=classes.list -p -mlib -m 
>> com.hello/com.hello.Main
>>
>>   test:
>>   1)
>>   java -Xshare:on -XX:SharedArchiveFile=test.jsa -Xlog:cds,class+load 
>> -p mlib/com.hello.jar:mlib/com.foo.jar -m com.hello/com.hello.Main
>>
>>   [0.085s][info][class,load] com.hello.Main source: shared objects file
>>   [0.086s][info][class,load] com.foo.Test source: shared objects file
>>   Test found!
>>
>>    2)
>>    java -Xshare:on -XX:SharedArchiveFile=test.jsa 
>> -Xlog:cds,class+load -p mlib/com.hello.jar -cp mlib/com.foo.jar -m 
>> com.hello/com.hello.Main
>>
>>    Error occurred during initialization of boot layer
>>    java.lang.module.FindException: Module com.foo not found, required 
>> by com.hello
>>
>>    3)
>>    java -Xshare:on -XX:SharedArchiveFile=test.jsa 
>> -Xlog:cds,class+load -cp mlib/com.hello.jar:mlib/com.foo.jar 
>> com.hello.Main
>>
>>    [0.052s][info][class,load] java.lang.Void source: shared objects file
>>    [0.053s][info][class,load] com.foo.Test source: shared objects file
>>    Test found!
>>
>>     From above result, we either give --module-path or -classpath for 
>> test. case 2) won't work since com.foo is not in --module-path, but 
>> it is required by com.hello.
>>     Do you mean test 3) should get CNFE?
>>
> I have uploaded a test case at
>
> http://cr.openjdk.java.net/~iklam/jdk15/modules/
> http://cr.openjdk.java.net/~iklam/jdk15/modules/all.tar
>
> You can read the Makefile for details. Examples:
>
> $ make runapp
> /myjdk/bin/java --module-path mlib --add-modules=com.foo 
> --add-exports=com.foo/com.foo=ALL-UNNAMED -cp hello.jar com.hello.Main
> Test found!
>
> $ make runapp0
> /myjdk/bin/java -cp hello.jar com.hello.Main
> ClassNotFoundException java.lang.ClassNotFoundException: com.foo.Test
>
> The com.foo.Test class is stored in the CDS image. So when it's 
> exported using --add-exports, it becomes visible to classes in the 
> unnamed package (such as com.hello.Main). However, if --add-exports is 
> not specified, it cannot be access by the unnamed package.
>
> Thanks
> - Ioi
>
>
>> Thanks
>> Yumin
>>
>> On 6/2/20 1:07 AM, Ioi Lam wrote:
>>> 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
>>>>>>
>>>>>
>>>>
>>>
>>
>


More information about the hotspot-runtime-dev mailing list