(RFR) 8240245: Avoid calling is_shared_class_visible() in SystemDictionary::load_shared_class()
Yumin Qi
yumin.qi at oracle.com
Mon Jun 8 16:49:17 UTC 2020
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.
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