(RFR) 8240245: Avoid calling is_shared_class_visible() in SystemDictionary::load_shared_class()
Yumin Qi
yumin.qi at oracle.com
Tue Jun 2 18:48:43 UTC 2020
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?
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