(RFR) 8240245: Avoid calling is_shared_class_visible() in SystemDictionary::load_shared_class()
Calvin Cheung
calvin.cheung at oracle.com
Wed Jun 10 23:52:44 UTC 2020
Hi Yumin,
The updated webrev looks good.
thanks,
Calvin
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