RFR(L) JDK-8188791 Move AppCDS from closed repo to open repo
Ioi Lam
ioi.lam at oracle.com
Fri Nov 17 15:51:08 UTC 2017
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