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

Calvin Cheung calvin.cheung at oracle.com
Mon Apr 9 06:38:47 UTC 2018



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