RFR (L) 8194812: Extend class-data sharing to support the module path
Ioi Lam
ioi.lam at oracle.com
Sat Apr 7 00:42:31 UTC 2018
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!");
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