RFR (L) 8194812: Extend class-data sharing to support the module path

Calvin Cheung calvin.cheung at oracle.com
Fri Apr 6 20:05:39 UTC 2018


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?
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