RFR(L) JDK-8188791 Move AppCDS from closed repo to open repo
Dmitry Samersoff
dmitry.samersoff at bell-sw.com
Mon Nov 27 19:15:16 UTC 2017
Volker,
> @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.
I'll do it.
-Dmitry
On 11/27/2017 09:34 PM, Volker Simonis wrote:
> 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