(RFR) 8240245: Avoid calling is_shared_class_visible() in SystemDictionary::load_shared_class()
Ioi Lam
ioi.lam at oracle.com
Wed Jun 3 02:03:03 UTC 2020
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