RFR(M) JDK-8188791 Move AppCDS implementation from closed repo to open repo

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR(M) JDK-8188791 Move AppCDS implementation from closed repo to open repo

Ioi Lam
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
Reply | Threaded
Open this post in threaded view
|

Re: RFR(M) JDK-8188791 Move AppCDS implementation from closed repo to open repo

Jiangli Zhou
Hi Ioi,

The move looks good to me.

Thanks,
Jiangli

> On Oct 12, 2017, at 4:48 PM, Ioi Lam <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: RFR(M) JDK-8188791 Move AppCDS implementation from closed repo to open repo

Calvin Cheung
In reply to this post by Ioi Lam
Hi Ioi,

The AppCDS source code relocation looks good to me.

thanks,
Calvin

On 10/12/17, 4:48 PM, 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR(M) JDK-8188791 Move AppCDS implementation from closed repo to open repo

Dmitry Samersoff-3
In reply to this post by Ioi Lam
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


Reply | Threaded
Open this post in threaded view
|

Re: RFR(M) JDK-8188791 Move AppCDS implementation from closed repo to open repo

Ioi Lam
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
>

Reply | Threaded
Open this post in threaded view
|

JDK-8190336 Make sure that AppCDS works on aarch64 platform

Dmitry Samersoff-3
Ioi,

I can confirm that current patchset works on aarch64 as expected.

It passes all tests in runtime/SharedArchiveFile.

I'll re-check it when other version of fix/more tests become available.

-Dmitry


On 30.10.2017 18:52, 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
>>
>


Reply | Threaded
Open this post in threaded view
|

Re: RFR(L) JDK-8188791 Move AppCDS from closed repo to open repo

Ioi Lam
In reply to this post by Ioi Lam
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
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(L) JDK-8188791 Move AppCDS from closed repo to open repo

Mikhailo Seledtsov
Test changes look good,


Misha


On 10/31/2017 07:43 PM, 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
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(L) JDK-8188791 Move AppCDS from closed repo to open repo

Dmitry Samersoff-3
In reply to this post by Ioi Lam
Ioi,

I tested new patch on aarch64 and it works fine with the changes below[1].

Also tests doesn't work with exploded image - is it possible to check
for this case and produce appropriate error message?

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
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(L) JDK-8188791 Move AppCDS from closed repo to open repo

yumin qi-2
In reply to this post by Ioi Lam
HI, Ioi


Only one minor comment,  in cdsoffsets.hpp:
#ifndef CLOSED_VM_PRIMS_CDSWHITEBOX_HPP

Should it be changed to SHARE_PRIMS_CDOFFSETS_HPP to keep it with same
style with other head files.
Since the hotspot directory changed, I don't know if all head files should
change to follow that (changes).

Thanks
Yumin

On Tue, Oct 31, 2017 at 7:43 PM, Ioi Lam <[hidden email]> 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/s
>>> ystemDictionaryShared.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
>>>>
>>>
>>>
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(L) JDK-8188791 Move AppCDS from closed repo to open repo

Ioi Lam
Hi Yumin,

Thanks for the review. I have changed it to SHARE_PRIMS_CDOFFSETS_HPP.

- Ioi


On 11/15/17 9:52 AM, yumin qi wrote:

> HI, Ioi
>
> Only one minor comment,  in cdsoffsets.hpp:
> #ifndef CLOSED_VM_PRIMS_CDSWHITEBOX_HPP
> Should it be changed to SHARE_PRIMS_CDOFFSETS_HPP to keep it with same
> style with other head files.
> Since the hotspot directory changed, I don't know if all head files
> should change to follow that (changes).
> Thanks
> Yumin
>
> On Tue, Oct 31, 2017 at 7:43 PM, Ioi Lam <[hidden email]
> <mailto:[hidden email]>> 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/
>     <http://cr.openjdk.java.net/%7Eiklam/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/
>                 <http://cr.openjdk.java.net/%7Eiklam/jdk10/8188791-open-appcds-impl.v01/>
>                 https://bugs.openjdk.java.net/browse/JDK-8188791
>                 <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
>                 <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
>
>
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(L) JDK-8188791 Move AppCDS from closed repo to open repo

Ioi Lam
In reply to this post by Dmitry Samersoff-3
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

Reply | Threaded
Open this post in threaded view
|

Re: RFR(L) JDK-8188791 Move AppCDS from closed repo to open repo

Ioi Lam
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
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(L) JDK-8188791 Move AppCDS from closed repo to open repo

Dmitry Samersoff-2
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
>>
>


Reply | Threaded
Open this post in threaded view
|

Re: RFR(L) JDK-8188791 Move AppCDS from closed repo to open repo

Volker Simonis
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.

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.

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.

Thank you and best regards,
Volker


On Thu, Nov 16, 2017 at 12:22 PM, Dmitry Samersoff <[hidden email]> 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
>>>
>>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(L) JDK-8188791 Move AppCDS from closed repo to open repo

Ioi Lam
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 <[hidden email]> 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
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(L) JDK-8188791 Move AppCDS from closed repo to open repo

Volker Simonis
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 <[hidden email]> 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 <[hidden email]>
>> 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
>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(L) JDK-8188791 Move AppCDS from closed repo to open repo

Dmitry Samersoff-2
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 <[hidden email]> 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 <[hidden email]>
>>> 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
>>>>
>>>>
>>