RFR (L) 8194812: Extend class-data sharing to support the module path
Ioi Lam
ioi.lam at oracle.com
Fri Apr 6 21:38:27 UTC 2018
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")) {...}
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.
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