RFR (L) 8194812: Extend class-data sharing to support the module path
Calvin Cheung
calvin.cheung at oracle.com
Tue Apr 10 17:31:07 UTC 2018
On 4/10/18, 8:11 AM, Lois Foltan wrote:
> 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.
Hi Lois,
Thanks again for re-reviewing the changes.
> 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.
I've filed JDK-8201375
<https://bugs.openjdk.java.net/browse/JDK-8201375> to follow-up on
adding more AppCDS/JVMTI tests.
thanks,
Calvin
>
>>
>> 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