RFR (L) 8194812: Extend class-data sharing to support the module path
Lois Foltan
lois.foltan at oracle.com
Tue Apr 10 15:11:25 UTC 2018
On 4/9/2018 7:08 PM, Calvin Cheung wrote:
> Hi Lois,
>
> Thanks for taking another look.
Hi Calvin,
Thanks for making the changes! The webrev looks good. I have some
minor questions about the interaction with JVMTI, but I am fine with
further JVMTI tests to be addressed if needed in a new bug. See my
JVMTI specific comments below.
>
> 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"?
Yes, incorrect statement on my part. AppCDS is disabled within
ClassLoaderExt::append_boot_classpath() which is called by
JvmtiEnv::AddToBootstrapClassLoaderSearch during JVMTI_PHASE_LIVE
phase. But AppCDS is not disabled if a Jvmti agent appends to the boot
class path entries during JVMTI_PHASE_ONLOAD.
> We do have existing tests under
> open/test/hotspot/jtreg/runtime/appcds/jvmti to test the interaction
> with jvmti.
So the test JvmtiAddPath.java looks like it uses JvmtiApp.java as the
"agent"? Calls within JvmtiApp.java to
AddToBootstrapClassLoaderSearch() all occur within the LIVE phase, thus
AppCDS would be disabled in every scenario, correct? My concern is that
there seems to be two aspects of Jvmti/AppCDS interaction that might be
nice to test:
1. What happens during DumpSharedSpaces when a call to
AddToBootstrapClassLoaderSearch() occurs during LIVE phase to the
classpath indexes? The classpath indexes have already been set and
would be incorrect if a call to AddToBootstrapClassLoaderSearch()
occurs. Should DumpSharedSpaces be disabled in this case like UseAppCDS is?
2. There is no test that verifies AppCDS interaction with an agent
calling AddToBootstrapClassLoaderSearch() during OnLoad phase, from a
Agent_OnLoad() method.
Again, if these are test cases that should be covered, I am fine with
addressing them in a new bug.
Thanks,
Lois
>
> 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