RFR (L) 8194812: Extend class-data sharing to support the module path

Mikhailo Seledtsov mikhailo.seledtsov at oracle.com
Mon Apr 9 20:30:04 UTC 2018


Looks good to me,

Thank you,
Misha

On 4/9/18, 12:44 PM, Calvin Cheung wrote:
> Hi Misha,
>
> Thanks for reviewing the test changes.
>
> On 4/9/18, 11:13 AM, Mikhailo Seledtsov wrote:
>> Hi Calvin,
>>
>>   I have reviewed the test portion of your change. Overall the test 
>> changes look good. I have 2 general comments:
>>
>>   - use of "@build" section of the test header
>>       The "@build" should only be used for classes that are not 
>> explicitly used/invoked in the code.
>>       E.g. when passing a class to a child process in the command line
>>       E.g. using reflection to access class.
>>       In most cases for new tests you do not need the @build statements
>>
>>   - some of new tests are missing @requires vm.cds
> I've deleted the unnecessary @build statements and added the missing 
> @requires statements.
> Updated webrevs:
>
>     incremental: 
> http://cr.openjdk.java.net/~ccheung/8194812/webrev.02_to_03/
>
>     full: http://cr.openjdk.java.net/~ccheung/8194812/webrev.03/
>
> thanks,
> Calvin
>>
>> Thank you,
>> Misha
>>
>> On 4/8/18, 11:38 PM, Calvin Cheung wrote:
>>>
>>>
>>> On 4/6/18, 5:42 PM, Ioi Lam wrote:
>>>> Hi Calvin,
>>>>
>>>> The changes look good to me.
>>>>
>>>> If you want to avoid some verbosity, you can add this new utility 
>>>> method:
>>>>
>>>>       public Result assertSilentlyDisabledCDS(int exitCode, 
>>>> String... matches) throws Exception {
>>>>           return assertSilentlyDisabledCDS((out) -> {
>>>>                   out.shouldHaveExitValue(exitCode);
>>>>                   checkMatches(out, matches);
>>>>               });
>>>>       }
>>>>
>>>> That way, many of the common pattern like this:
>>>>
>>>>   90             .assertSilentlyDisabledCDS(out -> {
>>>>   91                 out.shouldHaveExitValue(0)
>>>>   92                    .shouldContain("I pass!");
>>>>   93             });
>>>>
>>>> can be simplified as
>>>>
>>>>   90             .assertSilentlyDisabledCDS(0, "I pass!");
>>> I've made the above simplification.
>>>
>>> Updated webrevs:
>>>
>>>     incremental: 
>>> http://cr.openjdk.java.net/~ccheung/8194812/webrev.01_to_02/
>>>
>>>     full: http://cr.openjdk.java.net/~ccheung/8194812/webrev.02/
>>>
>>> thanks,
>>> Calvin
>>>>
>>>>
>>>> Thanks
>>>>
>>>> - Ioi
>>>>
>>>>
>>>>
>>>> On 4/6/18 4:11 PM, Calvin Cheung wrote:
>>>>>
>>>>>
>>>>> On 4/6/18, 2:38 PM, Ioi Lam wrote:
>>>>>> On 4/6/18 1:05 PM, Calvin Cheung wrote:
>>>>>>
>>>>>>> Hi Ioi,
>>>>>>>
>>>>>>> Thanks for your suggestions. I've made most of the changes. I 
>>>>>>> just want to make sure something regarding [2].
>>>>>>>
>>>>>>> On 4/5/18, 9:38 PM, Ioi Lam wrote:
>>>>>>>> Hi Calvin,
>>>>>>>>
>>>>>>>> Some comments on the tests:
>>>>>>>>
>>>>>>>>
>>>>>>>> [1] For all the new tests, I think we should avoid using 
>>>>>>>> TestCommon.execCommon.
>>>>>>>> execCommon is error prone. See 
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8179249
>>>>>>>>
>>>>>>>> For example, in MainModuleOnly.java
>>>>>>>>
>>>>>>>> +        output = TestCommon.execCommon( "-Xlog:class+load=trace",
>>>>>>>> +                                        "-cp", 
>>>>>>>> destJar.toString(),
>>>>>>>> +                                        "--module-path", 
>>>>>>>> moduleDir.toString(),
>>>>>>>> +                                        "-m", TEST_MODULE1);
>>>>>>>> +        TestCommon.checkExecReturn(output, 0, true,
>>>>>>>> +              "[class,load] com.simple.Main source: shared 
>>>>>>>> objects file");
>>>>>>>>
>>>>>>>> In the past we often have direct checks on <output> which would 
>>>>>>>> fail
>>>>>>>> due to mapping failure.
>>>>>>>>
>>>>>>>> It's must safer (and more readable) to change the code to:
>>>>>>>>
>>>>>>>> TestCommon.run( "-Xlog:class+load=trace",
>>>>>>>>     "-cp", destJar.toString(),
>>>>>>>>     "--module-path", moduleDir.toString(),
>>>>>>>>     "-m", TEST_MODULE1)
>>>>>>>>   .assertNormalExit("[class,load] com.simple.Main source: 
>>>>>>>> shared objects file");
>>>>>>>>
>>>>>>>>
>>>>>>>> You can read comments near the CDSTestUtils.Result class for 
>>>>>>>> more information on
>>>>>>>> the different forms of assertXXX().
>>>>>>>>
>>>>>>>> Also, TestCommon.execModule has the same problems as 
>>>>>>>> execCommon. For example,
>>>>>>>> in ModulePathAndCP.java
>>>>>>>>
>>>>>>>> +        output = TestCommon.execModule(prefix,
>>>>>>>> +                                       null, // 
>>>>>>>> --upgrade-module-path
>>>>>>>> + moduleDir2.toString(), // --module-path
>>>>>>>> +                                       MAIN_MODULE); // -m
>>>>>>>> +        output.shouldHaveExitValue(0)
>>>>>>>> +              .shouldMatch(".class.load. com.greetings.Main 
>>>>>>>> source:.*com.greetings.jar")
>>>>>>>> +              .shouldMatch(".class.load. org.astro.World 
>>>>>>>> source:.*org.astro.jar");
>>>>>>>>
>>>>>>>> This code forgets to call checkExecReturn.
>>>>>>>>
>>>>>>>> I think it's better to use a new method 
>>>>>>>> TestCommon.runWithModule, like this:
>>>>>>>>
>>>>>>>>       http://cr.openjdk.java.net/~iklam/jdk11/runWithModule/
>>>>>>>>
>>>>>>>> Then the above code can be changed to
>>>>>>>>
>>>>>>>>     TestCommon.runWithModule(....)
>>>>>>>>       .assertNormalExit(out -> {
>>>>>>>>         out.shouldMatch(".class.load. com.greetings.Main 
>>>>>>>> source:.*com.greetings.jar")
>>>>>>>>            .shouldMatch(".class.load. org.astro.World 
>>>>>>>> source:.*org.astro.jar");
>>>>>>>>       });
>>>>>>>>
>>>>>>>>
>>>>>>>> [2] in BootAppendTests.java
>>>>>>>>
>>>>>>>>       output.shouldHaveExitValue(0);
>>>>>>>>       if (!TestCommon.isUnableToMap(output))
>>>>>>>>
>>>>>>>> Here, the isUnableToMap check is not necessarily because we
>>>>>>>> know for sure mapping will not happen.
>>>>>>>>
>>>>>>>> I think we should add a new 
>>>>>>>> CDSUtils.assertCDSSilentlyDisabled() for
>>>>>>>> for all the tests for --limit-modules, --patch-module and
>>>>>>>> --upgrade-module-path.
>>>>>>>>
>>>>>>>>
>>>>>>>>   TestCommon.run(
>>>>>>>>       appJar,
>>>>>>>>       "-Xbootclasspath/a:" + bootAppendJar,
>>>>>>>>       "--limit-modules", "java.base",
>>>>>>>>       "-XX:+TraceClassLoading",
>>>>>>>>       MAIN_CLASS,
>>>>>>>>       "Test #4", BOOT_APPEND_MODULE_CLASS, "true", "BOOT")
>>>>>>>>     .assertCDSSilentlyDisabled(output -> {
>>>>>>>>             output.shouldHaveExitValue(0);
>>>>>>>>                   .shouldMatch(".class.load. 
>>>>>>>> sun.nio.cs.ext.MyClass source:.*bootAppend.jar");
>>>>>>>>         });
>>>>>>> Using TestCommon.run() as is doesn't work; it failed in the 
>>>>>>> constructor of Result in the following check:
>>>>>>>
>>>>>>>                     output.shouldContain("sharing");
>>>>>>>
>>>>>>> I've added a  runSilentlyDisabledCDS() method in TestCommon.java:
>>>>>>>
>>>>>>>     // This should be used only when
>>>>>>>     // {--limit-modules, --patch-module, and/or 
>>>>>>> --upgrade-module-path}
>>>>>>>     // are specified, CDS is silently disabled for both 
>>>>>>> -Xshare:auto and -Xshare:on.
>>>>>>>     public static Result runSilentlyDisabledCDS(String... 
>>>>>>> suffix) throws Exception {
>>>>>>>         AppCDSOptions opts = (new AppCDSOptions());
>>>>>>>         opts.addSuffix(suffix).setCDSDisabled(true);
>>>>>>>         return new Result(opts, runWithArchive(opts));
>>>>>>>     }
>>>>>>>
>>>>>>> In CDSOptions, I added the following flag and method:
>>>>>>>
>>>>>>> diff --git a/test/lib/jdk/test/lib/cds/CDSOptions.java 
>>>>>>> b/test/lib/jdk/test/lib/cds/CDSOptions.java
>>>>>>> --- a/test/lib/jdk/test/lib/cds/CDSOptions.java
>>>>>>> +++ b/test/lib/jdk/test/lib/cds/CDSOptions.java
>>>>>>> @@ -36,6 +36,10 @@
>>>>>>>      // Most of tests will use '-version'
>>>>>>>      public boolean useVersion = true;
>>>>>>>
>>>>>>> +    // When {--limit-modules, --patch-module, and/or 
>>>>>>> --upgrade-module-path}
>>>>>>> +    // are specified, CDS is silently disabled for both 
>>>>>>> -Xshare:auto and -Xshare:on.
>>>>>>> +    public boolean cdsDisabled = false;
>>>>>>> +
>>>>>>>
>>>>>>>      public CDSOptions() {
>>>>>>>      }
>>>>>>> @@ -68,4 +72,9 @@
>>>>>>>          this.useVersion = use;
>>>>>>>          return this;
>>>>>>>      }
>>>>>>> +
>>>>>>> +    public CDSOptions setCDSDisabled(boolean disabled) {
>>>>>>> +        this.cdsDisabled = disabled;
>>>>>>> +        return this;
>>>>>>> +    }
>>>>>>>  }
>>>>>>>
>>>>>>> In the constructor of Result, the cdsDisabled flag will be checked:
>>>>>>>
>>>>>>> bash-4.2$ hg diff CDSTestUtils.java
>>>>>>> diff --git a/test/lib/jdk/test/lib/cds/CDSTestUtils.java 
>>>>>>> b/test/lib/jdk/test/lib/cds/CDSTestUtils.java
>>>>>>> --- a/test/lib/jdk/test/lib/cds/CDSTestUtils.java
>>>>>>> +++ b/test/lib/jdk/test/lib/cds/CDSTestUtils.java
>>>>>>> @@ -126,7 +126,9 @@
>>>>>>>              hasNormalExit     = (!hasMappingFailure) && 
>>>>>>> (output.getExitValue() == 0);
>>>>>>>
>>>>>>>              if (hasNormalExit) {
>>>>>>> -                if ("on".equals(options.xShareMode) && 
>>>>>>> output.getStderr().contains("java version")) {
>>>>>>> +                if ("on".equals(options.xShareMode) &&
>>>>>>> +                    output.getStderr().contains("java version") &&
>>>>>>> +                    !(options.cdsDisabled)) {
>>>>>>>                      // "-showversion" is always passed in the 
>>>>>>> command-line by the execXXX methods.
>>>>>>>                      // During normal exit, we require that the 
>>>>>>> VM to show that sharing was enabled.
>>>>>>>                      output.shouldContain("sharing");
>>>>>>>
>>>>>>> Does the above changes look ok to you?
>>>>>> How about changing the code inside Result() constructor to be
>>>>>>
>>>>>> if ("on".equals(options.xShareMode) && 
>>>>>> output.getStderr().contains("java version") && 
>>>>>> !output.getStderr().contains("CDS is disabled")) {...}
>>>>> This works too.
>>>>>>
>>>>>> I think the "CDS is disabled" message can be changed to be more 
>>>>>> exact, something like " warning: CDS is disabled when the ". That 
>>>>>> way it won't accidentally match output by the test program.
>>>>> I've adjusted the message.
>>>>>
>>>>> updated webrevs:
>>>>>
>>>>>     incremental: 
>>>>> http://cr.openjdk.java.net/~ccheung/8194812/webrev.00_to_01/
>>>>>
>>>>>     full: http://cr.openjdk.java.net/~ccheung/8194812/webrev.01/
>>>>>
>>>>> thanks,
>>>>> Calvin
>>>>>
>>>>>>
>>>>>> Thanks
>>>>>> - Ioi
>>>>>>> I'll do more testing before posting an updated webrev.
>>>>>>>
>>>>>>> thanks,
>>>>>>> Calvin
>>>>>>>>
>>>>>>>>
>>>>>>>>   // When {--limit-modules, --patch-module, and/or 
>>>>>>>> --upgrade-module-path}
>>>>>>>>   // are specified, CDS is silently disabled for both 
>>>>>>>> -Xshare:auto and -Xshare:on.
>>>>>>>>   public Result assertSilentlyDisabledCDS(Checker checker) 
>>>>>>>> throws Exception {
>>>>>>>>       if (hasMappingFailure) {
>>>>>>>>           throw new RuntimeException("Unexpected mapping 
>>>>>>>> failure");
>>>>>>>>       }
>>>>>>>>       // this comes from a JVM warning message.
>>>>>>>>       output.shouldContain("CDS is disabled");
>>>>>>>>
>>>>>>>>       checker.check(output);
>>>>>>>>       return this;
>>>>>>>>   }
>>>>>>>>
>>>>>>>>
>>>>>>>> [3] Some tests have this pattern:
>>>>>>>>
>>>>>>>>    OutputAnalyzer output = TestCommon.createArchive(
>>>>>>>>                                    null, appClasses,
>>>>>>>>                                    "--module-path", 
>>>>>>>> moduleDir.toString(),
>>>>>>>>                                    "--add-modules",
>>>>>>>>                                    MAIN_MODULE1 + "," + 
>>>>>>>> MAIN_MODULE2);
>>>>>>>>    TestCommon.checkExecReturn(output, 0, true, "Loading classes 
>>>>>>>> to share");
>>>>>>>>
>>>>>>>> Here you shouldn't use TestCommon.checkExecReturn, because 
>>>>>>>> mapping failure
>>>>>>>> will never happen during -Xshare:dump.
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> - Ioi
>>>>>>>>
>>>>>>>>
>>>>>>>> On 4/2/18 2:30 PM, Calvin Cheung wrote:
>>>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8194812
>>>>>>>>>
>>>>>>>>> webrev: http://cr.openjdk.java.net/~ccheung/8194812/webrev.00/
>>>>>>>>>
>>>>>>>>> Design write-up: 
>>>>>>>>> http://cr.openjdk.java.net/~ccheung/8194812/write-up/module-path-design.pdf
>>>>>>>>>
>>>>>>>>> Testing: hs-tier1 through hs-tier4 (in progress, no new 
>>>>>>>>> failures so far).
>>>>>>>>>
>>>>>>>>> thanks,
>>>>>>>>> Calvin
>>>>>>>>
>>>>>>
>>>>


More information about the hotspot-runtime-dev mailing list