RFR (L) 8194812: Extend class-data sharing to support the module path
Calvin Cheung
calvin.cheung at oracle.com
Mon Apr 9 23:08:54 UTC 2018
Hi Lois,
Thanks for taking another look.
On 4/9/18, 1:45 PM, Lois Foltan wrote:
> On 4/9/2018 3:44 PM, Calvin Cheung wrote:
>
>> 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/
>
> Hi Calvin,
> Looks good. Thank you for making the changes in
> ClassLoader::record_result(), the section for the boot loader at line
> #1627-1628 look correct. Some more review comments:
>
> src/share/vm/classLoader.[h/c]pp:
> - line #750, ClassLoader::update_module_path_entry_list()
> My understanding of correct coding convention is to not obtain the
> current thread by calling Thread::current(). You should add a TRAPS
> parameter to everyone of these methods in the call hierarchy:
> ClassLoader::initialize_module_path()
> calls:
> ClassLoaderExt::setup_module_paths()
> calls:
> ClassLoaderExt::setup_module_search_path() - call to
> Thread::current(); at line #92
> calls:
> ClassLoaderExt::process_module_table()
> calls:
> ClassLoader::setup_module_search_path(path) -
> calls from a loop:
> ClassLoader::update_module_path_entry_list() - call to
> Thread::current(); at line #750
>
> So, several calls to Thread::current() result when again
> Threads::create_vm() has the THREAD to pass down.
>
> src/share/vm/classLoader.cpp:
> - line #1600, ClassLoader::record_result() - same comment, needs a
> TRAPS parameter.
I've fixed the above.
Updated webrevs:
incremental:
http://cr.openjdk.java.net/~ccheung/8194812/webrev.03_to_04/
full: http://cr.openjdk.java.net/~ccheung/8194812/webrev.04/
>
> src/share/vm/classLoader.cpp:
> - line #1617 & 1629 - the comment should indicate that its the boot
> loader append path which consists of [-Xbootclasspath/a]; | [jvmti
> appended entries].
I've updated the comment.
> There could be jvmti entries that have been added during the ONLOAD
> phase. I believe in the LIVE phase, CDS is disabled, correct?
Not sure what you meant by "CDS is disabled"?
We do have existing tests under
open/test/hotspot/jtreg/runtime/appcds/jvmti to test the interaction
with jvmti.
thanks,
Calvin
>
> Thanks,
> Lois
>
>> 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