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

Calvin Cheung calvin.cheung at oracle.com
Fri Apr 6 23:11:00 UTC 2018



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