(RFR) 8240245: Avoid calling is_shared_class_visible() in SystemDictionary::load_shared_class()
Yumin Qi
yumin.qi at oracle.com
Thu Jun 11 00:44:53 UTC 2020
Hi, Ioi and Calvin
Thanks for review!
Yumin
On 6/10/20 4:12 PM, Ioi Lam wrote:
> Hi Yumin,
>
> This version looks good to me. Thanks
>
> - Ioi
>
> On 6/10/20 4:02 PM, Yumin Qi wrote:
>> 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