RFR(L) JDK-8188791 Move AppCDS from closed repo to open repo
Dmitry Samersoff
dms at samersoff.net
Fri Nov 24 16:08:03 UTC 2017
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