(RFR) 8240245: Avoid calling is_shared_class_visible() in SystemDictionary::load_shared_class()
Ioi Lam
ioi.lam at oracle.com
Sat Jun 6 06:56:15 UTC 2020
Hi Yumin,
This looks good.
For OptimizeModuleHandlingTest, I think we can add more checks to make
sure the classes are loaded from the correct locations. I've just added
2 examples for where the Main class is loaded, but I think you can add
more checks for the Test class as well:
============
@@ -66,6 +66,8 @@
private static String CLASS_NOT_FOUND_MESSAGE =
"java.lang.ClassNotFoundException: com.foos.Test";
private static String OPTIMIZE_ENABLED = "Using optimized module
handling";
private static String OPTIMIZE_DISABLED =
"use_optimized_module_handling disabled";
+ private static String MAIN_FROM_JAR =
"class,load.*com.bars.Main.*[.]jar";
+ private static String MAIN_FROM_CDS =
"class,load.*com.bars.Main.*shared objects file";
private static String CP_SEPARATOR = Platform.isWindows() ? ";" : ":";
@@ -163,20 +165,24 @@
});
tty("6. run with CDS on, with module paths set correctly");
TestCommon.run("-Xlog:cds",
+ "-Xlog:class+load",
"-p", libsDir.toString(),
"-m", MAIN_MODULE)
.assertNormalExit(out -> {
out.shouldContain(CLASS_FOUND_MESSAGE)
.shouldContain(OPTIMIZE_DISABLED)
+ .shouldMatch(MAIN_FROM_CDS) // archived Main class
is for module only
.shouldNotContain(OPTIMIZE_ENABLED);
});
tty("7. run with CDS on, with jar on path");
TestCommon.run("-Xlog:cds",
+ "-Xlog:class+load",
"-cp", mainJar + CP_SEPARATOR + testJar,
MAIN_CLASS)
.assertNormalExit(out -> {
out.shouldContain(CLASS_FOUND_MESSAGE)
.shouldContain(OPTIMIZE_DISABLED)
+ .shouldMatch(MAIN_FROM_JAR) // archived Main class
is for module only, not for -cp
.shouldNotContain(OPTIMIZE_ENABLED);
});
==============
if (MetaspaceShared::use_optimized_module_handling()) {
log_info(cds)("Using optimized module handling");
return true;
}
I think this may be too verbose, since it print one line per loaded
class. How about moving the log to the end of
MetaspaceShared::map_archives()?
==============
Also, I think we should have a test case where "Using optimized module
handling" is printed.
===============
I think it's a good idea to keep this assert from your older version:
if (MetaspaceShared::use_optimized_module_handling()) {
assert(SystemDictionary::is_shared_class_visible_impl(class_name,
ik, pkg_entry, class_loader, THREAD), "Sanity check");
return true;
}
==================
Are the changes in classLoader.cpp still needed?
Thanks
- Ioi
On 6/5/20 4:02 PM, Yumin Qi wrote:
> 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