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