(RFR) 8240245: Avoid calling is_shared_class_visible() in SystemDictionary::load_shared_class()
Yumin Qi
yumin.qi at oracle.com
Wed Jun 10 23:02:08 UTC 2020
Hi, Ioi and Calvin
I have updated a new version of the test case 8 of
runWithJarPath(String... extraRuntimeArgs) that adding --add-module to
the vmoption.
As -p <module-dir> does not automatically exports the modules name
without -m, I added '--add-module' to export com.bars. Since the shared
archive created with jars not modules, this led Main/Test loaded from
jars but CDS. So the checking modified with matching them from jars.
Resubmitted tier4 test but failed on NMT which already picked and
fix by Thomas Stuefe (not integrated).
http://cr.openjdk.java.net/~minqi/2020/8240245/webrev-05/
Thanks
Yumin
On 6/8/20 8:58 PM, Yumin Qi wrote:
> Hi, Ioi and Calvin
>
>
> New updated webrev:
> http://cr.openjdk.java.net/~minqi/2020/8240245/webrev-04/
>
> Passed tier1/2.
>
>
> Thanks
>
> Yumin
>
> On 6/8/20 10:21 AM, Ioi Lam wrote:
>>
>>
>> On 6/8/20 9:49 AM, Yumin Qi wrote:
>>> Hi, Ioi
>>>
>>> Thanks for the review.
>>>
>>> On 6/5/20 11:56 PM, Ioi Lam wrote:
>>>> 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:
>>>>
>>> Sure will add more checks.
>>>> ============
>>>>
>>>> @@ -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()?
>>>>
>>> Yes, should move to one time check place for the log.
>>>> ==============
>>>>
>>>> Also, I think we should have a test case where "Using optimized
>>>> module handling" is printed.
>>>>
>>> We already have, I will add check for the log info.
>>>> ===============
>>>>
>>>> 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;
>>>> }
>>>>
>>> Will add it back.
>>>> ==================
>>>>
>>>> Are the changes in classLoader.cpp still needed?
>>>>
>>> Better to move this function to classLoader.cpp, the reason is if
>>> you want to add/change something in classLoader.hpp, it will cause
>>> build failure, I think it is because some functions are inlined
>>> versions and in classLoader.inline.hpp. This may come from C++
>>> compiler, but anyway, if you don't like move it to classLoader.cpp,
>>> I could keep it unchanged.
>>>
>>
>> Oh I didn't realize that. Please keep your changes. I think it's an
>> improvement.
>>
>> Thanks
>> - Ioi
>>>
>>> Thanks
>>>
>>> Yumin
>>>
>>>> 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