(RFR) 8240245: Avoid calling is_shared_class_visible() in SystemDictionary::load_shared_class()
Yumin Qi
yumin.qi at oracle.com
Tue Jun 9 03:58:34 UTC 2020
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