RFR: 8259070: Add jcmd option to dump CDS

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

RFR: 8259070: Add jcmd option to dump CDS

Yumin Qi-3
Hi, Please review

  Added jcmd option for dumping CDS archive during application runtime. Before this change, user has to dump shared archive in two steps: first run application with
  `java -XX:DumpLoadedClassList=<classlist> .... `  
 to collect shareable class names and saved in file `<classlist>` , then
  `java -Xshare:dump -XX:SharedClassListFile=<classlist> -XX:SharedArchiveFile=<archivefile> ...`
  With this change, user can use jcmd to dump CDS without going through above steps. Also user can choose a moment during the app runtime  to dump an archive.
   The bug is associated with the CSR: https://bugs.openjdk.java.net/browse/JDK-8259798 which has been approved.
   New added jcmd option:
   `jcmd <pid or AppName> VM.cds static_dump <filename>`
   or
    `jcmd <pid or AppName> VM.cds dynamic_dump <filename>`
  To dump dynamic archive, requires start app with newly added flag `-XX:+RecordDynamicDumpInfo`, with this flag, some information related to dynamic dump like loader constraints will be recorded. Note the dumping process changed some object memory locations so for dumping dynamic archive, can only done once for a running app. For static dump, user can dump multiple times against same process.
   The file name is optional, if the file name is not supplied, the file name will take format of `java_pid<number>_static.jsa` or `java_pid<number>_dynamic.jsa` for static and dynamic respectively. The `<number>` is the application process ID.

  Tests: tier1,tier2,tier3,tier4

Thanks
Yumin

-------------

Commit messages:
 - 8259070: Add jcmd option to dump CDS

Changes: https://git.openjdk.java.net/jdk/pull/2737/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2737&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8259070
  Stats: 502 lines in 13 files changed: 496 ins; 0 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2737.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2737/head:pull/2737

PR: https://git.openjdk.java.net/jdk/pull/2737
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8259070: Add jcmd option to dump CDS

Ioi Lam-2
On Sat, 27 Feb 2021 05:19:01 GMT, Thomas Stuefe <[hidden email]> wrote:

>> src/hotspot/share/memory/metaspaceShared.cpp line 799:
>>
>>> 797:       if (strstr(file_name, ".jsa") == nullptr) {
>>> 798:         os::snprintf(filename, sizeof(filename), "%s.jsa", file_name);
>>> 799:         file = filename;
>>
>> This could potentially overflow the buffer. I think it's best to just leave `file_name` alone. If the user doesn't want the `.jsa` extension, that's fine. Similarly, we don't add `.jsa` to `-XX:ArchiveClassesAtExit` or `-XX:SharedArchiveFile`.
>
> How would it overflow? But I agree, I would not add jsa extension if the user did not specify one. I dislike when programs do that.

`file_name` is user input that comes from the jcmd, so it can be arbitrarily long and exceed JVM_MAXPATHLEN characters.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2737
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8259070: Add jcmd option to dump CDS

Ioi Lam-2
In reply to this post by Yumin Qi-3
On Sat, 27 Feb 2021 05:48:04 GMT, Thomas Stuefe <[hidden email]> wrote:

> I really would consider rewriting the whole thing using posix_spawn. Since JDK15 I think posix_spawn is the default for Runtime.exec, so we know it works well. I would also do this in a separate RFE.
>
> Alternatively, you could call into java and use Runtime.exec().

I think we should call into Java and use `Runtime.exec()`. Running a subprocess is very complicated, so we shouldn't try to duplicate the code in the VM.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2737
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8259070: Add jcmd option to dump CDS

tstuefe
In reply to this post by Ioi Lam-2
Oh right, then it could get truncated, but should not overflow.

On Sat, Feb 27, 2021 at 7:15 PM Ioi Lam <[hidden email]> wrote:

> On Sat, 27 Feb 2021 05:19:01 GMT, Thomas Stuefe <[hidden email]>
> wrote:
>
> >> src/hotspot/share/memory/metaspaceShared.cpp line 799:
> >>
> >>> 797:       if (strstr(file_name, ".jsa") == nullptr) {
> >>> 798:         os::snprintf(filename, sizeof(filename), "%s.jsa",
> file_name);
> >>> 799:         file = filename;
> >>
> >> This could potentially overflow the buffer. I think it's best to just
> leave `file_name` alone. If the user doesn't want the `.jsa` extension,
> that's fine. Similarly, we don't add `.jsa` to `-XX:ArchiveClassesAtExit`
> or `-XX:SharedArchiveFile`.
> >
> > How would it overflow? But I agree, I would not add jsa extension if the
> user did not specify one. I dislike when programs do that.
>
> `file_name` is user input that comes from the jcmd, so it can be
> arbitrarily long and exceed JVM_MAXPATHLEN characters.
>
> -------------
>
> PR: https://git.openjdk.java.net/jdk/pull/2737
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8259070: Add jcmd option to dump CDS

Yumin Qi-3
In reply to this post by Yumin Qi-3
On Fri, 26 Feb 2021 22:05:06 GMT, Calvin Cheung <[hidden email]> wrote:

>> Hi, Please review
>>
>>   Added jcmd option for dumping CDS archive during application runtime. Before this change, user has to dump shared archive in two steps: first run application with
>>   `java -XX:DumpLoadedClassList=<classlist> .... `  
>>  to collect shareable class names and saved in file `<classlist>` , then
>>   `java -Xshare:dump -XX:SharedClassListFile=<classlist> -XX:SharedArchiveFile=<archivefile> ...`
>>   With this change, user can use jcmd to dump CDS without going through above steps. Also user can choose a moment during the app runtime  to dump an archive.
>>    The bug is associated with the CSR: https://bugs.openjdk.java.net/browse/JDK-8259798 which has been approved.
>>    New added jcmd option:
>>    `jcmd <pid or AppName> VM.cds static_dump <filename>`
>>    or
>>     `jcmd <pid or AppName> VM.cds dynamic_dump <filename>`
>>   To dump dynamic archive, requires start app with newly added flag `-XX:+RecordDynamicDumpInfo`, with this flag, some information related to dynamic dump like loader constraints will be recorded. Note the dumping process changed some object memory locations so for dumping dynamic archive, can only done once for a running app. For static dump, user can dump multiple times against same process.
>>    The file name is optional, if the file name is not supplied, the file name will take format of `java_pid<number>_static.jsa` or `java_pid<number>_dynamic.jsa` for static and dynamic respectively. The `<number>` is the application process ID.
>>
>>   Tests: tier1,tier2,tier3,tier4
>>
>> Thanks
>> Yumin
>
> Below are my comments...

@calvinccheung @iklam @tstuefe Thanks for review! I will use CDS.java to implement the dumping for next update. This way, we deal with CDS related code in a central place. Also using Runtime.exec will clear your concern, plus the code will be more readable though it will add some bridge functions between java/vm.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2737
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8259070: Add jcmd option to dump CDS [v2]

Yumin Qi-3
In reply to this post by Yumin Qi-3
> Hi, Please review
>
>   Added jcmd option for dumping CDS archive during application runtime. Before this change, user has to dump shared archive in two steps: first run application with
>   `java -XX:DumpLoadedClassList=<classlist> .... `  
>  to collect shareable class names and saved in file `<classlist>` , then
>   `java -Xshare:dump -XX:SharedClassListFile=<classlist> -XX:SharedArchiveFile=<archivefile> ...`
>   With this change, user can use jcmd to dump CDS without going through above steps. Also user can choose a moment during the app runtime  to dump an archive.
>    The bug is associated with the CSR: https://bugs.openjdk.java.net/browse/JDK-8259798 which has been approved.
>    New added jcmd option:
>    `jcmd <pid or AppName> VM.cds static_dump <filename>`
>    or
>     `jcmd <pid or AppName> VM.cds dynamic_dump <filename>`
>   To dump dynamic archive, requires start app with newly added flag `-XX:+RecordDynamicDumpInfo`, with this flag, some information related to dynamic dump like loader constraints will be recorded. Note the dumping process changed some object memory locations so for dumping dynamic archive, can only done once for a running app. For static dump, user can dump multiple times against same process.
>    The file name is optional, if the file name is not supplied, the file name will take format of `java_pid<number>_static.jsa` or `java_pid<number>_dynamic.jsa` for static and dynamic respectively. The `<number>` is the application process ID.
>
>   Tests: tier1,tier2,tier3,tier4
>
> Thanks
> Yumin

Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:

  Add function CDS.dumpSharedArchive in java to dump shared archive

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2737/files
  - new: https://git.openjdk.java.net/jdk/pull/2737/files/e371456c..d486c06e

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2737&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2737&range=00-01

  Stats: 450 lines in 13 files changed: 258 ins; 156 del; 36 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2737.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2737/head:pull/2737

PR: https://git.openjdk.java.net/jdk/pull/2737
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8259070: Add jcmd option to dump CDS [v3]

Yumin Qi-3
In reply to this post by Yumin Qi-3
> Hi, Please review
>
>   Added jcmd option for dumping CDS archive during application runtime. Before this change, user has to dump shared archive in two steps: first run application with
>   `java -XX:DumpLoadedClassList=<classlist> .... `  
>  to collect shareable class names and saved in file `<classlist>` , then
>   `java -Xshare:dump -XX:SharedClassListFile=<classlist> -XX:SharedArchiveFile=<archivefile> ...`
>   With this change, user can use jcmd to dump CDS without going through above steps. Also user can choose a moment during the app runtime  to dump an archive.
>    The bug is associated with the CSR: https://bugs.openjdk.java.net/browse/JDK-8259798 which has been approved.
>    New added jcmd option:
>    `jcmd <pid or AppName> VM.cds static_dump <filename>`
>    or
>     `jcmd <pid or AppName> VM.cds dynamic_dump <filename>`
>   To dump dynamic archive, requires start app with newly added flag `-XX:+RecordDynamicDumpInfo`, with this flag, some information related to dynamic dump like loader constraints will be recorded. Note the dumping process changed some object memory locations so for dumping dynamic archive, can only done once for a running app. For static dump, user can dump multiple times against same process.
>    The file name is optional, if the file name is not supplied, the file name will take format of `java_pid<number>_static.jsa` or `java_pid<number>_dynamic.jsa` for static and dynamic respectively. The `<number>` is the application process ID.
>
>   Tests: tier1,tier2,tier3,tier4
>
> Thanks
> Yumin

Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:

  Fix white space in CDS.java

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2737/files
  - new: https://git.openjdk.java.net/jdk/pull/2737/files/d486c06e..bfa71577

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2737&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2737&range=01-02

  Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2737.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2737/head:pull/2737

PR: https://git.openjdk.java.net/jdk/pull/2737
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8259070: Add jcmd option to dump CDS [v4]

Yumin Qi-3
In reply to this post by Yumin Qi-3
> Hi, Please review
>
>   Added jcmd option for dumping CDS archive during application runtime. Before this change, user has to dump shared archive in two steps: first run application with
>   `java -XX:DumpLoadedClassList=<classlist> .... `  
>  to collect shareable class names and saved in file `<classlist>` , then
>   `java -Xshare:dump -XX:SharedClassListFile=<classlist> -XX:SharedArchiveFile=<archivefile> ...`
>   With this change, user can use jcmd to dump CDS without going through above steps. Also user can choose a moment during the app runtime  to dump an archive.
>    The bug is associated with the CSR: https://bugs.openjdk.java.net/browse/JDK-8259798 which has been approved.
>    New added jcmd option:
>    `jcmd <pid or AppName> VM.cds static_dump <filename>`
>    or
>     `jcmd <pid or AppName> VM.cds dynamic_dump <filename>`
>   To dump dynamic archive, requires start app with newly added flag `-XX:+RecordDynamicDumpInfo`, with this flag, some information related to dynamic dump like loader constraints will be recorded. Note the dumping process changed some object memory locations so for dumping dynamic archive, can only done once for a running app. For static dump, user can dump multiple times against same process.
>    The file name is optional, if the file name is not supplied, the file name will take format of `java_pid<number>_static.jsa` or `java_pid<number>_dynamic.jsa` for static and dynamic respectively. The `<number>` is the application process ID.
>
>   Tests: tier1,tier2,tier3,tier4
>
> Thanks
> Yumin

Yumin Qi has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:

 - Fix filter more flags to exclude in static dump, add more test cases
 - Merge branch 'master' into jdk-8259070
 - Fix white space in CDS.java
 - Add function CDS.dumpSharedArchive in java to dump shared archive
 - 8259070: Add jcmd option to dump CDS

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2737/files
  - new: https://git.openjdk.java.net/jdk/pull/2737/files/bfa71577..a9010f8f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2737&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2737&range=02-03

  Stats: 13690 lines in 458 files changed: 7913 ins; 3760 del; 2017 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2737.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2737/head:pull/2737

PR: https://git.openjdk.java.net/jdk/pull/2737
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8259070: Add jcmd option to dump CDS [v3]

Yumin Qi-3
In reply to this post by Yumin Qi-3
On Wed, 10 Mar 2021 04:18:29 GMT, Ioi Lam <[hidden email]> wrote:

>> Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Fix white space in CDS.java
>
> src/hotspot/share/services/diagnosticCommand.cpp line 1124:
>
>> 1122:   }
>> 1123:   Symbol* cds_name  = vmSymbols::jdk_internal_misc_CDS();
>> 1124:   Klass*  cds_klass = SystemDictionary::resolve_or_null(cds_name, THREAD);
>
> Should be `cds_klass = SystemDictionary::resolve_or_fail(cds_name, CHECK);`

Changed to use resolve_or_fail.

> src/java.base/share/classes/jdk/internal/misc/CDS.java line 213:
>
>> 211:               testStr.contains("-XX:+DynamicDumpSharedSpaces") ||
>> 212:               testStr.contains("-XX:+RecordDynamicDumpInfo");
>> 213:     }
>
> The following flags should also be excluded:
>
> - -XX:-DumpSharedSpaces
> - -Xshare:
> - -XX:SharedClassListFile=
> - -XX:SharedArchiveFile=
> - -XX:ArchiveClassesAtExit=
> - -XX:+UseSharedSpaces
> - -XX:+RequireSharedSpaces
>
> We also need to have a few test cases when the LingeredApp is started with these flags.

Added String[] for those flags to check.

> src/java.base/share/classes/jdk/internal/misc/CDS.java line 262:
>
>> 260:                 String line;
>> 261:                 InputStreamReader isr = new InputStreamReader(proc.getInputStream());
>> 262:                 BufferedReader rdr = new BufferedReader(isr);
>
> Also, I think the output should always be logged. Otherwise if an error happens, it's very difficult for the user to diagnose (and they won't know about the "CDS.Debug" property).

Yes, done with separate thread.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2737
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8259070: Add jcmd option to dump CDS [v4]

Yumin Qi-3
In reply to this post by Yumin Qi-3
On Fri, 26 Feb 2021 22:46:07 GMT, Ioi Lam <[hidden email]> wrote:

>> Yumin Qi has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:
>>
>>  - Fix filter more flags to exclude in static dump, add more test cases
>>  - Merge branch 'master' into jdk-8259070
>>  - Fix white space in CDS.java
>>  - Add function CDS.dumpSharedArchive in java to dump shared archive
>>  - 8259070: Add jcmd option to dump CDS
>
> src/hotspot/share/runtime/arguments.cpp line 3525:
>
>> 3523:       os::free(SharedDynamicArchivePath);
>> 3524:       SharedDynamicArchivePath = nullptr;
>> 3525:     }
>
> Is this necessary?

When do dynamic dump, we set SharedDynamicArchivePath to the given file name, after that, restore the original value so free the old one to prevent memory leak.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2737
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8259070: Add jcmd option to dump CDS [v5]

Yumin Qi-3
In reply to this post by Yumin Qi-3
> Hi, Please review
>
>   Added jcmd option for dumping CDS archive during application runtime. Before this change, user has to dump shared archive in two steps: first run application with
>   `java -XX:DumpLoadedClassList=<classlist> .... `  
>  to collect shareable class names and saved in file `<classlist>` , then
>   `java -Xshare:dump -XX:SharedClassListFile=<classlist> -XX:SharedArchiveFile=<archivefile> ...`
>   With this change, user can use jcmd to dump CDS without going through above steps. Also user can choose a moment during the app runtime  to dump an archive.
>    The bug is associated with the CSR: https://bugs.openjdk.java.net/browse/JDK-8259798 which has been approved.
>    New added jcmd option:
>    `jcmd <pid or AppName> VM.cds static_dump <filename>`
>    or
>     `jcmd <pid or AppName> VM.cds dynamic_dump <filename>`
>   To dump dynamic archive, requires start app with newly added flag `-XX:+RecordDynamicDumpInfo`, with this flag, some information related to dynamic dump like loader constraints will be recorded. Note the dumping process changed some object memory locations so for dumping dynamic archive, can only done once for a running app. For static dump, user can dump multiple times against same process.
>    The file name is optional, if the file name is not supplied, the file name will take format of `java_pid<number>_static.jsa` or `java_pid<number>_dynamic.jsa` for static and dynamic respectively. The `<number>` is the application process ID.
>
>   Tests: tier1,tier2,tier3,tier4
>
> Thanks
> Yumin

Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:

  Fix according to review comment and add more tests

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2737/files
  - new: https://git.openjdk.java.net/jdk/pull/2737/files/a9010f8f..e882a074

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2737&range=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2737&range=03-04

  Stats: 142 lines in 7 files changed: 61 ins; 35 del; 46 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2737.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2737/head:pull/2737

PR: https://git.openjdk.java.net/jdk/pull/2737
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8259070: Add jcmd option to dump CDS [v6]

Yumin Qi-3
In reply to this post by Yumin Qi-3
> Hi, Please review
>
>   Added jcmd option for dumping CDS archive during application runtime. Before this change, user has to dump shared archive in two steps: first run application with
>   `java -XX:DumpLoadedClassList=<classlist> .... `  
>  to collect shareable class names and saved in file `<classlist>` , then
>   `java -Xshare:dump -XX:SharedClassListFile=<classlist> -XX:SharedArchiveFile=<archivefile> ...`
>   With this change, user can use jcmd to dump CDS without going through above steps. Also user can choose a moment during the app runtime  to dump an archive.
>    The bug is associated with the CSR: https://bugs.openjdk.java.net/browse/JDK-8259798 which has been approved.
>    New added jcmd option:
>    `jcmd <pid or AppName> VM.cds static_dump <filename>`
>    or
>     `jcmd <pid or AppName> VM.cds dynamic_dump <filename>`
>   To dump dynamic archive, requires start app with newly added flag `-XX:+RecordDynamicDumpInfo`, with this flag, some information related to dynamic dump like loader constraints will be recorded. Note the dumping process changed some object memory locations so for dumping dynamic archive, can only done once for a running app. For static dump, user can dump multiple times against same process.
>    The file name is optional, if the file name is not supplied, the file name will take format of `java_pid<number>_static.jsa` or `java_pid<number>_dynamic.jsa` for static and dynamic respectively. The `<number>` is the application process ID.
>
>   Tests: tier1,tier2,tier3,tier4
>
> Thanks
> Yumin

Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:

  Remove redundant check for if a class is shareable

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2737/files
  - new: https://git.openjdk.java.net/jdk/pull/2737/files/e882a074..3834f042

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2737&range=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2737&range=04-05

  Stats: 52 lines in 2 files changed: 1 ins; 33 del; 18 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2737.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2737/head:pull/2737

PR: https://git.openjdk.java.net/jdk/pull/2737
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8259070: Add jcmd option to dump CDS [v6]

Calvin Cheung
On Wed, 24 Mar 2021 15:35:55 GMT, Yumin Qi <[hidden email]> wrote:

>> Hi, Please review
>>
>>   Added jcmd option for dumping CDS archive during application runtime. Before this change, user has to dump shared archive in two steps: first run application with
>>   `java -XX:DumpLoadedClassList=<classlist> .... `  
>>  to collect shareable class names and saved in file `<classlist>` , then
>>   `java -Xshare:dump -XX:SharedClassListFile=<classlist> -XX:SharedArchiveFile=<archivefile> ...`
>>   With this change, user can use jcmd to dump CDS without going through above steps. Also user can choose a moment during the app runtime  to dump an archive.
>>    The bug is associated with the CSR: https://bugs.openjdk.java.net/browse/JDK-8259798 which has been approved.
>>    New added jcmd option:
>>    `jcmd <pid or AppName> VM.cds static_dump <filename>`
>>    or
>>     `jcmd <pid or AppName> VM.cds dynamic_dump <filename>`
>>   To dump dynamic archive, requires start app with newly added flag `-XX:+RecordDynamicDumpInfo`, with this flag, some information related to dynamic dump like loader constraints will be recorded. Note the dumping process changed some object memory locations so for dumping dynamic archive, can only done once for a running app. For static dump, user can dump multiple times against same process.
>>    The file name is optional, if the file name is not supplied, the file name will take format of `java_pid<number>_static.jsa` or `java_pid<number>_dynamic.jsa` for static and dynamic respectively. The `<number>` is the application process ID.
>>
>>   Tests: tier1,tier2,tier3,tier4
>>
>> Thanks
>> Yumin
>
> Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:
>
>   Remove redundant check for if a class is shareable

src/hotspot/share/oops/instanceKlass.cpp line 4269:

> 4267:         }
> 4268:
> 4269:         if (class_loader == NULL && ClassLoader::contains_append_entry(stream->source())) {

Since the above has been removed, I think the `ClassLoader::contains_append_entry()` function can be removed too.

test/hotspot/jtreg/runtime/cds/appcds/jcmd/JCmdTest.java line 59:

> 57: public class JCmdTest {
> 58:     static final String TEST_CLASS[] = {"LingeredTestApp", "jdk/test/lib/apps/LingeredApp"};
> 59:     static final String TEST_JAR   = "test.jar";

`TEST_JAR` is unused.

test/hotspot/jtreg/runtime/cds/appcds/jcmd/JCmdTest.java line 168:

> 166:          "-XX:ArchiveClassesAtExit=tmp.jsa",
> 167:          "-Xshare:auto",
> 168:          "-Xshare:on"};

The `excludeFlags` doesn't match the ones in CDS.java.

226     private static String[] excludeFlags = {
227          "-XX:DumpLoadedClassList=",
228          "-XX:+DumpSharedSpaces",
229          "-XX:+DynamicDumpSharedSpaces",
230          "-XX:+RecordDynamicDumpInfo",
231          "-Xshare:",
232          "-XX:SharedClassListFile=",
233          "-XX:SharedArchiveFile=",
234          "-XX:ArchiveClassesAtExit=",
235          "-XX:+UseSharedSpaces",
236          "-XX:+RequireSharedSpaces"};

-------------

PR: https://git.openjdk.java.net/jdk/pull/2737
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8259070: Add jcmd option to dump CDS [v6]

Calvin Cheung
In reply to this post by Yumin Qi-3
On Wed, 24 Mar 2021 15:35:55 GMT, Yumin Qi <[hidden email]> wrote:

>> Hi, Please review
>>
>>   Added jcmd option for dumping CDS archive during application runtime. Before this change, user has to dump shared archive in two steps: first run application with
>>   `java -XX:DumpLoadedClassList=<classlist> .... `  
>>  to collect shareable class names and saved in file `<classlist>` , then
>>   `java -Xshare:dump -XX:SharedClassListFile=<classlist> -XX:SharedArchiveFile=<archivefile> ...`
>>   With this change, user can use jcmd to dump CDS without going through above steps. Also user can choose a moment during the app runtime  to dump an archive.
>>    The bug is associated with the CSR: https://bugs.openjdk.java.net/browse/JDK-8259798 which has been approved.
>>    New added jcmd option:
>>    `jcmd <pid or AppName> VM.cds static_dump <filename>`
>>    or
>>     `jcmd <pid or AppName> VM.cds dynamic_dump <filename>`
>>   To dump dynamic archive, requires start app with newly added flag `-XX:+RecordDynamicDumpInfo`, with this flag, some information related to dynamic dump like loader constraints will be recorded. Note the dumping process changed some object memory locations so for dumping dynamic archive, can only done once for a running app. For static dump, user can dump multiple times against same process.
>>    The file name is optional, if the file name is not supplied, the file name will take format of `java_pid<number>_static.jsa` or `java_pid<number>_dynamic.jsa` for static and dynamic respectively. The `<number>` is the application process ID.
>>
>>   Tests: tier1,tier2,tier3,tier4
>>
>> Thanks
>> Yumin
>
> Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:
>
>   Remove redundant check for if a class is shareable

test/hotspot/jtreg/runtime/cds/appcds/jcmd/JCmdTest.java line 184:

> 182:             test(SUBCMD_STATIC_DUMP, null, pid, EXPECT_PASS);
> 183:         }
> 184:         app.stopApp();

For successful dumping cases like the above, you may want to add a runtime case making sure one of the classes in the jar is loaded from the archive.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2737
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8259070: Add jcmd option to dump CDS [v6]

Calvin Cheung
In reply to this post by Yumin Qi-3
On Wed, 24 Mar 2021 15:35:55 GMT, Yumin Qi <[hidden email]> wrote:

>> Hi, Please review
>>
>>   Added jcmd option for dumping CDS archive during application runtime. Before this change, user has to dump shared archive in two steps: first run application with
>>   `java -XX:DumpLoadedClassList=<classlist> .... `  
>>  to collect shareable class names and saved in file `<classlist>` , then
>>   `java -Xshare:dump -XX:SharedClassListFile=<classlist> -XX:SharedArchiveFile=<archivefile> ...`
>>   With this change, user can use jcmd to dump CDS without going through above steps. Also user can choose a moment during the app runtime  to dump an archive.
>>    The bug is associated with the CSR: https://bugs.openjdk.java.net/browse/JDK-8259798 which has been approved.
>>    New added jcmd option:
>>    `jcmd <pid or AppName> VM.cds static_dump <filename>`
>>    or
>>     `jcmd <pid or AppName> VM.cds dynamic_dump <filename>`
>>   To dump dynamic archive, requires start app with newly added flag `-XX:+RecordDynamicDumpInfo`, with this flag, some information related to dynamic dump like loader constraints will be recorded. Note the dumping process changed some object memory locations so for dumping dynamic archive, can only done once for a running app. For static dump, user can dump multiple times against same process.
>>    The file name is optional, if the file name is not supplied, the file name will take format of `java_pid<number>_static.jsa` or `java_pid<number>_dynamic.jsa` for static and dynamic respectively. The `<number>` is the application process ID.
>>
>>   Tests: tier1,tier2,tier3,tier4
>>
>> Thanks
>> Yumin
>
> Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:
>
>   Remove redundant check for if a class is shareable

Looks good. Few comments below.
Also, the vmSymbols.h and diagnosticCommand.hpp need copyright update.

Thanks,
Calvin

-------------

Changes requested by ccheung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2737
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8259070: Add jcmd option to dump CDS [v7]

Ioi Lam-2
In reply to this post by Yumin Qi-3
On Tue, 30 Mar 2021 03:48:08 GMT, Yumin Qi <[hidden email]> wrote:

>> Hi, Please review
>>
>>   Added jcmd option for dumping CDS archive during application runtime. Before this change, user has to dump shared archive in two steps: first run application with
>>   `java -XX:DumpLoadedClassList=<classlist> .... `  
>>  to collect shareable class names and saved in file `<classlist>` , then
>>   `java -Xshare:dump -XX:SharedClassListFile=<classlist> -XX:SharedArchiveFile=<archivefile> ...`
>>   With this change, user can use jcmd to dump CDS without going through above steps. Also user can choose a moment during the app runtime  to dump an archive.
>>    The bug is associated with the CSR: https://bugs.openjdk.java.net/browse/JDK-8259798 which has been approved.
>>    New added jcmd option:
>>    `jcmd <pid or AppName> VM.cds static_dump <filename>`
>>    or
>>     `jcmd <pid or AppName> VM.cds dynamic_dump <filename>`
>>   To dump dynamic archive, requires start app with newly added flag `-XX:+RecordDynamicDumpInfo`, with this flag, some information related to dynamic dump like loader constraints will be recorded. Note the dumping process changed some object memory locations so for dumping dynamic archive, can only done once for a running app. For static dump, user can dump multiple times against same process.
>>    The file name is optional, if the file name is not supplied, the file name will take format of `java_pid<number>_static.jsa` or `java_pid<number>_dynamic.jsa` for static and dynamic respectively. The `<number>` is the application process ID.
>>
>>   Tests: tier1,tier2,tier3,tier4
>>
>> Thanks
>> Yumin
>
> Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:
>
>   Remove CDS.getVMArguments, changed to use VM.getRuntimeVMArguments. Removed unused function from ClassLoader. Improved InstanceKlass::is_shareable() and related test. Added more test scenarios.

src/java.base/share/classes/jdk/internal/misc/CDS.java line 311:

> 309:             // done, delete classlist file.
> 310:             if (fileList.exists()) {
> 311:                 // fileList.delete();

Is the comment unintentional?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2737
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8259070: Add jcmd option to dump CDS [v4]

Yumin Qi-3
In reply to this post by Yumin Qi-3
On Fri, 19 Mar 2021 05:39:25 GMT, Ioi Lam <[hidden email]> wrote:

>> Yumin Qi has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:
>>
>>  - Fix filter more flags to exclude in static dump, add more test cases
>>  - Merge branch 'master' into jdk-8259070
>>  - Fix white space in CDS.java
>>  - Add function CDS.dumpSharedArchive in java to dump shared archive
>>  - 8259070: Add jcmd option to dump CDS
>
> Changes requested by iklam (Reviewer).

@iklam Thanks. Yes, the comment out for the deletion is unintentional for test only and forgot to revert. I will revert it. Also I will merge with upstream. Since this repo has no merge from upstream for long time, it may cause conflicts. If too many conflicts to  resolve I would like to withdraw this one and resubmit as a new PR. Thanks.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2737
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8259070: Add jcmd option to dump CDS [v8]

Yumin Qi-3
In reply to this post by Yumin Qi-3
> Hi, Please review
>
>   Added jcmd option for dumping CDS archive during application runtime. Before this change, user has to dump shared archive in two steps: first run application with
>   `java -XX:DumpLoadedClassList=<classlist> .... `  
>  to collect shareable class names and saved in file `<classlist>` , then
>   `java -Xshare:dump -XX:SharedClassListFile=<classlist> -XX:SharedArchiveFile=<archivefile> ...`
>   With this change, user can use jcmd to dump CDS without going through above steps. Also user can choose a moment during the app runtime  to dump an archive.
>    The bug is associated with the CSR: https://bugs.openjdk.java.net/browse/JDK-8259798 which has been approved.
>    New added jcmd option:
>    `jcmd <pid or AppName> VM.cds static_dump <filename>`
>    or
>     `jcmd <pid or AppName> VM.cds dynamic_dump <filename>`
>   To dump dynamic archive, requires start app with newly added flag `-XX:+RecordDynamicDumpInfo`, with this flag, some information related to dynamic dump like loader constraints will be recorded. Note the dumping process changed some object memory locations so for dumping dynamic archive, can only done once for a running app. For static dump, user can dump multiple times against same process.
>    The file name is optional, if the file name is not supplied, the file name will take format of `java_pid<number>_static.jsa` or `java_pid<number>_dynamic.jsa` for static and dynamic respectively. The `<number>` is the application process ID.
>
>   Tests: tier1,tier2,tier3,tier4
>
> Thanks
> Yumin

Yumin Qi has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 10 commits:

 - Fix revert unintentionally comment, merge master.
 - Merge branch 'master' of ssh://github.com/yminqi/jdk into jdk-8259070
 - Remove CDS.getVMArguments, changed to use VM.getRuntimeVMArguments. Removed unused function from ClassLoader. Improved InstanceKlass::is_shareable() and related test. Added more test scenarios.
 - Remove redundant check for if a class is shareable
 - Fix according to review comment and add more tests
 - Fix filter more flags to exclude in static dump, add more test cases
 - Merge branch 'master' into jdk-8259070
 - Fix white space in CDS.java
 - Add function CDS.dumpSharedArchive in java to dump shared archive
 - 8259070: Add jcmd option to dump CDS

-------------

Changes: https://git.openjdk.java.net/jdk/pull/2737/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2737&range=07
  Stats: 830 lines in 21 files changed: 758 ins; 58 del; 14 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2737.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2737/head:pull/2737

PR: https://git.openjdk.java.net/jdk/pull/2737
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8259070: Add jcmd option to dump CDS [v8]

Ioi Lam-2
On Wed, 31 Mar 2021 20:58:46 GMT, Yumin Qi <[hidden email]> wrote:

>> Hi, Please review
>>
>>   Added jcmd option for dumping CDS archive during application runtime. Before this change, user has to dump shared archive in two steps: first run application with
>>   `java -XX:DumpLoadedClassList=<classlist> .... `  
>>  to collect shareable class names and saved in file `<classlist>` , then
>>   `java -Xshare:dump -XX:SharedClassListFile=<classlist> -XX:SharedArchiveFile=<archivefile> ...`
>>   With this change, user can use jcmd to dump CDS without going through above steps. Also user can choose a moment during the app runtime  to dump an archive.
>>    The bug is associated with the CSR: https://bugs.openjdk.java.net/browse/JDK-8259798 which has been approved.
>>    New added jcmd option:
>>    `jcmd <pid or AppName> VM.cds static_dump <filename>`
>>    or
>>     `jcmd <pid or AppName> VM.cds dynamic_dump <filename>`
>>   To dump dynamic archive, requires start app with newly added flag `-XX:+RecordDynamicDumpInfo`, with this flag, some information related to dynamic dump like loader constraints will be recorded. Note the dumping process changed some object memory locations so for dumping dynamic archive, can only done once for a running app. For static dump, user can dump multiple times against same process.
>>    The file name is optional, if the file name is not supplied, the file name will take format of `java_pid<number>_static.jsa` or `java_pid<number>_dynamic.jsa` for static and dynamic respectively. The `<number>` is the application process ID.
>>
>>   Tests: tier1,tier2,tier3,tier4
>>
>> Thanks
>> Yumin
>
> Yumin Qi has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 10 commits:
>
>  - Fix revert unintentionally comment, merge master.
>  - Merge branch 'master' of ssh://github.com/yminqi/jdk into jdk-8259070
>  - Remove CDS.getVMArguments, changed to use VM.getRuntimeVMArguments. Removed unused function from ClassLoader. Improved InstanceKlass::is_shareable() and related test. Added more test scenarios.
>  - Remove redundant check for if a class is shareable
>  - Fix according to review comment and add more tests
>  - Fix filter more flags to exclude in static dump, add more test cases
>  - Merge branch 'master' into jdk-8259070
>  - Fix white space in CDS.java
>  - Add function CDS.dumpSharedArchive in java to dump shared archive
>  - 8259070: Add jcmd option to dump CDS

Changes requested by iklam (Reviewer).

src/hotspot/share/classfile/vmSymbols.hpp line 304:

> 302:   template(generateLambdaFormHolderClasses_signature, "([Ljava/lang/String;)[Ljava/lang/Object;") \
> 303:   template(dumpSharedArchive, "dumpSharedArchive")                                                \
> 304:   template(dumpSharedArchive_signature, "(ZLjava/lang/String;)V")                                 \

Need to align the "dumpSharedArchive" part with the previous line.

src/hotspot/share/prims/jvm.cpp line 3745:

> 3743: #if INCLUDE_CDS
> 3744:   assert(UseSharedSpaces && RecordDynamicDumpInfo, "already checked in arguments.cpp?");
> 3745:   if (DynamicArchive::has_been_dumped_once()) {

Maybe add a comment like this:?
// During dynamic archive dumping, some of the data structures are overwritten so
// we cannot dump the dynamic archive again. TODO: this should be fixed.

src/hotspot/share/prims/jvm.cpp line 3754:

> 3752:   assert(ArchiveClassesAtExit == nullptr, "already checked in arguments.cpp?");
> 3753:   Handle file_handle(THREAD, JNIHandles::resolve_non_null(archiveName));
> 3754:   char* archive_name  = java_lang_String::as_utf8_string(file_handle());

A ResourceMark is needed before calling java_lang_String::as_utf8_string().

In general, I think the code in jvm.cpp should only marshall the jobject argument (e.g., convert `jstring` to `char*`.). The main functionality of JVM_DumpDynamicArchive should be moved to dynamicArchive.cpp. Similarly, most of the work in JVM_DumpClassListToFile should be moved to metaspaceShared.cpp.

src/hotspot/share/prims/jvm.cpp line 3759:

> 3757:     DynamicArchive::dump();
> 3758:   } else {
> 3759:     THROW_MSG(vmSymbols::java_lang_RuntimeException(),

Need to set ArchiveClassesAtExit to NULL before throwing the exception, since dynamic dump may not work anymore after the failure.

test/hotspot/jtreg/runtime/cds/appcds/jcmd/LingeredTestApp.java line 28:

> 26: public class LingeredTestApp extends LingeredApp {
> 27:     // Do not use default test.class.path in class path.
> 28:     public boolean useDefaultClasspath() { return false; }

It's not obvious that you're changing the behavior of the base class by overriding a member function. It's better to have

public LingeredTestApp() {
   setUseDefaultClasspath(false);
}

Also, the name of LingeredTestApp is kind of generic. How about renaming it to JCmdTestLingeredApp?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2737
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8259070: Add jcmd option to dump CDS [v9]

Yumin Qi-3
In reply to this post by Yumin Qi-3
> Hi, Please review
>
>   Added jcmd option for dumping CDS archive during application runtime. Before this change, user has to dump shared archive in two steps: first run application with
>   `java -XX:DumpLoadedClassList=<classlist> .... `  
>  to collect shareable class names and saved in file `<classlist>` , then
>   `java -Xshare:dump -XX:SharedClassListFile=<classlist> -XX:SharedArchiveFile=<archivefile> ...`
>   With this change, user can use jcmd to dump CDS without going through above steps. Also user can choose a moment during the app runtime  to dump an archive.
>    The bug is associated with the CSR: https://bugs.openjdk.java.net/browse/JDK-8259798 which has been approved.
>    New added jcmd option:
>    `jcmd <pid or AppName> VM.cds static_dump <filename>`
>    or
>     `jcmd <pid or AppName> VM.cds dynamic_dump <filename>`
>   To dump dynamic archive, requires start app with newly added flag `-XX:+RecordDynamicDumpInfo`, with this flag, some information related to dynamic dump like loader constraints will be recorded. Note the dumping process changed some object memory locations so for dumping dynamic archive, can only done once for a running app. For static dump, user can dump multiple times against same process.
>    The file name is optional, if the file name is not supplied, the file name will take format of `java_pid<number>_static.jsa` or `java_pid<number>_dynamic.jsa` for static and dynamic respectively. The `<number>` is the application process ID.
>
>   Tests: tier1,tier2,tier3,tier4
>
> Thanks
> Yumin

Yumin Qi has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 13 commits:

 - Restructed code and rename LingeredTestApp.java to JCmdTestLingeredApp.java
 - Merge branch 'master' of ssh://github.com/yminqi/jdk into jdk-8259070
 - Merge branch 'master' of ssh://github.com/yminqi/jdk into jdk-8259070
 - Fix revert unintentionally comment, merge master.
 - Merge branch 'master' of ssh://github.com/yminqi/jdk into jdk-8259070
 - Remove CDS.getVMArguments, changed to use VM.getRuntimeVMArguments. Removed unused function from ClassLoader. Improved InstanceKlass::is_shareable() and related test. Added more test scenarios.
 - Remove redundant check for if a class is shareable
 - Fix according to review comment and add more tests
 - Fix filter more flags to exclude in static dump, add more test cases
 - Merge branch 'master' into jdk-8259070
 - ... and 3 more: https://git.openjdk.java.net/jdk/compare/328e9514...79822e79

-------------

Changes: https://git.openjdk.java.net/jdk/pull/2737/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2737&range=08
  Stats: 846 lines in 23 files changed: 772 ins; 58 del; 16 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2737.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2737/head:pull/2737

PR: https://git.openjdk.java.net/jdk/pull/2737
12