RFR(L) JDK-8188791 Move AppCDS from closed repo to open repo
Volker Simonis
volker.simonis at gmail.com
Mon Nov 27 18:34:49 UTC 2017
Hi Ioi, Dmitry,
I've finished my testing on Linux/ppc64, Linux/ppc64le, Linux/s390 and
AIX. All the tests passed successfully (with the corresponding extra
ppc/s390 patches currently under review).
I did run the tests with the following add-on to your change, which
enables AppCDS with custom class loaders on all 64-bit Linux, Solaris
and AIX platforms:
http://cr.openjdk.java.net/~simonis/webrevs/2017/8188791-open-appcds.v02.tests-addon/
The only change required in HotSpot is in "classListParser.cpp" to
enable the feature on the named platforms. The other changes just
enable the additional tests on all 64-bit Linux, Solaris and AIX
platfroms.
@Ioi: can you please append this add-on to your change?
@Dmitry: can you please re-build and re-run the tests on 64-bit ARM
just to make sure everything is working? Notice, that with my add-ons
about 20 more tests than before will be executed, but that was no
problem an the other Linux platforms I've tested.
Thank you and best regards,
Volker
On Fri, Nov 24, 2017 at 5:08 PM, Dmitry Samersoff <dms at samersoff.net> wrote:
> Volker,
>
>> Currently I'm building and running some tests
>> and I hope that until Monday we may even extend the list of supported
>> platforms by Linux/s390. I'm not sure about AArch (Dmitry?), but if
>> that works as well, the check could be simplified to allow all the
>> 64-bit Linux platforms.
>
> Yes, I added similar patch for Linux/AARCH64 and it works.
>
> So we should simplify the check.
>
> -Dmitry
>
> On 11/24/2017 05:51 PM, Volker Simonis wrote:
>> Hi Ioi,
>>
>> you patch is not applying any more after "8187118: Remove appending
>> -cp path to the boot class path at AppCDS dump time" was pushed to
>> jdk/hs because of conflicts in classLoaderExt.hpp.
>>
>> Could you please rebase your change upon the newest version of jdk/hs ?
>>
>> By the way, on ppc64 all the tests passed after I found and fixed a
>> trivial bug (I've opened "JDK-8191770: [ppc64] Fix CDS: don't rewrite
>> invokefinal if DumpSharedSpaces" for it).
>>
>> Can you please explain why AppCDS with custom class loaders is
>> currently restricted to Linux/x86_64 and 64-bit Solaris?
>>
>> With the following small patch:
>>
>> diff -r 23deffd3a9c4 src/hotspot/share/classfile/classListParser.cpp
>> --- a/src/hotspot/share/classfile/classListParser.cpp Fri Nov 24
>> 15:37:19 2017 +0100
>> +++ b/src/hotspot/share/classfile/classListParser.cpp Fri Nov 24
>> 15:38:21 2017 +0100
>> @@ -272,6 +272,7 @@
>> // during archive dumping.
>> InstanceKlass* ClassListParser::load_class_from_source(Symbol*
>> class_name, TRAPS) {
>> #if !((defined(LINUX) && defined(X86) && defined(_LP64)) || \
>> + (defined(PPC) && defined(_LP64)) || \
>> (defined(SOLARIS) && defined(_LP64)))
>> // The only supported platforms are: (1) Linux/AMD64; (2) Solaris/64-bit
>> error("AppCDS custom class loaders not supported on this platform");
>> diff -r 23deffd3a9c4
>> test/hotspot/jtreg/runtime/appcds/customLoader/UnsupportedPlatforms.java
>> --- a/test/hotspot/jtreg/runtime/appcds/customLoader/UnsupportedPlatforms.java
>> Fri Nov 24 15:37:19 2017 +0100
>> +++ b/test/hotspot/jtreg/runtime/appcds/customLoader/UnsupportedPlatforms.java
>> Fri Nov 24 15:38:21 2017 +0100
>> @@ -56,6 +56,7 @@
>> OutputAnalyzer out = TestCommon.dump(appJar, classlist);
>>
>> if ((Platform.isSolaris() && Platform.is64bit()) ||
>> + (Platform.isLinux() && Platform.isPPC()) ||
>> (Platform.isLinux() && Platform.isX64())) {
>> out.shouldNotContain(PLATFORM_NOT_SUPPORTED_WARNING);
>> out.shouldHaveExitValue(0);
>>
>>
>> I could at least pass all the relevant jtreg tests on Linux/ppc64. So
>> if there are no other hidden reasons I'd kindly ask you to add this
>> patch to you change. I'm currently testing on AIX and I hope the tests
>> will also succeed. If this will be the case, the list in the
>> aforementioned patch could also be extended by AIX.
>>
>> Finally I've also did some small fixes on s390 (opened "8191863:
>> [s390] Fix CDS: some bytecode rewriting doesn't depend on
>> RewriteControl" for it). Currently I'm building and running some tests
>> and I hope that until Monday we may even extend the list of supported
>> platforms by Linux/s390. I'm not sure about AArch (Dmitry?), but if
>> that works as well, the check could be simplified to allow all the
>> 64-bit Linux platforms.
>>
>> Thank you and best regards,
>> Volker
>>
>> On Fri, Nov 17, 2017 at 4:51 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
>>> Hi Volker,
>>>
>>> Thanks for trying the patch out.
>>>
>>>
>>> On 11/17/17 5:42 AM, Volker Simonis wrote:
>>>>
>>>> Hi Ioi,
>>>>
>>>> I'm just trying to verify if AppCDS will be running on our ppc/s390/AIX
>>>> ports.
>>>>
>>>> I applied your patch to the current jdk-hs repo as of today and first
>>>> of all I see the same compilation problems already reported by Dmitry
>>>> before. Can you please update you webrev with the corresponding
>>>> changes or create a new one. That will help other who wish to look at
>>>> this change.
>>>
>>>
>>> I've updated the patch inside the webrev. You can download it from
>>>
>>> http://cr.openjdk.java.net/~iklam/jdk10/8188791-open-appcds.v02/open.patch
>>>
>>>
>>> The previous patch (which you had the problems with) is still available as
>>>
>>> http://cr.openjdk.java.net/~iklam/jdk10/8188791-open-appcds.v02/open.patch.old
>>>
>>> I will post a new webrev, but it's big and I might be running out of quota
>>> from cr.openjdk.java.net, so I need to figure out how to handle that.
>>>
>>> The jdk/hs changeset corresponding to the new patch is the following:
>>>
>>> changeset: 47860:564882d918d4
>>> user: zgu
>>> date: Thu Nov 16 20:21:11 2017 -0500
>>> files: src/hotspot/share/services/memTracker.cpp
>>> description:
>>> 8190357: NMT: Include metadata information in NMT final report when
>>> PrintNMTStatistics is on
>>> Summary: Include metadata information in NMT final report
>>> Reviewed-by: adinn, stuefe
>>>
>>>> After building, I ran the appcds jtreg tests (i.e.
>>>> test/hotspot/jtreg/runtime/appcds/) under Linux/x86_64 and again I see
>>>> the same four failures like Dmitry.
>>>
>>> Those are also due to merge issues. With the updated patch, I passed all
>>> tests under on Linux/x64:
>>>
>>> test/hotspot/jtreg/runtime/SharedArchiveFile
>>> test/hotspot/jtreg/runtime/CDSCompressedKPtrs
>>> test/hotspot/jtreg/runtime/modules/PatchModule/PatchModuleCDS.java
>>> test/hotspot/jtreg/runtime/appcds
>>>
>>> $ jtreg -J-Djavatest.maxOutputSize=1000000 -conc:28
>>> -testjdk:/home/iklam/jdk/bld/opencdso-fastdebug/images/jdk
>>> -compilejdk:/home/iklam/jdk/bld/opencds/images/jdk -verbose:2 -timeout:4.0
>>> -retain -agentvm
>>> -exclude:/home/iklam/jdk/opencds/closed/test/hotspot/jtreg/ProblemList.txt
>>> -exclude:/home/iklam/jdk/opencds/open/test/hotspot/jtreg/ProblemList.txt
>>> -nativepath:/home/iklam/jdk/bld/opencdso/images/jdk/../../images/test/hotspot/jtreg/native
>>> -nr -w /home/iklam/tmp/jtreg/work -r /home/iklam/tmp/jtreg/report/
>>> open/test/hotspot/jtreg/runtime/SharedArchiveFile
>>> open/test/hotspot/jtreg/runtime/CDSCompressedKPtrs
>>> open/test/hotspot/jtreg/runtime/modules/PatchModule/PatchModuleCDS.java
>>> open/test/hotspot/jtreg/runtime/appcds
>>> [...]
>>> Test results: passed: 128
>>> Results written to /jdk/tmp/jtreg/work
>>>
>>>
>>>> Is that expected? I see a lot more errors on Linux/ppc64le but before
>>>> going into more detail I'd like to know what is the correct baseline
>>>> on which we can rely.
>>>>
>>>> Finally, I saw that with a product build, I get two additional test
>>>> failures:
>>>>
>>>> runtime/appcds/ProhibitedPackage.java
>>>> runtime/appcds/DumpClassList.java
>>>>
>>>> because of:
>>>>
>>>> Error: VM option 'PrintSystemDictionaryAtExit' is notproduct and is
>>>> available only in debug version of VM.
>>>>
>>>> So these two tests should probably be adjusted to only run for
>>>> slow/fastdebug builds.
>>>
>>>
>>> You're correct. I will exclude those tests from product builds, and file an
>>> RFE to fix them later.
>>>
>>> Thanks
>>> - Ioi
>>>
>>>> Thank you and best regards,
>>>> Volker
>>>>
>>>>
>>>> On Thu, Nov 16, 2017 at 12:22 PM, Dmitry Samersoff <dms at samersoff.net>
>>>> wrote:
>>>>>
>>>>> Ioi,
>>>>>
>>>>> Thank you!
>>>>>
>>>>> I did some additional testing and find that four tests
>>>>> fails the same way on both x86_64 and AARCH64 (see below).
>>>>>
>>>>> runtime/appcds/VerifierTest_0.java
>>>>> runtime/appcds/cacheObject/GCStressTest.java
>>>>> runtime/appcds/cacheObject/RedefineClassTest.java
>>>>> runtime/appcds/sharedStrings/InternSharedString.java
>>>>>
>>>>> Is it expected?
>>>>>
>>>>> test result: Failed. Execution failed: `main' threw exception:
>>>>> java.lang.RuntimeException: 'Please remove the unverifiable cl
>>>>> asses' missing from stdout/stderr
>>>>>
>>>>> Exception in thread "main" java.lang.RuntimeException: FAILED.
>>>>> GCStressApp_nonshared_string should not be shared
>>>>> at GCStressApp.main(GCStressApp.java:73)
>>>>>
>>>>>
>>>>> FAILED. buzzshould not be shared
>>>>>
>>>>> Exception in thread "main" java.lang.RuntimeException: Failed:
>>>>> unexpected shared string.
>>>>> at InternStringTest.main(InternStringTest.java:63)
>>>>>
>>>>> -Dmitry
>>>>>
>>>>> On 16.11.2017 03:10, Ioi Lam wrote:
>>>>>>
>>>>>> I filed:
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8191375 Add high-level jtreg
>>>>>> VMProps to filter out CDS tests
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8191374 Improve error message
>>>>>> when CDS is not supported
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> - Ioi
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 11/15/17 4:01 PM, Ioi Lam wrote:
>>>>>>>
>>>>>>> Hi Dmitry,
>>>>>>>
>>>>>>> Thanks for the review.
>>>>>>>
>>>>>>> On 11/6/17 7:07 AM, Dmitry Samersoff wrote:
>>>>>>>
>>>>>>>> Ioi,
>>>>>>>>
>>>>>>>> I tested new patch on aarch64 and it works fine with the changes
>>>>>>>> below[1].
>>>>>>>
>>>>>>> I've fixed it. Thanks for the patch.
>>>>>>>>
>>>>>>>> Also tests doesn't work with exploded image - is it possible to check
>>>>>>>> for this case and produce appropriate error message?
>>>>>>>
>>>>>>> CDS is not supported on exploded images. I think the best thing to do
>>>>>>> is to add something like "@requires vm.cds" into the test cases
>>>>>>> (similar to the use of "@require vm.aot" in other tests). That way,
>>>>>>> none of the CDS tests will be executed for
>>>>>>>
>>>>>>> It's too late to do this in JDK 10 now, but I will file an RFE to do
>>>>>>> it in the next release.
>>>>>>>
>>>>>>> I'll also file another RFE to print a better message. Today if you use
>>>>>>> an exploded build the error message is kind of confusing:
>>>>>>>
>>>>>>> $ ./jdk/bin/java -Xshare:dump
>>>>>>> Error: non-empty directory '/jdk/bld/cons/jdk/modules/java.base'
>>>>>>> Hint: enable -Xlog:class+path=info to diagnose the failure
>>>>>>> Error occurred during initialization of VM
>>>>>>> CDS allows only empty directories in archived classpaths
>>>>>>>
>>>>>>> It should say something like "CDS is not supported on exploded images".
>>>>>>>
>>>>>>> Thanks
>>>>>>> - Ioi
>>>>>>>
>>>>>>>> 1.
>>>>>>>> diff -r dbf9cec6a568 src/hotspot/share/classfile/classListParser.cpp
>>>>>>>> --- a/src/hotspot/share/classfile/classListParser.cpp Mon Nov 06
>>>>>>>> 09:45:23 2017 +0000
>>>>>>>> +++ b/src/hotspot/share/classfile/classListParser.cpp Mon Nov 06
>>>>>>>> 15:02:51 2017 +0000
>>>>>>>> @@ -31,7 +31,6 @@
>>>>>>>> -#include "prims/jvm.h"
>>>>>>>>
>>>>>>>> diff -r dbf9cec6a568
>>>>>>>> src/hotspot/share/classfile/systemDictionaryShared.cpp
>>>>>>>> --- a/src/hotspot/share/classfile/systemDictionaryShared.cpp Mon Nov
>>>>>>>> 06 09:45:23 2017 +0000
>>>>>>>> +++ b/src/hotspot/share/classfile/systemDictionaryShared.cpp Mon Nov
>>>>>>>> 06 15:02:51 2017 +0000
>>>>>>>> @@ -518,7 +518,7 @@
>>>>>>>>
>>>>>>>> {
>>>>>>>> MutexLocker mu(SystemDictionary_lock, THREAD);
>>>>>>>> - Klass* check = find_class(d_index, d_hash, name, dictionary);
>>>>>>>> + Klass* check = dictionary->find_class(d_index, d_hash, name);
>>>>>>>> if (check != NULL) {
>>>>>>>> return InstanceKlass::cast(check);
>>>>>>>> }
>>>>>>>>
>>>>>>>> -Dmitry
>>>>>>>>
>>>>>>>> On 11/01/2017 05:43 AM, Ioi Lam wrote:
>>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> Here's the new webrev for both the AppCDS implementation and tests.
>>>>>>>>> During internal review of the JEP, we have decided to integrate both
>>>>>>>>> implementation and tests at the same time.
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~iklam/jdk10/8188791-open-appcds.v02/
>>>>>>>>>
>>>>>>>>> As mentioned before, most of the "diffs" shown in this webrev are the
>>>>>>>>> result of copying the closed source files on top of files of the same
>>>>>>>>> name in the open repo. So in reviewing, instead of focusing on what's
>>>>>>>>> "changed", please focus on the entire content of the new version of
>>>>>>>>> each
>>>>>>>>> file.
>>>>>>>>>
>>>>>>>>> Testing: I did an OpenJDK linux-x64 build (without Oracle closed
>>>>>>>>> sources) and all the new appcds tests passed.
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>>
>>>>>>>>> - Ioi
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 10/30/17 8:52 AM, Ioi Lam wrote:
>>>>>>>>>>
>>>>>>>>>> Hi Dmitry,
>>>>>>>>>>
>>>>>>>>>> In the latest JDK 10 repo, is_jrt has been renamed to
>>>>>>>>>> is_modules_image. Please change the code accordingly.
>>>>>>>>>>
>>>>>>>>>> I will post my latest diff soon, with some test cases as well.
>>>>>>>>>>
>>>>>>>>>> Thanks
>>>>>>>>>>
>>>>>>>>>> - Ioi
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 10/30/17 4:04 AM, Dmitry Samersoff wrote:
>>>>>>>>>>>
>>>>>>>>>>> Ioi,
>>>>>>>>>>>
>>>>>>>>>>> I'd tried to apply your patch to latest open JDK10 and
>>>>>>>>>>> the compilation fails with:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> /root/dsamersoff/ESC/appcds/hs/src/hotspot/share/classfile/systemDictionaryShared.cpp:400:16:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> error: ‘class SharedClassPathEntry’ has no member named ‘is_jrt’
>>>>>>>>>>>
>>>>>>>>>>> Did I miss something?
>>>>>>>>>>>
>>>>>>>>>>> -Dmitry
>>>>>>>>>>>
>>>>>>>>>>> On 13.10.2017 02:48, Ioi Lam wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> Please review this change set.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> http://cr.openjdk.java.net/~iklam/jdk10/8188791-open-appcds-impl.v01/
>>>>>>>>>>>>
>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8188791
>>>>>>>>>>>>
>>>>>>>>>>>> This is the first step of implementing the following JEP, which
>>>>>>>>>>>> moves
>>>>>>>>>>>> AppCDS from
>>>>>>>>>>>> closed repos into the openjdk repo:
>>>>>>>>>>>>
>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8185996
>>>>>>>>>>>>
>>>>>>>>>>>> In JDK 9, significant portion of AppCDS code resided in the closed
>>>>>>>>>>>> repo.
>>>>>>>>>>>> As part
>>>>>>>>>>>> of the open-sourcing effort of JDK 18.3, we will move the source
>>>>>>>>>>>> code
>>>>>>>>>>>> into the
>>>>>>>>>>>> open repo.
>>>>>>>>>>>>
>>>>>>>>>>>> In this changeset, the code is moved verbatim as much as
>>>>>>>>>>>> possible. The
>>>>>>>>>>>> intention is
>>>>>>>>>>>> only to relocate the sources, not to changing existing behaviors,
>>>>>>>>>>>> and not
>>>>>>>>>>>> to do any sort of refactoring.
>>>>>>>>>>>>
>>>>>>>>>>>> Most of the "diffs" shown in this webrev are the result of
>>>>>>>>>>>> copying the
>>>>>>>>>>>> closed source
>>>>>>>>>>>> files on top of files of the same name in the open repo. So in
>>>>>>>>>>>> reviewing, instead of
>>>>>>>>>>>> focusing on what's "changed", it's better to focus on the entire
>>>>>>>>>>>> content
>>>>>>>>>>>> of the new
>>>>>>>>>>>> version of each file.
>>>>>>>>>>>>
>>>>>>>>>>>> The only functional change in this task is that the UseAppCDS
>>>>>>>>>>>> flag is
>>>>>>>>>>>> changed from
>>>>>>>>>>>> a "commercial" flag to a regular "product" flag. This is because
>>>>>>>>>>>> "commercial"
>>>>>>>>>>>> flags are not supported by the OpenJDK build.
>>>>>>>>>>>>
>>>>>>>>>>>> Source code refactoring may be desirable, because the old
>>>>>>>>>>>> open/closed
>>>>>>>>>>>> source
>>>>>>>>>>>> code structure had introduced some intermediary APIs to connect
>>>>>>>>>>>> code
>>>>>>>>>>>> between
>>>>>>>>>>>> the two repos. Such API should be removed in a separate RFE.
>>>>>>>>>>>>
>>>>>>>>>>>> Also, some AppCDS tests are currently in the closed repo. These
>>>>>>>>>>>> tests
>>>>>>>>>>>> will be
>>>>>>>>>>>> moved in a separate task. See JDK-8188792 for details.
>>>>>>>>>>>>
>>>>>>>>>>>> All the AppCDS tests (currently still in closed sources) passed
>>>>>>>>>>>> with
>>>>>>>>>>>> both Oracle JDK
>>>>>>>>>>>> and OpenJDK.
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks
>>>>>>>>>>>> - Ioi
>>>>>
>>>>>
>>>
>
>
More information about the hotspot-runtime-dev
mailing list