RFR (L) 8194812: Extend class-data sharing to support the module path
Lois Foltan
lois.foltan at oracle.com
Mon Apr 9 20:45:43 UTC 2018
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.
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]. There could be jvmti entries that have been added
during the ONLOAD phase. I believe in the LIVE phase, CDS is disabled,
correct?
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