RFR (L) 8194812: Extend class-data sharing to support the module path
Calvin Cheung
calvin.cheung at oracle.com
Mon Apr 9 19:44:10 UTC 2018
Hi Misha,
Thanks for reviewing the test changes.
On 4/9/18, 11:13 AM, Mikhailo Seledtsov wrote:
> 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
I've deleted the unnecessary @build statements and added the missing
@requires statements.
Updated webrevs:
incremental:
http://cr.openjdk.java.net/~ccheung/8194812/webrev.02_to_03/
full: http://cr.openjdk.java.net/~ccheung/8194812/webrev.03/
thanks,
Calvin
>
> 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