(RFR) 8240245: Avoid calling is_shared_class_visible() in SystemDictionary::load_shared_class()
Ioi Lam
ioi.lam at oracle.com
Mon Jun 8 17:21:47 UTC 2020
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