RFR (L) 8194812: Extend class-data sharing to support the module path
Mikhailo Seledtsov
mikhailo.seledtsov at oracle.com
Mon Apr 9 18:13:05 UTC 2018
Hi Calvin,
I have reviewed the test portion of your change. Overall the test
changes look good. I have 2 general comments:
- use of "@build" section of the test header
The "@build" should only be used for classes that are not
explicitly used/invoked in the code.
E.g. when passing a class to a child process in the command line
E.g. using reflection to access class.
In most cases for new tests you do not need the @build statements
- some of new tests are missing @requires vm.cds
Thank you,
Misha
On 4/8/18, 11:38 PM, Calvin Cheung wrote:
>
>
> 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