Integrated: 8273152: Refactor CDS FileMapHeader loading code

[Yumin Qi]

On Thu, 30 Sep 2021 05:43:40 GMT, Yumin Qi  wrote:

> Please review,
>   Refactor fundamental CDS FileMapHeader code for reliable reading of basic 
> info from shared archive.
>   With the change, it makes it possible to read an archive generated by 
> different version of hotspot. Also it is possible to automatically generate a 
> CDS archive If the archive supplied is not readable or fails to pass the 
> check.
> 
>   Tests: tier1-4
>jtreg on sa.
> 
> Thanks
> Yumin

This pull request has now been integrated.

Changeset: 8de77634
Author:Yumin Qi 
URL:   
https://git.openjdk.java.net/jdk/commit/8de77634c414cc348a6eb7b28fd6339befdb12d7
Stats: 566 lines in 11 files changed: 333 ins; 100 del; 133 mod

8273152: Refactor CDS FileMapHeader loading code

Reviewed-by: ccheung, iklam

-

PR: https://git.openjdk.java.net/jdk/pull/5768

Re: RFR: 8273152: Refactor CDS FileMapHeader loading code [v8]

[Yumin Qi]

On Thu, 7 Oct 2021 06:06:02 GMT, Ioi Lam  wrote:

>> Yumin Qi has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Add offset check for _generic_header in populate_header
>
> LGTM

@iklam @calvinccheung Thanks for review!

-

PR: https://git.openjdk.java.net/jdk/pull/5768

Re: RFR: 8273152: Refactor CDS FileMapHeader loading code [v8]

[Yumin Qi]

On Thu, 7 Oct 2021 20:30:00 GMT, Calvin Cheung  wrote:

> I just have one minor comment. It's up to you if you want to change it.

Thanks for review --- I will leave as it is now.

-

PR: https://git.openjdk.java.net/jdk/pull/5768

Re: RFR: 8273152: Refactor CDS FileMapHeader loading code [v8]

[Yumin Qi]

> Please review,
>   Refactor fundamental CDS FileMapHeader code for reliable reading of basic 
> info from shared archive.
>   With the change, it makes it possible to read an archive generated by 
> different version of hotspot. Also it is possible to automatically generate a 
> CDS archive If the archive supplied is not readable or fails to pass the 
> check.
> 
>   Tests: tier1-4
>jtreg on sa.
> 
> Thanks
> Yumin

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

  Add offset check for _generic_header in populate_header

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/5768/files
  - new: https://git.openjdk.java.net/jdk/pull/5768/files/81bd3a12..753adaa9

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5768&range=07
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5768&range=06-07

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

PR: https://git.openjdk.java.net/jdk/pull/5768

Re: RFR: 8273152: Refactor CDS FileMapHeader loading code [v5]

[Yumin Qi]

On Wed, 6 Oct 2021 17:01:58 GMT, Ioi Lam  wrote:

>> see above reply. We need read the full _header_size for _header. Also note 
>> that helper class will delete gen_header when out of scope.
>
> I see. I think your current code is fine.
> 
> Note that the current code writes the header as a FileMapHeader, but it reads 
> it as both a FileMapHeader and a CDSFileMapHeaderBase. So it has the basic 
> assumption `FileMapHeader::_generic_header` is at offset 0 of FileMapHeader. 
> Therefore, in FileHeaderHelper::initialize, we should add an assert:
> 
> 
> static_assert(offsetof(FileMapHeader, _generic_header) == 0, "must be");

This caused compiling error: since FileHeaderHelper is not a friend of 
FileMapHeader so no access to its fields:
error: 'GenericCDSFileMapHeader CDSFileMapHeaderBase::_generic_header' is 
inaccessible within this context

-

PR: https://git.openjdk.java.net/jdk/pull/5768

Re: RFR: 8273152: Refactor CDS FileMapHeader loading code [v7]

[Yumin Qi]

> Please review,
>   Refactor fundamental CDS FileMapHeader code for reliable reading of basic 
> info from shared archive.
>   With the change, it makes it possible to read an archive generated by 
> different version of hotspot. Also it is possible to automatically generate a 
> CDS archive If the archive supplied is not readable or fails to pass the 
> check.
> 
>   Tests: tier1-4
>jtreg on sa.
> 
> Thanks
> Yumin

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

  Change FileHeaderHelper::_header to value of non-ptr

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/5768/files
  - new: https://git.openjdk.java.net/jdk/pull/5768/files/b804dbc6..81bd3a12

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

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

PR: https://git.openjdk.java.net/jdk/pull/5768

Re: RFR: 8273152: Refactor CDS FileMapHeader loading code [v5]

[Yumin Qi]

On Wed, 6 Oct 2021 04:09:32 GMT, Ioi Lam  wrote:

>> Yumin Qi has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Added a helper class to facilitate checking archive
>
> src/hotspot/share/cds/filemap.cpp line 1099:
> 
>> 1097: lseek(_fd, _header->_base_archive_path_offset, SEEK_SET); // 
>> position to correct offset.
>> 1098: size_t n = os::read(_fd, *target, (unsigned int)name_size);
>> 1099: if (n != name_size) {
> 
> I think there's no need to do another read. The base name string is already 
> inside the buffer that was read on line 1079. You can just do a `strncpy` 
> into `*target`

FileHeaderHelper only read sizeof(GenericCDSFileMapHeader) which is the stable 
information for reading other information and checking the archive. It does not 
include name in the buffer.

-

PR: https://git.openjdk.java.net/jdk/pull/5768

Re: RFR: 8273152: Refactor CDS FileMapHeader loading code [v6]

[Yumin Qi]

> Please review,
>   Refactor fundamental CDS FileMapHeader code for reliable reading of basic 
> info from shared archive.
>   With the change, it makes it possible to read an archive generated by 
> different version of hotspot. Also it is possible to automatically generate a 
> CDS archive If the archive supplied is not readable or fails to pass the 
> check.
> 
>   Tests: tier1-4
>jtreg on sa.
> 
> Thanks
> Yumin

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

  Fixed comment for magic check, fixed _file_offset to _header_size

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/5768/files
  - new: https://git.openjdk.java.net/jdk/pull/5768/files/1a88076f..b804dbc6

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

  Stats: 8 lines in 1 file changed: 3 ins; 4 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5768.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5768/head:pull/5768

PR: https://git.openjdk.java.net/jdk/pull/5768

Re: RFR: 8273152: Refactor CDS FileMapHeader loading code [v5]

[Yumin Qi]

On Wed, 6 Oct 2021 04:15:06 GMT, Ioi Lam  wrote:

>> Yumin Qi has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Added a helper class to facilitate checking archive
>
> src/hotspot/share/cds/filemap.cpp line 1205:
> 
>> 1203:   }
>> 1204: 
>> 1205:   _header = (FileMapHeader*)os::malloc(gen_header->_header_size, 
>> mtInternal);
> 
> There's no need to allocate and read the header again. It's already in 
> gen_header. This should be enough:
> 
> 
> _header = (FileMapHeader*)gen_header;

see above reply. We need read the full _header_size for _header. Also note that 
helper class will delete gen_header when out of scope.

-

PR: https://git.openjdk.java.net/jdk/pull/5768

Re: RFR: 8273152: Refactor CDS FileMapHeader loading code [v5]

[Yumin Qi]

On Wed, 6 Oct 2021 04:17:16 GMT, Ioi Lam  wrote:

>> Yumin Qi has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Added a helper class to facilitate checking archive
>
> src/jdk.hotspot.agent/share/native/libsaproc/ps_core_common.c line 369:
> 
>> 367: 
>> 368:   // check file magic
>> 369:   if (header._generic_header._magic != CDS_ARCHIVE_MAGIC) {
> 
> Use `header.magic()` instead?

header is a structure of CDSFileMapHeaderBase, unless we cast it to 
FileMapHeader.

-

PR: https://git.openjdk.java.net/jdk/pull/5768

Re: RFR: 8273152: Refactor CDS FileMapHeader loading code [v5]

[Yumin Qi]

> Please review,
>   Refactor fundamental CDS FileMapHeader code for reliable reading of basic 
> info from shared archive.
>   With the change, it makes it possible to read an archive generated by 
> different version of hotspot. Also it is possible to automatically generate a 
> CDS archive If the archive supplied is not readable or fails to pass the 
> check.
> 
>   Tests: tier1-4
>jtreg on sa.
> 
> Thanks
> Yumin

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

  Added a helper class to facilitate checking archive

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/5768/files
  - new: https://git.openjdk.java.net/jdk/pull/5768/files/033c5763..1a88076f

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

  Stats: 267 lines in 4 files changed: 114 ins; 111 del; 42 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5768.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5768/head:pull/5768

PR: https://git.openjdk.java.net/jdk/pull/5768

Re: RFR: 8273152: Refactor CDS FileMapHeader loading code [v4]

[Yumin Qi]

> Please review,
>   Refactor fundamental CDS FileMapHeader code for reliable reading of basic 
> info from shared archive.
>   With the change, it makes it possible to read an archive generated by 
> different version of hotspot. Also it is possible to automatically generate a 
> CDS archive If the archive supplied is not readable or fails to pass the 
> check.
> 
>   Tests: tier1-4
>jtreg on sa.
> 
> Thanks
> Yumin

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

  Fixed comment

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/5768/files
  - new: https://git.openjdk.java.net/jdk/pull/5768/files/913aa64b..033c5763

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

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

PR: https://git.openjdk.java.net/jdk/pull/5768

Re: RFR: 8273152: Refactor CDS FileMapHeader loading code [v3]

[Yumin Qi]

> Please review,
>   Refactor fundamental CDS FileMapHeader code for reliable reading of basic 
> info from shared archive.
>   With the change, it makes it possible to read an archive generated by 
> different version of hotspot. Also it is possible to automatically generate a 
> CDS archive If the archive supplied is not readable or fails to pass the 
> check.
> 
>   Tests: tier1-4
>jtreg on sa.
> 
> Thanks
> Yumin

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

  Fixed output according to comment, fixed member accessor of CDSArchiveUtils

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/5768/files
  - new: https://git.openjdk.java.net/jdk/pull/5768/files/032e819c..913aa64b

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

  Stats: 54 lines in 4 files changed: 23 ins; 0 del; 31 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5768.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5768/head:pull/5768

PR: https://git.openjdk.java.net/jdk/pull/5768

Re: RFR: 8273152: Refactor CDS FileMapHeader loading code [v2]

[Yumin Qi]

> Please review,
>   Refactor fundamental CDS FileMapHeader code for reliable reading of basic 
> info from shared archive.
>   With the change, it makes it possible to read an archive generated by 
> different version of hotspot. Also it is possible to automatically generate a 
> CDS archive If the archive supplied is not readable or fails to pass the 
> check.
> 
>   Tests: tier1-4
>jtreg on sa.
> 
> Thanks
> Yumin

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

  Fixed miss return after file read failed

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/5768/files
  - new: https://git.openjdk.java.net/jdk/pull/5768/files/eb560883..032e819c

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

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

PR: https://git.openjdk.java.net/jdk/pull/5768

RFR: 8273152: Refactor CDS FileMapHeader loading code

[Yumin Qi]

Please review,
  Refactor fundamental CDS FileMapHeader code for reliable reading of basic 
info from shared archive.
  With the change, it makes it possible to read an archive generated by 
different version of hotspot. Also it is possible to automatically generate a 
CDS archive If the archive supplied is not readable or fails to pass the check.

  Tests: tier1-4
   jtreg on sa.

Thanks
Yumin

-

Commit messages:
 - Fix comparison between size_t and unsigned int
 - 8273152: Refactor CDS FileMapHeader loading code

Changes: https://git.openjdk.java.net/jdk/pull/5768/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5768&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8273152
  Stats: 423 lines in 11 files changed: 263 ins; 51 del; 109 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5768.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5768/head:pull/5768

PR: https://git.openjdk.java.net/jdk/pull/5768

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

[Yumin Qi]

On Sat, 27 Feb 2021 18:20:52 GMT, Ioi Lam  wrote:

>> Hi Yumin,
>> 
>> This is a very useful addition.
>> 
>> My biggest concern with this patch though is the use of 
>> `os::fork_and_exec()` in regular, non-fatal situations. I had a look at that 
>> function and I think that is very unsafe.
>> 
>> - it does not close file descriptors, so the child vm will inherit all file 
>> descriptors not opened with FD_CLOEXEC. In Runtime.exec, we do this whole 
>> dance around safely closing off all unused file descriptors, which is 
>> missing here. Note that even though we mostly open all fds with CLOEXEC, 
>> this does not matter, since user code may not do that.
>> - It uses vfork "if available", which is probably always, but that may be 
>> okay since the child exec's right away. Still, vfork makes me nervous.
>> - Weirdly enough, it always spawns the child program via one indirection 
>> using a shell; so there is always the shell between you and your spawned VM, 
>> and you probably wont get the return code of your child vm process back.
>>  
>> Until now, os::fork_and_exec() was only used in fatal situations where the 
>> VM was about to die anyway. And where it did not really matter if it worked 
>> or not. 
>> 
>> If we now want to use it in regular situations, we need to give that thing 
>> an overhaul and make it a first class fork api, and also test it better that 
>> it is today. The file descriptor issue has to be addressed at the very least.
>> 
>> 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().
>> 
>> Further remarks inline.
>> 
>> Thanks, Thomas
>
>> 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.

@iklam @calvinccheung @Hamlin-Li Thanks for review!

-

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

Integrated: 8259070: Add jcmd option to dump CDS

[Yumin Qi]

On Fri, 26 Feb 2021 00:03:40 GMT, Yumin Qi  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=  `  
>  to collect shareable class names and saved in file `` , then 
>   `java -Xshare:dump -XX:SharedClassListFile= 
> -XX:SharedArchiveFile= ...`
>   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  VM.cds static_dump `
>or
> `jcmd  VM.cds dynamic_dump `
>   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_static.jsa` or 
> `java_pid_dynamic.jsa` for static and dynamic respectively. The 
> `` is the application process ID.
> 
>   Tests: tier1,tier2,tier3,tier4
> 
> Thanks
> Yumin

This pull request has now been integrated.

Changeset: e7cbeba8
Author:Yumin Qi 
URL:   https://git.openjdk.java.net/jdk/commit/e7cbeba8
Stats: 857 lines in 23 files changed: 783 ins; 58 del; 16 mod

8259070: Add jcmd option to dump CDS

Reviewed-by: ccheung, iklam, mli

-

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

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

[Yumin Qi]

> 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=  `  
>  to collect shareable class names and saved in file `` , then 
>   `java -Xshare:dump -XX:SharedClassListFile= 
> -XX:SharedArchiveFile= ...`
>   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  VM.cds static_dump `
>or
> `jcmd  VM.cds dynamic_dump `
>   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_static.jsa` or 
> `java_pid_dynamic.jsa` for static and dynamic respectively. The 
> `` 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:

  Removed extra assert and fixed extra Path usage in test

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2737/files
  - new: https://git.openjdk.java.net/jdk/pull/2737/files/88c0f7d1..042ec8f3

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

  Stats: 5 lines in 2 files changed: 0 ins; 3 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

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

[Yumin Qi]

> 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=  `  
>  to collect shareable class names and saved in file `` , then 
>   `java -Xshare:dump -XX:SharedClassListFile= 
> -XX:SharedArchiveFile= ...`
>   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  VM.cds static_dump `
>or
> `jcmd  VM.cds dynamic_dump `
>   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_static.jsa` or 
> `java_pid_dynamic.jsa` for static and dynamic respectively. The 
> `` 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 16 commits:

 - Fix too much verbose output, make call to stopApp after test
 - Merge branch 'master' of ssh://github.com/yminqi/jdk into jdk-8259070
 - Merge branch 'master' of ssh://github.com/yminqi/jdk into jdk-8259070
 - 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
 - ... and 6 more: https://git.openjdk.java.net/jdk/compare/008fc75a...88c0f7d1

-

Changes: https://git.openjdk.java.net/jdk/pull/2737/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2737&range=10
  Stats: 860 lines in 23 files changed: 786 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

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

[Yumin Qi]

> 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=  `  
>  to collect shareable class names and saved in file `` , then 
>   `java -Xshare:dump -XX:SharedClassListFile= 
> -XX:SharedArchiveFile= ...`
>   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  VM.cds static_dump `
>or
> `jcmd  VM.cds dynamic_dump `
>   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_static.jsa` or 
> `java_pid_dynamic.jsa` for static and dynamic respectively. The 
> `` 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 14 commits:

 - Merge branch 'master' of ssh://github.com/yminqi/jdk into jdk-8259070
 - 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
 - ... and 4 more: https://git.openjdk.java.net/jdk/compare/a756d8d7...b3d20348

-

Changes: https://git.openjdk.java.net/jdk/pull/2737/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2737&range=09
  Stats: 835 lines in 23 files changed: 762 ins; 58 del; 15 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

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

[Yumin Qi]

> 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=  `  
>  to collect shareable class names and saved in file `` , then 
>   `java -Xshare:dump -XX:SharedClassListFile= 
> -XX:SharedArchiveFile= ...`
>   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  VM.cds static_dump `
>or
> `jcmd  VM.cds dynamic_dump `
>   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_static.jsa` or 
> `java_pid_dynamic.jsa` for static and dynamic respectively. The 
> `` 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

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

[Yumin Qi]

> 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=  `  
>  to collect shareable class names and saved in file `` , then 
>   `java -Xshare:dump -XX:SharedClassListFile= 
> -XX:SharedArchiveFile= ...`
>   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  VM.cds static_dump `
>or
> `jcmd  VM.cds dynamic_dump `
>   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_static.jsa` or 
> `java_pid_dynamic.jsa` for static and dynamic respectively. The 
> `` 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

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

[Yumin Qi]

On Fri, 19 Mar 2021 05:39:25 GMT, Ioi Lam  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

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

[Yumin Qi]

> 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=  `  
>  to collect shareable class names and saved in file `` , then 
>   `java -Xshare:dump -XX:SharedClassListFile= 
> -XX:SharedArchiveFile= ...`
>   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  VM.cds static_dump `
>or
> `jcmd  VM.cds dynamic_dump `
>   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_static.jsa` or 
> `java_pid_dynamic.jsa` for static and dynamic respectively. The 
> `` 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.

-

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

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

  Stats: 236 lines in 12 files changed: 89 ins; 69 del; 78 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

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

[Yumin Qi]

> 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=  `  
>  to collect shareable class names and saved in file `` , then 
>   `java -Xshare:dump -XX:SharedClassListFile= 
> -XX:SharedArchiveFile= ...`
>   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  VM.cds static_dump `
>or
> `jcmd  VM.cds dynamic_dump `
>   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_static.jsa` or 
> `java_pid_dynamic.jsa` for static and dynamic respectively. The 
> `` 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

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

[Yumin Qi]

> 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=  `  
>  to collect shareable class names and saved in file `` , then 
>   `java -Xshare:dump -XX:SharedClassListFile= 
> -XX:SharedArchiveFile= ...`
>   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  VM.cds static_dump `
>or
> `jcmd  VM.cds dynamic_dump `
>   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_static.jsa` or 
> `java_pid_dynamic.jsa` for static and dynamic respectively. The 
> `` 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

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

[Yumin Qi]

On Fri, 26 Feb 2021 22:46:07 GMT, Ioi Lam  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

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

[Yumin Qi]

On Fri, 26 Feb 2021 22:01:50 GMT, Calvin Cheung  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/memory/metaspaceShared.cpp line 788:
> 
>> 786:   // The existing file will be overwritten.
>> 787:   char filename[JVM_MAXPATHLEN];
>> 788:   const char* file = file_name;
> 
> Is the variable at line 788 necessary? Could you just pass filename to 
> callees?

New solutions using CDS.java to do the dump.

> src/hotspot/share/memory/metaspaceShared.cpp line 801:
> 
>> 799: file = filename;
>> 800:   }
>> 801: }
> 
> This block of code is very similar to lines 813 - 821 below. Maybe factor it 
> into another function?

changed to use java to dump. That will be much simple to deal with string.

> src/hotspot/share/memory/metaspaceShared.cpp line 831:
> 
>> 829:   DumpClassListCLDClosure(fileStream* f) : CLDClosure() { _stream = f; }
>> 830:   ~DumpClassListCLDClosure() {
>> 831: delete _stream; // The file need close since in child process it 
>> will be used.
> 
> Can you clarify the above comment?

Changed to use java to do the dump.

> test/hotspot/jtreg/runtime/cds/appcds/jcmd/JCmdTest.java line 213:
> 
>> 211: if (!cdsEnabled) {
>> 212: System.out.println("CDS is not available for this JDK, skip 
>> the test.");
>> 213: return;
> 
> Should throw SkippedException instead.

Fixed

-

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

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

[Yumin Qi]

On Wed, 10 Mar 2021 04:28:04 GMT, Ioi Lam  wrote:

>> Yumin Qi has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Fix white space in CDS.java
>
> src/java.base/share/classes/jdk/internal/misc/CDS.java line 256:
> 
>> 254: 
>> 255: // Do not take parent env which will cause dumping fail.
>> 256: Process proc = Runtime.getRuntime().exec(cmds.toArray(new 
>> String[0]),
> 
> Could you explain why the parent's env variables will cause dumping to fail?

I found jtreg env will be brought in to the children env which is not needed in 
this case. Add comment.

-

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

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

[Yumin Qi]

On Sat, 27 Feb 2021 18:11:38 GMT, Ioi Lam  wrote:

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

The stuff no in java code, and is simple to deal with.

-

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

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

[Yumin Qi]

On Sat, 27 Feb 2021 05:12:25 GMT, Thomas Stuefe  wrote:

>> src/hotspot/share/memory/metaspaceShared.cpp line 783:
>> 
>>> 781:   char* start = buffer + strlen(buffer);
>>> 782:   snprintf(start, buff_len, "%s ", arg);
>>> 783: }
>> 
>> Maybe move the above function to the StringUtils class under share/utilities?
>> Use `os::snprintf()` instead of `snprintf()`?
>
> The calculation is also wrong, this would overflow. You need:
>   char* start = buffer + strlen(buffer);
>   snprintf(start, buff_len - (start - buffer), "%s ", arg);
> - and maybe add an assert that strlen(buf) < bufflen. 
> - and as Ioi wrote, I'd use either one of os::snprintf or jio_snprintf since 
> both guarantee zero termination on truncation.
> - or, just use strncat()

The new solution is using CDS.java to do the work.

-

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

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

[Yumin Qi]

On Fri, 26 Feb 2021 21:39:48 GMT, Ioi Lam  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/memory/dynamicArchive.cpp line 347:
> 
>> 345:   if (Arguments::GetSharedDynamicArchivePath() == NULL) {
>> 346: if (!RecordDynamicDumpInfo) {
>> 347:   // If run with -XX:+RecordDynamicDumpInfo, 
>> DynamicDumpSharedSpaces will be turned on,
> 
> Is this check needed? It looks like `MetaspaceShared::cmd_dump_dynamic` will 
> not call `DynamicArchive::dump()` unless the path was set up correctly.

Fixed. The warning is harmless so I just revert it back.

> src/hotspot/share/memory/metaspaceShared.cpp line 789:
> 
>> 787:   char filename[JVM_MAXPATHLEN];
>> 788:   const char* file = file_name;
>> 789:   assert(strcmp(cmd, "static_dump") == 0 || strcmp(cmd, "dynamic_dump") 
>> == 0, "Sanity check");
> 
> Since the caller of this function already performed the string validity 
> check, I think it's better to pass `bool is_static` as a parameter and not 
> pass `cmd`.

Moved to CDS.java, code is simple than this.

> src/hotspot/share/memory/metaspaceShared.cpp line 863:
> 
>> 861: MutexLocker lock(ClassLoaderDataGraph_lock);
>> 862: DumpClassListCLDClosure collect_classes(stream);
>> 863: ClassLoaderDataGraph::loaded_cld_do(&collect_classes);
> 
> Need to close the stream.

Changed to use stack object so it will close the file at destrutor.

> src/hotspot/share/runtime/globals.hpp line 1896:
> 
>> 1894:
>>  \
>> 1895:   product(bool, RecordDynamicDumpInfo, false,  
>>  \
>> 1896:   "Record class info for jcmd Dynamic dump")   
>>  \
> 
> "Record class info for jcmd VM.cds dynamic_dump"?

Fixed.

-

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

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

[Yumin Qi]

On Wed, 10 Mar 2021 04:18:29 GMT, Ioi Lam  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

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

[Yumin Qi]

> 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=  `  
>  to collect shareable class names and saved in file `` , then 
>   `java -Xshare:dump -XX:SharedClassListFile= 
> -XX:SharedArchiveFile= ...`
>   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  VM.cds static_dump `
>or
> `jcmd  VM.cds dynamic_dump `
>   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_static.jsa` or 
> `java_pid_dynamic.jsa` for static and dynamic respectively. The 
> `` 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

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

[Yumin Qi]

> 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=  `  
>  to collect shareable class names and saved in file `` , then 
>   `java -Xshare:dump -XX:SharedClassListFile= 
> -XX:SharedArchiveFile= ...`
>   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  VM.cds static_dump `
>or
> `jcmd  VM.cds dynamic_dump `
>   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_static.jsa` or 
> `java_pid_dynamic.jsa` for static and dynamic respectively. The 
> `` 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

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

[Yumin Qi]

> 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=  `  
>  to collect shareable class names and saved in file `` , then 
>   `java -Xshare:dump -XX:SharedClassListFile= 
> -XX:SharedArchiveFile= ...`
>   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  VM.cds static_dump `
>or
> `jcmd  VM.cds dynamic_dump `
>   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_static.jsa` or 
> `java_pid_dynamic.jsa` for static and dynamic respectively. The 
> `` 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

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

[Yumin Qi]

On Fri, 26 Feb 2021 22:05:06 GMT, Calvin Cheung  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=  `  
>>  to collect shareable class names and saved in file `` , then 
>>   `java -Xshare:dump -XX:SharedClassListFile= 
>> -XX:SharedArchiveFile= ...`
>>   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  VM.cds static_dump `
>>or
>> `jcmd  VM.cds dynamic_dump `
>>   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_static.jsa` or 
>> `java_pid_dynamic.jsa` for static and dynamic respectively. The 
>> `` 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

Integrated: 8262157: LingeredApp.startAppExactJvmOpts does not print app output when launching fails

[Yumin Qi]

On Mon, 22 Feb 2021 22:26:50 GMT, Yumin Qi  wrote:

> Hi, Please review
> 
>  When debugging for other test case which uses jcmd to attach LingeredApp 
> process, found there is no error information logged when the app started with 
> function 'startAppExactJvmOpts' exits due to some reason. This is not 
> convenient for trace what is the app failure.
>  This simple fix for adding finishApp to print out error information when 
> LingeredApp could not start with startAppExactJvmOpts, this is similar to 
> startApp.
> 
> Tests: This is a simple fix and done tests with test/jdk/sun/tools/jinfo 
> which uses the function startAppExactJvmOpts to create LingeredApp, also the 
> test case in debugging, which indeed captured the error message upon start 
> error.
> 
> Thanks
> Yumin

This pull request has now been integrated.

Changeset: 3e13b66e
Author:Yumin Qi 
URL:   https://git.openjdk.java.net/jdk/commit/3e13b66e
Stats: 17 lines in 1 file changed: 8 ins; 7 del; 2 mod

8262157: LingeredApp.startAppExactJvmOpts does not print app output when 
launching fails

Reviewed-by: iklam, cjplummer

-

PR: https://git.openjdk.java.net/jdk/pull/2679

Re: RFR: 8262157: LingeredApp.startAppExactJvmOpts does not print app output when launching fails [v3]

[Yumin Qi]

> Hi, Please review
> 
>  When debugging for other test case which uses jcmd to attach LingeredApp 
> process, found there is no error information logged when the app started with 
> function 'startAppExactJvmOpts' exits due to some reason. This is not 
> convenient for trace what is the app failure.
>  This simple fix for adding finishApp to print out error information when 
> LingeredApp could not start with startAppExactJvmOpts, this is similar to 
> startApp.
> 
> Tests: This is a simple fix and done tests with test/jdk/sun/tools/jinfo 
> which uses the function startAppExactJvmOpts to create LingeredApp, also the 
> test case in debugging, which indeed captured the error message upon start 
> error.
> 
> Thanks
> Yumin

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

  Remove finishApp from startApp, move printout to startAppExactJvmOpts.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2679/files
  - new: https://git.openjdk.java.net/jdk/pull/2679/files/f8cbc91b..f0476031

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

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

PR: https://git.openjdk.java.net/jdk/pull/2679

Re: RFR: 8262157: LingeredApp.startAppExactJvmOpts does not print app output when launching fails [v2]

[Yumin Qi]

> Hi, Please review
> 
>  When debugging for other test case which uses jcmd to attach LingeredApp 
> process, found there is no error information logged when the app started with 
> function 'startAppExactJvmOpts' exits due to some reason. This is not 
> convenient for trace what is the app failure.
>  This simple fix for adding finishApp to print out error information when 
> LingeredApp could not start with startAppExactJvmOpts, this is similar to 
> startApp.
> 
> Tests: This is a simple fix and done tests with test/jdk/sun/tools/jinfo 
> which uses the function startAppExactJvmOpts to create LingeredApp, also the 
> test case in debugging, which indeed captured the error message upon start 
> error.
> 
> Thanks
> Yumin

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

  Add a flag to remember finishApp already called

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2679/files
  - new: https://git.openjdk.java.net/jdk/pull/2679/files/68a564f5..f8cbc91b

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

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

PR: https://git.openjdk.java.net/jdk/pull/2679

Re: RFR: 8262157: LingeredApp.startAppExactJvmOpts does not print app output when launching fails

[Yumin Qi]

On Tue, 23 Feb 2021 03:43:41 GMT, Chris Plummer  wrote:

>> It seems error prone to have to call finishApp() manually in order to see 
>> the error log. Since LingeredApp.startApp calls finishApp() on exceptions, 
>> shouldn't startAppExactJvmOpts do the same thing?
>
> Although you have a point, you've also pointed out another problem with this 
> fix.  I think users of `startApp()` are already going to see the output twice 
> because of the `finishApp()` call present there. By adding yet another 
> `finishApp()` call to `startAppExactJvmOpts()`, now they will see it 3 times.
> 
> If you want to "move" the `finishApp()` call from `startApp()` to 
> `startAppExactJvmOpts()`, then at least that will maintain the status quo 
> with existing `startApp()` users, but we still have an issue with the output 
> appearing twice, even before this change, and with this change it is now more 
> common as `startAppExactJvmOpts()` will also start seeing it.
> 
> Maybe `finishApp()` should maintain an `alreadyCalled` flag so it does 
> nothing on subsequent calls.

@plummercj @iklam Thanks for review. I will add a flag for finishApp so we will 
not call it more than once.

-

PR: https://git.openjdk.java.net/jdk/pull/2679

Re: CSR: 8259798: Add jcmd option to dump CDS

[Yumin Qi]

Hi, Thomas and all


  I have updated the CSR ( https://bugs.openjdk.java.net/browse/JDK-8259798). 
During the code/debug, I have found some information in the CSR needs revision. 
Please take a look and give your comment.

  I am working on newly added test case failures found by mach5 tests, will 
send codereview after the problems fixed.


Thanks

Yumin


On 1/14/21 12:03 PM, Yumin Qi wrote:

Hi, All

  I have created bug and CSR for this issue and wish have your comments on it:

  BUG: https://bugs.openjdk.java.net/browse/JDK-8259070

  CSR: https://bugs.openjdk.java.net/browse/JDK-8259798

   The added jcmd option gives users an option to dump a shared archive in one 
step at any time when the java application is running. Please check the CSR for 
more detail description for it.


    Thanks for your comment.


Yumin

Fwd: CSR: 8259798: Add jcmd option to dump CDS

[Yumin Qi]

Oops, push send too soon.


Thanks

Yumin


 Forwarded Message 
Subject:CSR: 8259798: Add jcmd option to dump CDS
Date:   Thu, 14 Jan 2021 12:03:38 -0800
From:   Yumin Qi 
To: hotspot-runtime-...@openjdk.java.net



Hi, All

  I have created bug and CSR for this issue and wish have your comments on it:

  BUG: https://bugs.openjdk.java.net/browse/JDK-8259070

  CSR: https://bugs.openjdk.java.net/browse/JDK-8259798

   The added jcmd option gives users an option to dump a shared archive in one 
step at any time when the java application is running. Please check the CSR for 
more detail description for it.


    Thanks for your comment.


Yumin

Re: RFR: 8257726: Make -XX:+StressLdcRewrite option a diagnostic option

[Yumin Qi]

On Tue, 15 Dec 2020 17:26:25 GMT, Coleen Phillimore  wrote:

> See bug for details.  Tested:
> 
> $ java -XX:+StressLdcRewrite -version
> Error: VM option 'StressLdcRewrite' is diagnostic and must be enabled via 
> -XX:+UnlockDiagnosticVMOptions.
> Error: The unlock option must precede 'StressLdcRewrite'.
> Error: Could not create the Java Virtual Machine.
> Error: A fatal exception has occurred. Program will exit.
> $ java -XX:+UnlockDiagnosticVMOptions -XX:+StressLdcRewrite -version
> openjdk version "16-internal" 2021-03-16
> OpenJDK Runtime Environment (build 16-internal+0-2020-12-15-1356558.coleen...)
> OpenJDK 64-Bit Server VM (build 16-internal+0-2020-12-15-1356558.coleen..., 
> mixed mode, sharing)
> 
> Also, java/lang/instrument which has a test using StressLdcRewrite and tier1.

jvmtiRedfineClasses.cpp use it.

-

PR: https://git.openjdk.java.net/jdk/pull/1783

Re: RFR(T) 8203005: The top-of-stack type specified for nofast_* bytecodes are wrong

[Yumin Qi]

Thanks Dan for the review!


On 6/25/20 12:17 PM, Daniel D. Daugherty wrote:

On 6/25/20 1:33 PM, Yumin Qi wrote:

Hi, please review the tiny changes for

bug: https://bugs.openjdk.java.net/browse/JDK-8203005

webrev:http://cr.openjdk.java.net/~minqi/2020/8203005/webrev-00/


src/hotspot/share/interpreter/bytecodes.cpp
    No comments.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/interpreter/Bytecodes.java 


    No comments.

Thumbs up. I agree that this is a trivial fix and you don't need to
wait 24 hours to push.

Dan







Summary: The change was left by 
8074345(https://bugs.openjdk.java.net/browse/JDK-8074345), the types 
were wrongly put as T_ILLEGAL for T_OBJECT, and T_ILLEGAL for T_INT. 
This has not triggered any failures yet since the types stored in the 
type array for nofast version are never used, the used types are 
always the original types fortunately(unfortunately either).



tests: tier1,tier2,tier3


Thanks

Yumin

Re: RFR(T) 8203005: The top-of-stack type specified for nofast_* bytecodes are wrong

[Yumin Qi]

Hi, Chris

  Thanks for the review!


Thanks

Yumin

On 6/25/20 11:37 AM, Chris Plummer wrote:
Nevermind. I should have looked at your full webrev. I see you already 
covered this.


thanks,

Chris

On 6/25/20 11:28 AM, Chris Plummer wrote:

Hi Yumin,

It looks like the fix for 
https://bugs.openjdk.java.net/browse/JDK-8174995 has this same bug. 
What do you think?


thanks,

Chris

On 6/25/20 10:33 AM, Yumin Qi wrote:

Hi, please review the tiny changes for

bug: https://bugs.openjdk.java.net/browse/JDK-8203005

webrev:http://cr.openjdk.java.net/~minqi/2020/8203005/webrev-00/


Summary: The change was left by 
8074345(https://bugs.openjdk.java.net/browse/JDK-8074345), the types 
were wrongly put as T_ILLEGAL for T_OBJECT, and T_ILLEGAL for T_INT. 
This has not triggered any failures yet since the types stored in 
the type array for nofast version are never used, the used types are 
always the original types fortunately(unfortunately either).



tests: tier1,tier2,tier3


Thanks

Yumin

RFR(T) 8203005: The top-of-stack type specified for nofast_* bytecodes are wrong

[Yumin Qi]

Hi, please review the tiny changes for

bug: https://bugs.openjdk.java.net/browse/JDK-8203005

webrev:http://cr.openjdk.java.net/~minqi/2020/8203005/webrev-00/


Summary: The change was left by 
8074345(https://bugs.openjdk.java.net/browse/JDK-8074345), the types 
were wrongly put as T_ILLEGAL for T_OBJECT, and T_ILLEGAL for T_INT. 
This has not triggered any failures yet since the types stored in the 
type array for nofast version are never used, the used types are always 
the original types fortunately(unfortunately either).



tests: tier1,tier2,tier3


Thanks

Yumin

Re: RFR(S) 8246019 PerfClassTraceTime slows down VM start-up

[Yumin Qi]

  product(bool, UsePerfData, 
true,  \
  "Flag to disable jvmstat instrumentation for performance 
testing "\
  "and problem isolation 
purposes") \



The flag default value set to true --- should we change that? If the 
flag set to false at default, performance can benefit from that. If 
users want to collect performance data, should explicitly turn it on.



Thanks

Yumin


On 6/16/20 8:19 PM, Ioi Lam wrote:



On 6/16/20 6:20 PM, David Holmes wrote:

Hi Ioi,

On 17/06/2020 6:14 am, Ioi Lam wrote:

https://bugs.openjdk.java.net/browse/JDK-8246019
http://cr.openjdk.java.net/~iklam/jdk16/8246019-avoid-PerfClassTraceTime.v01/ 



PerfClassTraceTime is (a rarely used feature) for measuring the time 
spent during class linking and initialization.


"A special command jcmd  PerfCounter.print 
prints all performance counters in the process."


How do you know this is a "rarely used feature"?

Hi David,

Sure, the counter will be dumped, but by "rarely used" -- I mean no 
one will find this particular counter useful, and no one will be 
actively looking at it.


I changed two parts of the code -- class init and class linking.

For class initialization, the counter may be useful for people who 
want to know how much time is spent in their  functions, and 
my patch doesn't change that. It only avoids using the counter when a 
class has no , i.e., we know that the counter counts nothing 
(except for a logging statement).


=

For class linking, no user code is executed, so it only measures VM 
code. If it's useful for anyone, that would be VM engineers like me 
who are trying to optimize the speed of class loading. However, due to 
the overhead of the counter vs what it's trying to measure, the 
results are pretty meaningless.


Note that I've not disabled the counter altogether. Instead, I disable 
it only when linking a CDS shared class, and we know that very little 
is happening for this class (e.g., no verification).


I think the class linking timer might have been useful 15 years ago 
when it was introduced, or it might be useful today when CDS is 
disabled. But with CDS enabled, we are paying a constant price that 
seems to benefit no one.


I think we should short-circuit it when it seems appropriate. If this 
indeed causes problems for our users, it's easy to re-enable it. 
That's better than just keeping this forever just because we're afraid 
to touch anything.




I find it hard to evaluate whether this short-circuiting of the time 
tracing is reasonable or not. Obviously any monitoring mechanism 
should impose minimal overhead compared to what is being measured, 
and these timers fall short in that regard. But if these stats become 
meaningless then they may as well be removed.


I think the serviceability folk (cc'd) need to evaluate this in the 
context of the M&M tools.


However, it's quite expensive and it needs to start and stop a bunch 
of timers. With CDS, it's quite often for the overhead of the timer 
itself to be much more than the time it's trying to measure, giving 
unreliable measurement.


In this patch, when it's clear that the init and linking will be 
very quick, I disable the timer and count only the number of 
invocations. This shows a small improvement in start-up


I'm curious if you tried to forcing EagerInitialization to be true to 
see how that improves the baseline. I've always noticed eager_init in 
the code, but hadn't realized it is disabled by default.




I think it cannot be done by default, as it will violate the JLS. A 
class can be initialized only when it's touched by bytecodes.


It can also backfire as we may load many classes without initializing 
them. E.g., during bytecode verification, we load many classes and 
just check that one is a supertype of another.


Thanks
- Ioi


Cheers,
David
-

Results of " perf stat -r 100 bin/java -Xshare:on 
-XX:SharedArchiveFile=jdk2.jsa -Xint -version "


59623970 59341935 (-282035)   -  41.774  41.591 ( -0.183) -
59623495 59331646 (-291849)   -  41.696  41.165 ( -0.531) --
59627148 59329526 (-297622)   -  41.249  41.094 ( -0.155) -
59612439 59340760 (-271679)      41.773  40.657 ( -1.116) -
59626438 59335681 (-290757)   -  41.683  40.901 ( -0.782) 
59618436 59338953 (-279483)   -  41.861  41.249 ( -0.612) ---
59608782 59340173 (-268609)      41.198  41.508 (  0.310) +
59614612 59325177 (-289435)   -  41.397  41.738 (  0.341) ++
59615905 59344006 (-271899)      41.921  40.969 ( -0.952) 
59635867 59333147 (-302720)   -  41.491  40.836 ( -0.655) ---

59620708 59336100 (-284608)   -  41.604  41.169 ( -0.434) --
instruction delta =  -284608    -0.4774%
time    delta =   -0.434 ms -1.0435%

The number of PerfClassTraceTime's used is reduced from 564 to 116 
(so we have an overhead of about 715 instructions per use, yikes!)

Re: RFR(T): 8247495: ProblemList vmTestbase/nsk/jvmti/SetFieldAccessWatch/setfldw001/TestDescription.java

[Yumin Qi]

Hi, Dan

  Looks good to me and it is trivial.


Thanks

Yumin

On 6/12/20 11:40 AM, Daniel D. Daugherty wrote:

Tap, tap, tap... is this thing working?

Anyone out there? This is a trivial 1-liner review...

Dan


On 6/12/20 12:46 PM, Daniel D. Daugherty wrote:

Greetings,

It's time to reduce the noise in the CI so I'm ProblemListing tests.

Here's the bug for failure:

    JDK-8205957 setfldw001/TestDescription.java fails with bad field 
value

    https://bugs.openjdk.java.net/browse/JDK-8205957

and here's the bug for the ProblemListing:

    JDK-8247495 ProblemList 
vmTestbase/nsk/jvmti/SetFieldAccessWatch/setfldw001/TestDescription.java

    https://bugs.openjdk.java.net/browse/JDK-8247495

I'm considering this a trivial change so I need a single (R)eviewer.

Here's the context diff for the change:

$ hg diff
diff -r 015533451f4c test/hotspot/jtreg/ProblemList.txt
--- a/test/hotspot/jtreg/ProblemList.txt    Fri Jun 12 09:31:08 2020 
-0700
+++ b/test/hotspot/jtreg/ProblemList.txt    Fri Jun 12 12:40:17 2020 
-0400

@@ -141,6 +141,7 @@
 vmTestbase/nsk/jvmti/scenarios/jni_interception/JI05/ji05t001/TestDescription.java 
8219652 aix-ppc64
 vmTestbase/nsk/jvmti/scenarios/jni_interception/JI06/ji06t001/TestDescription.java 
8219652 aix-ppc64
 vmTestbase/nsk/jvmti/SetJNIFunctionTable/setjniftab001/TestDescription.java 
8219652 aix-ppc64
+vmTestbase/nsk/jvmti/SetFieldAccessWatch/setfldw001/TestDescription.java 
8205957 generic-all


 vmTestbase/gc/lock/jni/jnilock002/TestDescription.java 
8208243,8192647 generic-all



This issue is actually much older than JDK-8205957 would indicate
(first sighting in JDK11 for that bug ID). The older version of
the test is covered by https://bugs.openjdk.java.net/browse/JDK-6528079
and that failures first sighting is in JDK7.


Thanks, in advance, for any comments, questions, or suggestions.

Dan

Re: (S) 8237750: Load libzip.so only if necessary

[Yumin Qi]

Ioi, Calvin

  Thanks. The inline sometime prefix to function declaration sometimes 
not. I will put inline without updating webrev.


Thanks
Yumin

On 5/5/20 2:23 PM, Ioi Lam wrote:

Hi Yumin,

258   static void load_zip_library_if_needed();

I think "inline" should be added to the declaration. It probably 
doesn't matter, but we usually add 'inline' to the declaration of 
functions that appear in xxx_inline.hpp files.


Thanks
- Ioi

On 5/5/20 1:32 PM, Yumin Qi wrote:

Hi, Ioi and Calvin

  There is an file classLoader.inline.hpp which also includes 
atomic.hpp. I put load_zip_library_if_needed in it. Updated webrev:

http://cr.openjdk.java.net/~minqi/8237750/webrev-03/
  Also, double checked the function is not in libjvm.so by
  nm libjvm.so | grep load_zip_library_if_needed

Thanks
Yumin

On 5/5/20 10:59 AM, Ioi Lam wrote:

Hi Yumin,

Looks good.

Just small nits. No need for new webrev.

[1] I think this should be named _libzip_loaded to be consistent 
with other fields


  static int  libzip_loaded;   // used to sync loading zip.


[2] This introduces dependency on atomic.hpp to classLoader.hpp

  static void load_zip_library_if_needed() {
    if (Atomic::load_acquire(&libzip_loaded) == 0) {
  release_load_zip_library();
    }
  }

Since this function is used only privately in the cpp file, I think 
it's better to change the hpp to this (to reduce unnecessary header 
file dependencies)


  inline static void load_zip_library_if_needed();

and then move the implementation to the classLoader.cpp file. You 
also need to add include "runtime/atomic.hpp" to classLoader.cpp.


Thanks
- Ioi





On 5/4/20 4:40 PM, Yumin Qi wrote:

Hi, Ioi

  Thanks. Updated webrev at: 
http://cr.openjdk.java.net/~minqi/8237750/webrev-02/
  libzip_loaded now is a class member of ClassLoader, no longer a 
file private integer since it is used in both .hpp and .cpp files.


Thanks
 Yumin

On 5/4/20 8:31 AM, Ioi Lam wrote:

Hi Yumin,

 977 void ClassLoader::load_zip_library() {
 978   if (Atomic::load_acquire(&libzip_loaded) == 0) {
 979 MutexLocker locker(Zip_lock, 
Monitor::_no_safepoint_check_flag);

 980 if (libzip_loaded == 0) {
 981   load_zip_library0();
 982   Atomic::release_store(&libzip_loaded, 1);
 983 }
 984   }
 985 }

1026 int ClassLoader::crc32(int crc, const char* buf, int len) {
1027   if (libzip_loaded == 0) {
1028 load_zip_library();
1029   }
1030   return (*Crc32)(crc, (const jbyte*)buf, len);
1031 }

ClassLoader::crc32 access libzip_loaded without a memory barrier, 
so it may read libzip_loaded out-of-order from Crc32. This means, 
thread 1 may be writing the memory in this order:


    Crc32 = ;
    libzip_loaded = 1;

but the order observed in thread 2 may be

    libzip_loaded = 1;
 >> ClassLoader::crc32 called here in thread 2
    Crc32 = ;

as a result, thread 2 may read libzip_loaded = 1, but still reads 
a NULL from Crc32.


To ensure the memory barrier is used, I think we should do this:

// private
inline void ClassLoader::load_zip_library_if_needed() {
  if (Atomic::load_acquire(&libzip_loaded) == 0) {
    release_load_zip_library();
  }
}

// private
void ClassLoader::release_load_zip_library() {
  MutexLocker locker(Zip_lock, Monitor::_no_safepoint_check_flag);
  if (libzip_loaded == 0) {
    load_zip_library0();
    Atomic::release_store(&libzip_loaded, 1);
  }
}

int ClassLoader::crc32(int crc, const char* buf, int len) {
  load_zip_library_if_needed();
  return (*Crc32)(crc, (const jbyte*)buf, len);
}

HotSpot code uses the "release_" prefix to indicate that the 
function must be used as a part of an acquire/release memory 
barrier. (E.g., InstanceKlass::release_set_array_klasses()).


Some backgrounds: 
https://preshing.com/20130922/acquire-and-release-fences/


Thanks
- Ioi



On 5/1/20 8:00 PM, Yumin Qi wrote:

Dean,

  I have updated to use MutexLocker instead at same link: 
http://cr.openjdk.java.net/~minqi/8237750/webrev-01/


  Tested locally, passed jtreg/runtime.

Thanks
Yumin

On 5/1/20 4:24 PM, Dean Long wrote:

OK, I didn't realize compute_fingerprint is using ZIP_CRC32.

dl

On 5/1/20 2:42 PM, Yumin Qi wrote:

Hi, Dean
  Thanks for the review. I will try MutextLocker, for AOT, I 
think currently the tests are not using CDS then it will load 
classes from stream, that will use libzip's Crc32.
  I will check for AOT to see if it really loads libzip with 
the patch. For test compiler/aot/DeoptimizationTest.java:


#0  ClassLoader::load_zip_library () at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classLoader.cpp:978 

#1  0x757d4693 in ClassLoader::crc32 (crc=0, 
buf=0x70244ee0 "\312\376\272\276", len=1888) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classLoader.cpp:1028 

#2  0x757cef5d in ClassFileStream::compute_fingerprint 
(this=0x70245640) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfil

Re: (S) 8237750: Load libzip.so only if necessary

[Yumin Qi]

Hi, Ioi and Calvin

  There is an file classLoader.inline.hpp which also includes 
atomic.hpp. I put load_zip_library_if_needed in it. Updated webrev:

http://cr.openjdk.java.net/~minqi/8237750/webrev-03/
  Also, double checked the function is not in libjvm.so by
  nm libjvm.so | grep load_zip_library_if_needed

Thanks
Yumin

On 5/5/20 10:59 AM, Ioi Lam wrote:

Hi Yumin,

Looks good.

Just small nits. No need for new webrev.

[1] I think this should be named _libzip_loaded to be consistent with 
other fields


  static int  libzip_loaded;   // used to sync loading zip.


[2] This introduces dependency on atomic.hpp to classLoader.hpp

  static void load_zip_library_if_needed() {
    if (Atomic::load_acquire(&libzip_loaded) == 0) {
  release_load_zip_library();
    }
  }

Since this function is used only privately in the cpp file, I think 
it's better to change the hpp to this (to reduce unnecessary header 
file dependencies)


  inline static void load_zip_library_if_needed();

and then move the implementation to the classLoader.cpp file. You also 
need to add include "runtime/atomic.hpp" to classLoader.cpp.


Thanks
- Ioi





On 5/4/20 4:40 PM, Yumin Qi wrote:

Hi, Ioi

  Thanks. Updated webrev at: 
http://cr.openjdk.java.net/~minqi/8237750/webrev-02/
  libzip_loaded now is a class member of ClassLoader, no longer a 
file private integer since it is used in both .hpp and .cpp files.


Thanks
 Yumin

On 5/4/20 8:31 AM, Ioi Lam wrote:

Hi Yumin,

 977 void ClassLoader::load_zip_library() {
 978   if (Atomic::load_acquire(&libzip_loaded) == 0) {
 979 MutexLocker locker(Zip_lock, 
Monitor::_no_safepoint_check_flag);

 980 if (libzip_loaded == 0) {
 981   load_zip_library0();
 982   Atomic::release_store(&libzip_loaded, 1);
 983 }
 984   }
 985 }

1026 int ClassLoader::crc32(int crc, const char* buf, int len) {
1027   if (libzip_loaded == 0) {
1028 load_zip_library();
1029   }
1030   return (*Crc32)(crc, (const jbyte*)buf, len);
1031 }

ClassLoader::crc32 access libzip_loaded without a memory barrier, so 
it may read libzip_loaded out-of-order from Crc32. This means, 
thread 1 may be writing the memory in this order:


    Crc32 = ;
    libzip_loaded = 1;

but the order observed in thread 2 may be

    libzip_loaded = 1;
 >> ClassLoader::crc32 called here in thread 2
    Crc32 = ;

as a result, thread 2 may read libzip_loaded = 1, but still reads a 
NULL from Crc32.


To ensure the memory barrier is used, I think we should do this:

// private
inline void ClassLoader::load_zip_library_if_needed() {
  if (Atomic::load_acquire(&libzip_loaded) == 0) {
    release_load_zip_library();
  }
}

// private
void ClassLoader::release_load_zip_library() {
  MutexLocker locker(Zip_lock, Monitor::_no_safepoint_check_flag);
  if (libzip_loaded == 0) {
    load_zip_library0();
    Atomic::release_store(&libzip_loaded, 1);
  }
}

int ClassLoader::crc32(int crc, const char* buf, int len) {
  load_zip_library_if_needed();
  return (*Crc32)(crc, (const jbyte*)buf, len);
}

HotSpot code uses the "release_" prefix to indicate that the 
function must be used as a part of an acquire/release memory 
barrier. (E.g., InstanceKlass::release_set_array_klasses()).


Some backgrounds: 
https://preshing.com/20130922/acquire-and-release-fences/


Thanks
- Ioi



On 5/1/20 8:00 PM, Yumin Qi wrote:

Dean,

  I have updated to use MutexLocker instead at same link: 
http://cr.openjdk.java.net/~minqi/8237750/webrev-01/


  Tested locally, passed jtreg/runtime.

Thanks
Yumin

On 5/1/20 4:24 PM, Dean Long wrote:

OK, I didn't realize compute_fingerprint is using ZIP_CRC32.

dl

On 5/1/20 2:42 PM, Yumin Qi wrote:

Hi, Dean
  Thanks for the review. I will try MutextLocker, for AOT, I 
think currently the tests are not using CDS then it will load 
classes from stream, that will use libzip's Crc32.
  I will check for AOT to see if it really loads libzip with the 
patch. For test compiler/aot/DeoptimizationTest.java:


#0  ClassLoader::load_zip_library () at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classLoader.cpp:978 

#1  0x757d4693 in ClassLoader::crc32 (crc=0, 
buf=0x70244ee0 "\312\376\272\276", len=1888) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classLoader.cpp:1028
#2  0x757cef5d in ClassFileStream::compute_fingerprint 
(this=0x70245640) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classFileStream.cpp:80
#3  0x757c40ed in ClassFileParser::create_instance_klass 
(this=0x77fc6fc0, changed_by_loadhook=false, 
cl_inst_info=..., __the_thread__=0x70033000) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classFileParser.cpp:5630
#4  0x75dea647 in KlassFactory::create_from_stream 
(stream=0x70245640, name=0x740550f0, 
loader_data=0x7022dbc0, cl_info=..., 
__the_thread__=0x70033000)
    at 
/home/minqi/ws/jdk15/jdk/src/hotspot/sha

Re: (S) 8237750: Load libzip.so only if necessary

[Yumin Qi]

Hi, Ioi

  Thanks. Updated webrev at: 
http://cr.openjdk.java.net/~minqi/8237750/webrev-02/
  libzip_loaded now is a class member of ClassLoader, no longer a file 
private integer since it is used in both .hpp and .cpp files.


Thanks
 Yumin

On 5/4/20 8:31 AM, Ioi Lam wrote:

Hi Yumin,

 977 void ClassLoader::load_zip_library() {
 978   if (Atomic::load_acquire(&libzip_loaded) == 0) {
 979 MutexLocker locker(Zip_lock, Monitor::_no_safepoint_check_flag);
 980 if (libzip_loaded == 0) {
 981   load_zip_library0();
 982   Atomic::release_store(&libzip_loaded, 1);
 983 }
 984   }
 985 }

1026 int ClassLoader::crc32(int crc, const char* buf, int len) {
1027   if (libzip_loaded == 0) {
1028 load_zip_library();
1029   }
1030   return (*Crc32)(crc, (const jbyte*)buf, len);
1031 }

ClassLoader::crc32 access libzip_loaded without a memory barrier, so 
it may read libzip_loaded out-of-order from Crc32. This means, thread 
1 may be writing the memory in this order:


    Crc32 = ;
    libzip_loaded = 1;

but the order observed in thread 2 may be

    libzip_loaded = 1;
 >> ClassLoader::crc32 called here in thread 2
    Crc32 = ;

as a result, thread 2 may read libzip_loaded = 1, but still reads a 
NULL from Crc32.


To ensure the memory barrier is used, I think we should do this:

// private
inline void ClassLoader::load_zip_library_if_needed() {
  if (Atomic::load_acquire(&libzip_loaded) == 0) {
    release_load_zip_library();
  }
}

// private
void ClassLoader::release_load_zip_library() {
  MutexLocker locker(Zip_lock, Monitor::_no_safepoint_check_flag);
  if (libzip_loaded == 0) {
    load_zip_library0();
    Atomic::release_store(&libzip_loaded, 1);
  }
}

int ClassLoader::crc32(int crc, const char* buf, int len) {
  load_zip_library_if_needed();
  return (*Crc32)(crc, (const jbyte*)buf, len);
}

HotSpot code uses the "release_" prefix to indicate that the function 
must be used as a part of an acquire/release memory barrier. (E.g., 
InstanceKlass::release_set_array_klasses()).


Some backgrounds: 
https://preshing.com/20130922/acquire-and-release-fences/


Thanks
- Ioi



On 5/1/20 8:00 PM, Yumin Qi wrote:

Dean,

  I have updated to use MutexLocker instead at same link: 
http://cr.openjdk.java.net/~minqi/8237750/webrev-01/


  Tested locally, passed jtreg/runtime.

Thanks
Yumin

On 5/1/20 4:24 PM, Dean Long wrote:

OK, I didn't realize compute_fingerprint is using ZIP_CRC32.

dl

On 5/1/20 2:42 PM, Yumin Qi wrote:

Hi, Dean
  Thanks for the review. I will try MutextLocker, for AOT, I think 
currently the tests are not using CDS then it will load classes 
from stream, that will use libzip's Crc32.
  I will check for AOT to see if it really loads libzip with the 
patch. For test compiler/aot/DeoptimizationTest.java:


#0  ClassLoader::load_zip_library () at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classLoader.cpp:978 

#1  0x757d4693 in ClassLoader::crc32 (crc=0, 
buf=0x70244ee0 "\312\376\272\276", len=1888) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classLoader.cpp:1028
#2  0x757cef5d in ClassFileStream::compute_fingerprint 
(this=0x70245640) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classFileStream.cpp:80
#3  0x757c40ed in ClassFileParser::create_instance_klass 
(this=0x77fc6fc0, changed_by_loadhook=false, cl_inst_info=..., 
__the_thread__=0x70033000) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classFileParser.cpp:5630
#4  0x75dea647 in KlassFactory::create_from_stream 
(stream=0x70245640, name=0x740550f0, 
loader_data=0x7022dbc0, cl_info=..., 
__the_thread__=0x70033000)
    at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/klassFactory.cpp:207
#5  0x757d53e4 in ClassLoader::load_class 
(name=0x740550f0, search_append_only=false, 
__the_thread__=0x70033000) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classLoader.cpp:1285
#6  0x76234fcf in SystemDictionary::load_instance_class 
(class_name=0x740550f0, class_loader=..., 
__the_thread__=0x70033000) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:1550
#7  0x76232d0a in 
SystemDictionary::resolve_instance_class_or_null 
(name=0x740550f0, class_loader=..., protection_domain=..., 
__the_thread__=0x70033000)
    at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:882
#8  0x7623137e in 
SystemDictionary::resolve_instance_class_or_null_helper 
(class_name=0x740550f0, class_loader=..., 
protection_domain=..., __the_thread__=0x70033000)
    at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:302
#9  0x7623124c in SystemDictionary::resolve_or_null 
(class_name=0x740550f0, class_loader=..., 
protection_domain=..., __the_thread__=0x70033000) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfil

Re: (S) 8237750: Load libzip.so only if necessary

[Yumin Qi]

Dean,

  I have updated to use MutexLocker instead at same link: 
http://cr.openjdk.java.net/~minqi/8237750/webrev-01/


  Tested locally, passed jtreg/runtime.

Thanks
Yumin

On 5/1/20 4:24 PM, Dean Long wrote:

OK, I didn't realize compute_fingerprint is using ZIP_CRC32.

dl

On 5/1/20 2:42 PM, Yumin Qi wrote:

Hi, Dean
  Thanks for the review. I will try MutextLocker, for AOT, I think 
currently the tests are not using CDS then it will load classes from 
stream, that will use libzip's Crc32.
  I will check for AOT to see if it really loads libzip with the 
patch. For test compiler/aot/DeoptimizationTest.java:


#0  ClassLoader::load_zip_library () at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classLoader.cpp:978
#1  0x757d4693 in ClassLoader::crc32 (crc=0, 
buf=0x70244ee0 "\312\376\272\276", len=1888) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classLoader.cpp:1028
#2  0x757cef5d in ClassFileStream::compute_fingerprint 
(this=0x70245640) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classFileStream.cpp:80
#3  0x757c40ed in ClassFileParser::create_instance_klass 
(this=0x77fc6fc0, changed_by_loadhook=false, cl_inst_info=..., 
__the_thread__=0x70033000) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classFileParser.cpp:5630
#4  0x75dea647 in KlassFactory::create_from_stream 
(stream=0x70245640, name=0x740550f0, 
loader_data=0x7022dbc0, cl_info=..., __the_thread__=0x70033000)
    at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/klassFactory.cpp:207
#5  0x757d53e4 in ClassLoader::load_class 
(name=0x740550f0, search_append_only=false, 
__the_thread__=0x70033000) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/classLoader.cpp:1285
#6  0x76234fcf in SystemDictionary::load_instance_class 
(class_name=0x740550f0, class_loader=..., 
__the_thread__=0x70033000) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:1550
#7  0x76232d0a in 
SystemDictionary::resolve_instance_class_or_null 
(name=0x740550f0, class_loader=..., protection_domain=..., 
__the_thread__=0x70033000)
    at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:882
#8  0x7623137e in 
SystemDictionary::resolve_instance_class_or_null_helper 
(class_name=0x740550f0, class_loader=..., protection_domain=..., 
__the_thread__=0x70033000)
    at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:302
#9  0x7623124c in SystemDictionary::resolve_or_null 
(class_name=0x740550f0, class_loader=..., protection_domain=..., 
__the_thread__=0x70033000) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:285
#10 0x76230f57 in SystemDictionary::resolve_or_fail 
(class_name=0x740550f0, class_loader=..., protection_domain=..., 
throw_error=true, __the_thread__=0x70033000)
    at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:233
#11 0x762311eb in SystemDictionary::resolve_or_fail 
(class_name=0x740550f0, throw_error=true, 
__the_thread__=0x70033000) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:275
#12 0x76236722 in SystemDictionary::resolve_wk_klass 
(id=SystemDictionary::Object_klass_knum, 
__the_thread__=0x70033000) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:2029
#13 0x7623681e in SystemDictionary::resolve_wk_klasses_until 
(limit_id=SystemDictionary::Cloneable_klass_knum, 
start_id=@0x77fc79d4: SystemDictionary::Object_klass_knum, 
__the_thread__=0x70033000)
    at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:2039
#14 0x7623ac01 in 
SystemDictionary::resolve_wk_klasses_through 
(end_id=SystemDictionary::Class_klass_knum, start_id=@0x77fc79d4: 
SystemDictionary::Object_klass_knum, __the_thread__=0x70033000)
    at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.hpp:418
#15 0x762369b0 in 
SystemDictionary::resolve_well_known_classes 
(__the_thread__=0x70033000) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:2082
#16 0x76236517 in SystemDictionary::initialize 
(__the_thread__=0x70033000) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/classfile/systemDictionary.cpp:1985
#17 0x762afe15 in Universe::genesis 
(__the_thread__=0x70033000) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/memory/universe.cpp:326
#18 0x762b1e51 in universe2_init () at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/memory/universe.cpp:847
#19 0x75af21ed in init_globals () at 
/home/minqi/ws/jdk15/jdk/src/hotspot/share/runtime/init.cpp:128
#20 0x7627feff in Threads::create_vm (args=0x77fc7e50, 
canTryAgain=0x77fc7d5b) at 
/home/minqi/ws/jdk15/jdk/src/hotspot/shar

Re: (S) 8237750: Load libzip.so only if necessary

[Yumin Qi]

qi/ws/jdk15/jdk/src/hotspot/share/prims/jni.cpp:3872
#23 0x77bca4c9 in InitializeJVM (pvm=0x77fc7ea8, 
penv=0x77fc7eb0, ifn=0x77fc7f00) at 
/home/minqi/ws/jdk15/jdk/src/java.base/share/native/libjli/java.c:1538
#24 0x77bc70b5 in JavaMain (_args=0x7fffab10) at 
/home/minqi/ws/jdk15/jdk/src/java.base/share/native/libjli/java.c:417
#25 0x77bceb5f in ThreadJavaMain (args=0x7fffab10) at 
/home/minqi/ws/jdk15/jdk/src/java.base/unix/native/libjli/java_md_solinux.c:734
#26 0x771c56ba in start_thread (arg=0x77fc8700) at 
pthread_create.c:333
#27 0x7790041d in clone () at 
../sysdeps/unix/sysv/linux/x86_64/clone.S:109


This is not with CDS.

Thanks
Yumin

On 5/1/20 2:11 PM, Dean Long wrote:

It looks like Zip_lock could be a MutexLocker instead of a MonitorLocker.

dl

On 5/1/20 10:24 AM, Yumin Qi wrote:

Hi, please review:
bug 8237750: https://bugs.openjdk.java.net/browse/JDK-8237750
webrev: http://cr.openjdk.java.net/~minqi/8237750/webrev-01/
Summary:
  zip library is loaded unconditionally at start up but it is only 
needed when

  1) bootclasspath contains zip or
  2) UseAOT enabled or
  3) VerifySharedArchive turned on or
  4) CDS archives custom loaded classes
   If none of above in java application, it is not necessary to have 
zip library loaded.


  Solution by loading zip library on demand when needed.

Performance for java -Xint version:

Results of " perf stat -r 50 bin/java -Xshare:on 
-XX:SharedArchiveFile=jdk2.jsa -Xint -version "
   1: 59611556    59450206 (-161350)  - 39.799 40.274 
(  0.475)    ++
   2: 59602708    59425234 (-177474)  - 40.591 41.183 
(  0.592)    ++
   3: 59579718    59441272 (-138446)    40.777 40.471 
( -0.306)  -
   4: 59584882    59410155 (-174727)  - 40.824 40.233 
( -0.591)  --
   5: 59590998    59447252 (-143746)    40.400 40.493 
(  0.093)
   6: 59589523    59441934 (-147589)    40.475 40.064 
( -0.411)  --
   7: 59581820    59413612 (-168208)  - 39.763 40.077 
(  0.314) +
   8: 59593678    59418738 (-174940)  - 40.912 39.724 
( -1.188)  -
   9: 59573058    59412554 (-160504)  - 40.126 40.033 
( -0.093)
  10: 59591469    59419291 (-172178)  - 40.731 40.689 
( -0.042)


  59589940    59428022 (-161917)  - 40.438 40.322 
( -0.116)

instr delta =  -161917    -0.2717%
time  delta =   -0.116 ms -0.2859%

Tests: hs-tier1-4.
Due to zip library not loaded at default, I removed 'zip' from pmap 
list in test case: *test/hotspot/jtreg/serviceability/sa/ClhsdbPmap.java


**
* Thanks
Yumin

(S) 8237750: Load libzip.so only if necessary

[Yumin Qi]

Hi, please review:
bug 8237750: https://bugs.openjdk.java.net/browse/JDK-8237750
webrev: http://cr.openjdk.java.net/~minqi/8237750/webrev-01/
Summary:
  zip library is loaded unconditionally at start up but it is only 
needed when

  1) bootclasspath contains zip or
  2) UseAOT enabled or
  3) VerifySharedArchive turned on or
  4) CDS archives custom loaded classes
   If none of above in java application, it is not necessary to have 
zip library loaded.


  Solution by loading zip library on demand when needed.

Performance for java -Xint version:

Results of " perf stat -r 50 bin/java -Xshare:on 
-XX:SharedArchiveFile=jdk2.jsa -Xint -version "
   1: 59611556    59450206 (-161350)  - 39.799 40.274 
(  0.475)    ++
   2: 59602708    59425234 (-177474)  - 40.591 41.183 
(  0.592)    ++
   3: 59579718    59441272 (-138446)    40.777 40.471 ( 
-0.306)  -
   4: 59584882    59410155 (-174727)  - 40.824 40.233 ( 
-0.591)  --
   5: 59590998    59447252 (-143746)    40.400 40.493 
(  0.093)
   6: 59589523    59441934 (-147589)    40.475 40.064 ( 
-0.411)  --
   7: 59581820    59413612 (-168208)  - 39.763 40.077 
(  0.314) +
   8: 59593678    59418738 (-174940)  - 40.912 39.724 ( 
-1.188)  -
   9: 59573058    59412554 (-160504)  - 40.126 40.033 ( 
-0.093)
  10: 59591469    59419291 (-172178)  - 40.731 40.689 ( 
-0.042)


  59589940    59428022 (-161917)  - 40.438 40.322 ( 
-0.116)

instr delta =  -161917    -0.2717%
time  delta =   -0.116 ms -0.2859%

Tests: hs-tier1-4.
Due to zip library not loaded at default, I removed 'zip' from pmap list 
in test case: *test/hotspot/jtreg/serviceability/sa/ClhsdbPmap.java


**
* Thanks
Yumin

Re: RFR (XS) 8215495: Always set isCopy

[yumin qi]

Looks good to me.

Thanks
Yumin

On Wed, Dec 19, 2018 at 7:42 AM JC Beyler  wrote:

> Hi all,
>
> Could I get a second review for this please? :)
>
> Webrev: http://cr.openjdk.java.net/~jcbeyler/8215495/webrev.01/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8215495
>
> Thanks!
> Jc
>
> On Mon, Dec 17, 2018 at 4:25 PM David Holmes 
> wrote:
>
> > LGTM.
> >
> > Thanks,
> > David
> >
> > On 18/12/2018 8:53 am, JC Beyler wrote:
> > > Sounds good to me:
> > >
> > > For the odd corner case:
> > >
> > > Webrev: http://cr.openjdk.java.net/~jcbeyler/8215495/webrev.01/
> > > 
> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8215495
> > >
> > > Thanks!
> > > Jc
> > >
> > > On Mon, Dec 17, 2018 at 2:39 PM David Holmes  > > > wrote:
> > >
> > > Hi Jc,
> > >
> > > On 18/12/2018 8:12 am, JC Beyler wrote:
> > >  > Hi David,
> > >  >
> > >  > I understand the rationale behind the "If the method does return
> > > NULL,
> > >  > then isCopy's value is undefined". But what about
> > >  > the DEFINE_GETSCALARARRAYELEMENTS case?
> > >  >
> > >  > It does this:
> > >  >if (len == 0) { \
> > >  >  /* Empty array: legal but useless, can't return NULL. \
> > >  >   * Return a pointer to something useless. \
> > >  >   * Avoid asserts in typeArrayOop. */ \
> > >  >  result = (ElementType*)get_bad_address(); \
> > >  >
> > >  > Should we at least then put isCopy to JNI_FALSE in that case
> > > since it
> > >  > does not return NULL and no exception is raised,
> > >
> > > Sure - it's an odd corner case, but better not to leave isCopy
> > > potentially uninitialized.
> > >
> > > Thanks,
> > > David
> > >
> > >  > Thanks,
> > >  > Jc
> > >  >
> > >  >
> > >  > On Mon, Dec 17, 2018 at 1:29 PM David Holmes
> > > mailto:david.hol...@oracle.com>
> > >  >  > > >> wrote:
> > >  >
> > >  > Hi Jc,
> > >  >
> > >  > On 18/12/2018 3:42 am, JC Beyler wrote:
> > >  >  > Hi all,
> > >  >  >
> > >  >  > Could I get a review for this webrev:
> > >  >  >
> > >  >  > Webrev:
> > > http://cr.openjdk.java.net/~jcbeyler/8215495/webrev.00/
> > >  >  > Bug: https://bugs.openjdk.java.net/browse/JDK-8215495
> > >  >
> > >  > isCopy only has to be set if the method executes
> successfully
> > >  >
> > >  > "If isCopy is not NULL, then *isCopy is set to JNI_TRUE if a
> > > copy is
> > >  > made; or it is set to JNI_FALSE if no copy is made."
> > >  >
> > >  > You can only make (or not) a copy if the operation actually
> > >  > succeeds. So
> > >  > before checking isCopy the caller must check for NULL and/or
> > > a pending
> > >  > exception.
> > >  >
> > >  > I see no bug here.
> > >  >
> > >  > David
> > >  > -
> > >  >
> > >  >  > Thanks,
> > >  >  > Jc
> > >  >
> > >  >
> > >  >
> > >  > --
> > >  >
> > >  > Thanks,
> > >  > Jc
> > >
> > >
> > >
> > > --
> > >
> > > Thanks,
> > > Jc
> >
>
>
> --
>
> Thanks,
> Jc
>

Re: RFR: JDK-8175312: SA: clhsdb: Provide an improved heap summary for 'universe' for G1GC

[yumin qi]

Jini,

  Looks good. One minor comment:

+   public void printG1HeapSummary(G1CollectedHeap heap) {+
G1CollectedHeap g1h = (G1CollectedHeap) heap;


 'heap' has been cast to 'G1CollectedHeap' at call site, so seems no need
to convert here again.

Thanks
Yumin

On Mon, Mar 12, 2018 at 8:52 AM, Jini George  wrote:

> Thank you very much, Stefan. Could one more reviewer please take a look at
> it ?
>
> - Jini.
>
>
> On 3/12/2018 8:52 PM, Stefan Johansson wrote:
>
>> Hi Jini,
>>
>> This looks good. I'm totally fine with skipping metaspace if that isn't
>> displayed for the other GCs.
>>
>> Cheers,
>> Stefan
>>
>> On 2018-03-09 10:29, Jini George wrote:
>>
>>> Here is the revised webrev:
>>>
>>> http://cr.openjdk.java.net/~jgeorge/8175312/webrev.02/
>>>
>>> I have made modifications to have the 'universe' command display details
>>> like:
>>>
>>> hsdb> universe
>>> Heap Parameters:
>>> garbage-first heap [0x00072520, 0x0007c000] region size
>>> 1024K
>>> G1 Heap:
>>>regions  = 2478
>>>capacity = 2598371328 (2478.0MB)
>>>used = 5242880 (5.0MB)
>>>free = 2593128448 (2473.0MB)
>>>0.20177562550443906% used
>>> G1 Young Generation:
>>> Eden Space:
>>>regions  = 5
>>>capacity = 8388608 (8.0MB)
>>>used = 5242880 (5.0MB)
>>>free = 3145728 (3.0MB)
>>>62.5% used
>>> Survivor Space:
>>>regions  = 0
>>>capacity = 0 (0.0MB)
>>>used = 0 (0.0MB)
>>>free = 0 (0.0MB)
>>>0.0% used
>>> G1 Old Generation:
>>>regions  = 0
>>>capacity = 155189248 (148.0MB)
>>>used = 0 (0.0MB)
>>>free = 155189248 (148.0MB)
>>>0.0% used
>>>
>>>
>>> I did not add the metaspace details since that did not seem to be in
>>> line with the 'universe' output for other GCs. I have added a new command
>>> "g1regiondetails" to display the region details, and have modified the
>>> tests accordingly.
>>>
>>> hsdb> g1regiondetails
>>> Region Details:
>>> Region: 0x00072520,0x00072520,0x00072530:Free
>>> Region: 0x00072530,0x00072530,0x00072540:Free
>>> Region: 0x00072540,0x00072540,0x00072550:Free
>>> Region: 0x00072550,0x00072550,0x00072560:Free
>>> Region: 0x00072560,0x00072560,0x00072570:Free
>>> Region: 0x00072570,0x00072570,0x00072580:Free
>>> ...
>>>
>>> Thanks,
>>> Jini.
>>>
>>>
>>> On 2/28/2018 12:56 PM, Jini George wrote:
>>>
 Thank you very much, Stefan. My answers inline.

 On 2/27/2018 3:30 PM, Stefan Johansson wrote:

> Hi Jini,
>

 JIRA ID:https://bugs.openjdk.java.net/browse/JDK-8175312
>>> Webrev: http://cr.openjdk.java.net/~jgeorge/8175312/webrev.00/index.
>>> html
>>>
>>> It looks like a file is missing, did you forget to add it to the
> changeset?
>

 Indeed, I had missed that! I added the missing file in the following
 webrev:

 http://cr.openjdk.java.net/~jgeorge/8175312/webrev.01/

 ---
> open/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/G1CollectedHeap.java:36:
> error: cannot find symbol
> import sun.jvm.hotspot.gc.shared.PrintRegionClosure;
> ---
>
> Otherwise the change looks good, but I would like to see the output
> live. For a big heap this will print a lot of data, just wondering if the
> universe command is the correct choice for this kind of output. I like
> having the possibility to print all regions, so I want the change but 
> maybe
> it should be a different command and 'universe' just prints a little more
> than before. Something like our logging heap-summary at shutdown:
> garbage-first heap   total 16384K, used 3072K [0xff00,
> 0x0001)
>   region size 1024K, 4 young (4096K), 0 survivors (0K)
> Metaspace   used 6731K, capacity 6825K, committed 7040K, reserved
> 1056768K
>   class spaceused 559K, capacity 594K, committed 640K, reserved
> 1048576K
>

 Ok, will add this, and could probably have the region details displayed
 under a new command called "g1regiondetails", or some such, and send out a
 new webrev.

 Thanks,
 Jini.


> Thanks,
> Stefan
>
>> Modifications have been made to display the regions like:
>>>
>>> ...
>>> Region: 0x0005c540,0x0005c560,0x0005c560:Old
>>> Region: 0x0005c560,0x0005c580,0x0005c580:Old
>>> Region: 0x0005c580,0x0005c5a0,0x0005c5a0:Old
>>> Region: 0x0005c5a0,0x0005c5c0,0x0005c5c0:Old
>>> Region: 0x0005c5c0,0x0005c5c0,0x0005c5e0:
>>> Free
>>> Region: 0x0005c5e0,0x0005c5e0,0x0005c600:
>>> Free
>>> Region: 0x0005c600,0x0005c620,0x0005c620:Old
>>> ...
>>>
>>> Th

Re: RFR: 8130115: REDO - Reduce Symbol::_identity_hash to 2 bytes

[Yumin Qi]

Ioi,

  I am trying to add a test case in SA for the testing as you 
mentioned. The easy part is adding a simple SA Tool (SymbolsInfo.java) 
to get the Symbol information  but encountered a problem as:


  In the testing java process (1), create (spawn) another java 
process(2), which will run SA (SymbolsInfo) and attach back to the 
process (1).  It failed due to time out waiting for response from 
target(1). I am investigating and trying to find a solution.  It may 
have issue for such case.


  Webrev (Note: in the webrev, WhitBox.java, white_box.cpp, 
SymbolsInfo.java and IdentityHashForSymbols.java added to previous 
version webrev01)


  http://cr.openjdk.java.net/~minqi/8130115/webrev02/

  Any one has comments how to solve the problem?
  Following are the two processes, you can see process 2) has parent as 
process 1): ($WS, $TEST are for real locations on my host machine)


1) 25939 25807 19 09:32 pts/100:00:00 $MYJDK/bin/java 
-Dtest.src=$WS/hotspot/test/serviceability/sa 
-Dtest.src.path=$WS/hotspot/test/serviceability/sa:$WS/hotspot/test/testlibrary:$WS/test/lib 
-Dtest.classes=$TEST/JTwork/classes/serviceability/sa 
-Dtest.class.path=$TEST/JTwork/classes/serviceability/sa:$TEST/JTwork/classes/testlibrary:$TEST/test/lib 
-Dtest.vm.opts= -Dtest.tool.vm.opts= -Dtest.compiler.opts= 
-Dtest.java.opts= -Dtest.jdk=$MYJDK -Dcompile.jdk=$MYJDK 
-Dtest.timeout.factor=1.0 -Xbootclasspath/a:. 
-XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI 
com.sun.javatest.regtest.agent.MainWrapper 
$TEST/JTwork/classes/serviceability/sa/IdentityHashForSymbols.jta


2) 25976 25939 63 09:32 pts/100:00:03 $MYJDK/bin/java -cp 
$TEST/jtreg/lib/javatest.jar:$TEST/jtreg/lib/jtreg.jar:$TEST/JTwork/classes/serviceability/sa:$WS/hotspot/test/serviceability/sa:$TEST/JTwork/classes/testlibrary:$TEST/test/lib 
sun.jvm.hotspot.tools.SymbolsInfo 25939



Thanks
Yumin

On 7/27/2015 9:29 PM, Ioi Lam wrote:

Hi Yumin,

The C code changes look good to me.

I am a little concerned about the Java code's calculation of 
identityHash:


Java version:
  86   public int identityHash() {
  87 long addr_value = getAddress().asLongValue();
  88 int  addr_bits = (int)(addr_value >> 
(VM.getVM().getLogMinObjAlignmentInBytes() + 3));

  89 int  length = (int)getLength();
  90 int  byte0 = getByteAt(0);
  91 int  byte1 = getByteAt(1);
  92 int  id_hash = (int)(0x & idHash.getValue(this.addr));
  93 return id_hash |
  94((addr_bits ^ (length << 8) ^ ((byte0 << 8) | byte1)) 
<< 16);

  95   }

C version:
 148   unsigned identity_hash() {
 149 unsigned addr_bits = (unsigned)((uintptr_t)this >> 
(LogMinObjAlignmentInBytes + 3));

 150 return (unsigned)_identity_hash |
 151((addr_bits ^ (_length << 8) ^ (( _body[0] << 8) | 
_body[1])) << 16);

 152   }

The main problem is to correctly emulate the C unsigned operations in 
the Java code. I've eyeballed the code and it seems correct, but I am 
wondering if you have actually tested and verified that the Java 
version indeed returns the same value as the C code? A unit test case 
would be good:


 * Add a new test in hotspot/agent/test
 * Get a few Symbols (e.g., call
   sun.jvm.hotspot.runtime.VM.getSymbolTable and iterate over the first
   1000 Symbols)
 * For each Symbol, call its Symbol.identityHash() method
 * Add a new whitebox API to return the C version of the
   identity_hash() value
 * Check if the C value is the same as the Java value

Please run the test on all platforms (both 32-bit and 64-bit, and all 
OSes).


Thanks
- Ioi


On 7/15/15 10:37 AM, Yumin Qi wrote:

Hi,

  This is redo for bug 8087143, in that push, it caused failure on 
Serviceability Agent failed to get type for "_identity_hash": 
mistakenly used JShortField for it, but in fact it still is 
CIntegerField. In this change, besides of the original change in 
hotspot/src, I add code to calculate identity_hash in hotspot/agent 
based on the changed in hotspot.


Old webrev for 8087143:
bug: https://bugs.openjdk.java.net/browse/JDK-8087143
webrev: http://cr.openjdk.java.net/~minqi/8087143/webrev03/

Summary: _identity_hash is an integer in Symbol (SymbolBase), it is 
used to compute hash bucket index by modulus division of table size. 
Currently in hotspot, no table size is more than 65535 so we can use 
short instead.  For case with table size over 65535 we can use the 
first two bytes of symbol data to be as the upper 16 bits for the 
calculation but rare cases.


New webrev for 8130115:
bug: https://bugs.openjdk.java.net/browse/JDK-8130115
webrev: http://cr.openjdk.java.net/~minqi/8130115/webrev01/


Tests: JPRT, SA manual tests, -atk quick, jtreg hotspot/runtime
Also internal large application used for hashtable data analysis --- 
the No. of loaded classes is big(over 19K), and tested with different 
bucket sizes including over 65535 to see the new algorithm for 
identity_hash calcu

RFR: 8130115: REDO - Reduce Symbol::_identity_hash to 2 bytes

[Yumin Qi]

Hi,

  This is redo for bug 8087143, in that push, it caused failure on 
Serviceability Agent failed to get type for "_identity_hash": mistakenly 
used JShortField for it, but in fact it still is CIntegerField. In this 
change, besides of the original change in hotspot/src, I add code to 
calculate identity_hash in hotspot/agent based on the changed in hotspot.


Old webrev for 8087143:
bug: https://bugs.openjdk.java.net/browse/JDK-8087143
webrev: http://cr.openjdk.java.net/~minqi/8087143/webrev03/

Summary: _identity_hash is an integer in Symbol (SymbolBase), it is used 
to compute hash bucket index by modulus division of table size. 
Currently in hotspot, no table size is more than 65535 so we can use 
short instead.  For case with table size over 65535 we can use the first 
two bytes of symbol data to be as the upper 16 bits for the calculation 
but rare cases.


New webrev for 8130115:
bug: https://bugs.openjdk.java.net/browse/JDK-8130115
webrev: http://cr.openjdk.java.net/~minqi/8130115/webrev01/


Tests: JPRT, SA manual tests, -atk quick, jtreg hotspot/runtime
Also internal large application used for hashtable data analysis --- the 
No. of loaded classes is big(over 19K), and tested with different bucket 
sizes including over 65535 to see the new algorithm for identity_hash 
calculation, result shows the consistency before and after the fix.


Thanks
Yumin

Re: RFR 8071962 The SA code needs to be updated to support Symbol lookup from the shared archive

[Yumin Qi]

Jiangli,

  Two minor suggestions (did not pay attention in first round, anyway, 
no harm):


  1) CompactHashtable.java:

  58 private static long uintxSize;

   I did not find the usage of this var, please remove it.

  2)

+  anyway. Searches the regular symbol table and the shared symbol
+  table. Null is returned if the given string is not in the symbol
+  tables. */

  Maybe better with: "Return null if the given name is not found in both 
tables." ?

  No need to send review again if you change.

Thanks
Yumin



On 1/30/2015 2:31 PM, Jiangli Zhou wrote:

Here is the updated webrev that incorporates all feedbacks:

   http://cr.openjdk.java.net/~jiangli/8071962/webrev.01/

Thanks,
Jiangli

On Jan 30, 2015, at 10:05 AM, Jiangli Zhou  wrote:


Hi Ioi,

Thanks for the review.

On Jan 30, 2015, at 9:49 AM, Ioi Lam  wrote:


Hi Jiangli,

The code looks good. I am wondering if you need a more specific JTREG test for 
the new sun/jvm/hotspot/utilities/CompactHashTable.java. There's a lot of code 
in CompactHashTable.java. Will the existing test case in 8071962 provide enough 
coverage?

My thinking would be yes. The symbol lookup is very foundational operations in 
sun.jvm.hotspot.tools.DumpJFR. If it fails, it would get exception immediately. 
We also have JTREG tests that uncovers the SA symbol lookup issue.

Thanks,
Jiangli


Thanks
- Ioi

On 1/29/15, 5:46 PM, Jiangli Zhou wrote:

Hi Serguei,

Thanks for noticing that. It’s actually a duplicated bug ID for the same issue. 
I just fixed the web rev: 
http://cr.openjdk.java.net/~jiangli/8071962/webrev.00/.

Thanks,
Jiangli

On Jan 29, 2015, at 5:40 PM, serguei.spit...@oracle.com wrote:


On 1/29/15 5:13 PM, Jiangli Zhou wrote:

Hi,

Please review the change for JDK-8071962, "The SA code needs to be updated to 
support Symbol lookup from the shared archive”.

In JDK-8059510, we introduced a compact form of hash table for the symbols in 
shared archive. The shared symbols are stored separately from the regular 
symbol table. The VM looks up both tables when searching for existing symbol at 
runtime. The SA code needs to do the same when looking up symbols.

Webrev: http://cr.openjdk.java.net/~jiangli/8067977/webrev.00/

Jiangli,

I'm not reviewing, just wanted to make sure there is no typo here ...
The webrev bug number is 8067977, but the RFR bug number is 8071962 which is 
strange.

Thanks,
Serguei


Tested on both 32-bit and 64-bit platforms:
  bin/java sun.jvm.hotspot.tools.DumpJFR 

JPRT tests in progress.

Thanks,
Jiangli

Re: RFR 8071962 The SA code needs to be updated to support Symbol lookup from the shared archive

[Yumin Qi]

Looks good to me. Not a 'R'eviwer.

Thanks
Yumin

On 1/29/2015 5:46 PM, Jiangli Zhou wrote:

Hi Serguei,

Thanks for noticing that. It’s actually a duplicated bug ID for the same issue. 
I just fixed the web rev: 
http://cr.openjdk.java.net/~jiangli/8071962/webrev.00/.

Thanks,
Jiangli

On Jan 29, 2015, at 5:40 PM, serguei.spit...@oracle.com wrote:


On 1/29/15 5:13 PM, Jiangli Zhou wrote:

Hi,

Please review the change for JDK-8071962, "The SA code needs to be updated to 
support Symbol lookup from the shared archive”.

In JDK-8059510, we introduced a compact form of hash table for the symbols in 
shared archive. The shared symbols are stored separately from the regular 
symbol table. The VM looks up both tables when searching for existing symbol at 
runtime. The SA code needs to do the same when looking up symbols.

Webrev: http://cr.openjdk.java.net/~jiangli/8067977/webrev.00/

Jiangli,

I'm not reviewing, just wanted to make sure there is no typo here ...
The webrev bug number is 8067977, but the RFR bug number is 8071962 which is 
strange.

Thanks,
Serguei


Tested on both 32-bit and 64-bit platforms:
   bin/java sun.jvm.hotspot.tools.DumpJFR 

JPRT tests in progress.

Thanks,
Jiangli

Re: RFR 8067982: some jcmd /gc/heap_dump tests failed: hprof output contains warning or error

[Yumin Qi]

I'm not a "R"eviewer, so you still need one. (Forgot to mention in my 
reply email).


Thanks
Yumin

On 1/14/2015 8:33 AM, Jiangli Zhou wrote:

Hi Yumin,

Thanks for the review! Will fix the copyright.

Thanks,
Jiangli

On 01/13/2015 07:56 PM, Yumin Qi wrote:

Looks good to me. Could you update the copyright year?

Thanks
Yumin

On 1/13/2015 4:50 PM, Jiangli Zhou wrote:

Hi,

Please review the fix for JDK-8067982 
<https://bugs.openjdk.java.net/browse/JDK-8067982> (Some jcmd 
/gc/heap_dump tests failed: hprof output contains warning or error). 
The jcmd heap_dump tests failed due to incomplete symbol information 
in the dump output. The shared symbols are not included in the dump 
output because they are not in the SymbolTable, but in a separate 
table within the shared archive. The fix is to include the shared 
symbols when iterating all symbols in 
SymbolTable::symbols_do(SymbolClosure).


  http://cr.openjdk.java.net/~jiangli/8067982/webrev.00/

Tested with vm.jcmd tests and jprt. All failed jcmd heap dump tests 
passed with the fix.


Thanks,
Jiangli

Re: RFR 8067982: some jcmd /gc/heap_dump tests failed: hprof output contains warning or error

[Yumin Qi]

Looks good to me. Could you update the copyright year?

Thanks
Yumin

On 1/13/2015 4:50 PM, Jiangli Zhou wrote:

Hi,

Please review the fix for JDK-8067982 
 (Some jcmd 
/gc/heap_dump tests failed: hprof output contains warning or error). 
The jcmd heap_dump tests failed due to incomplete symbol information 
in the dump output. The shared symbols are not included in the dump 
output because they are not in the SymbolTable, but in a separate 
table within the shared archive. The fix is to include the shared 
symbols when iterating all symbols in 
SymbolTable::symbols_do(SymbolClosure).


  http://cr.openjdk.java.net/~jiangli/8067982/webrev.00/

Tested with vm.jcmd tests and jprt. All failed jcmd heap dump tests 
passed with the fix.


Thanks,
Jiangli

Re: RFR: 8038468: java/lang/instrument/ParallelTransformerLoader.sh fails with ClassCircularityError

[Yumin Qi]

Hi, David and all,

  Second webrev here: http://cr.openjdk.java.net/~minqi/8038468/webrev01/

  Answer to David's question about 'main' and 'DestroyJavaVM'. I still 
did not find how when exception printing the stack trace, 'main' was 
retrieved but, at the moment JavaThread for "DestroyJavaVM' was created, 
'main' is not dead. They may exist and with same C thread and id. This 
may cause we got 'main' not 'DestroyJavaVM'.


  Loading another class from agent in 'transform' with same custom 
class loader is not a good design. We already have two threads loading 
from the agent in parallel, TestClass1 in 'main' and TestClass2 in 
'TestThread'.  Should avoid loading another class with same agent in 
'transform' in nested.


  Thanks
  Yumin

On 10/17/2014 10:54 PM, David Holmes wrote:

Hi Yumin,

Quick response ... when shutdown is initiated the Shutdown class will 
be loaded and initialized:


at java.lang.Shutdown.(Shutdown.java:61)

Presumably this static initialization is what triggers the involvement 
of the agent to do the transform, and hence encounters the exception. 
Though I'm unclear how it still reports "main" as the name when it has 
now become "DestroyJavaVM"


David

On 18/10/2014 3:08 PM, Yumin Qi wrote:

David,  (cc Karen)

   I think I got why it throws CircularityError in 'main' thread.
   The CircularityError thrown in TestThread, which was handled in
classloading, the loading class is put into unresolved list. Note we
clean pending exception and return null to caller, which in the search
next will load the instance class. There is no exception in java level
be caught in TestThread.
   When main ended,  we create a JavaThread named 'DestroyJavaVM' and
give the thread id the current thread id, which is the main thread id.
Since the All JavaThread object should be freed when this last
JavaThread exit, I have no idea how the 'DestroyJavaVM' thread saw the
exception,  from the stack trace, the calling begins with

ShutDown.java:

 /* The preceding static fields are protected by this lock */
 private static class Lock { };
 private static Object lock = new Lock();
//<<< line 61

   How come the call via agent and call transform? At shutdown time, do
we need to turn down the request to agent at this time?

Thanks
Yumin


   java.lang.Exception: Stack trace
 at java.lang.Thread.dumpStack(Thread.java:1329)
 at
ParallelTransformerLoaderAgent$TestTransformer.transform(ParallelTransformerLoaderAgent.java:92) 



 at
sun.instrument.TransformerManager.transform(TransformerManager.java:188)
 at
sun.instrument.InstrumentationImpl.transform(InstrumentationImpl.java:428) 


java.lang.Exception: Stack trace
 at java.lang.Thread.dumpStack(Thread.java:1329)
 at
ParallelTransformerLoaderAgent$TestTransformer.transform(ParallelTransformerLoaderAgent.java:92) 



 at
sun.instrument.TransformerManager.transform(TransformerManager.java:188)
 at
sun.instrument.InstrumentationImpl.transform(InstrumentationImpl.java:428) 


 at java.lang.Shutdown.(Shutdown.java:61)

This output in





On 10/15/2014 9:58 AM, Yumin Qi wrote:

David,

  I will take another detail trace to see where the exception begins
in main thread, it should not thrown in main thread. I only saw it is
thrown in TestThread, not main, not DestroyJavaVM. If that happens,
maybe something wrong in vm.
  The output in all 'failed' case (many failed not cause exception
output, not caught), the main thread got the exception. That is not
right.

Thanks
Yumin

On 10/14/2014 5:28 PM, David Holmes wrote:

Hi Yumin,

On 15/10/2014 4:40 AM, Yumin Qi wrote:

David,  Thanks for the comment. See embedded.


On 10/13/2014 7:30 PM, David Holmes wrote:

Hi Yumin,

jdk9-dev is not the best place for code review requests.
serviceability-dev would be better for this test.

On 14/10/2014 8:58 AM, Yumin Qi wrote:

bug: https://bugs.openjdk.java.net/browse/JDK-8038468
webrev:*http://cr.openjdk.java.net/~minqi/8038468/webrev00/

the bug marked as confidential so post the webrev internally.


Not any more :)


Thanks. I changed to non security related bug. Usually when test
failed,
a confidential bug is filed. I would like to create bug open if the
test
is in open part.

Problem: The test case tries to load a class from the same jar via
agent
in the middle of loading another class from the jar via same class
loader in same thread. The call happens in transform which is a 
rare

case --- in middle of loading class, loading another class. The
result
is a CircularityError. When first class is in loading, in vm we put
JarLoader$2 on place holder table, then we start the defineClass,
which
calls transform, begins loading the second class so go along the 
same

routine for loa

Re: RFR: 8038468: java/lang/instrument/ParallelTransformerLoader.sh fails with ClassCircularityError

[Yumin Qi]

in debugger, it indeed is 'main' thread id:

output of loading TestClass1 (which is always in main):


'TestClass1', loader class loader 0x72c62780a 
'java/net/URLClassLoader', supername 'java/lang/Object'

loadInstanceThreadQ threads:AllocatedObj(0x7000b000),
superThreadQ threads:
defineThreadQ threads:AllocatedObj(0x7000b000),


thread_id is 24590, in debugger (note: gdb attach result) I checked here:

jni_DestroyJavaVM:

  JNIWrapper("DestroyJavaVM");
  JNIEnv *env;
  JavaVMAttachArgs destroyargs;
  destroyargs.version = CurrentVersion;
  destroyargs.name = (char *)"DestroyJavaVM";
  destroyargs.group = NULL;
  res = vm->AttachCurrentThread((void **)&env, (void *)&destroyargs);
  if (res != JNI_OK) {
return res;
  }

  // Since this is not a JVM_ENTRY we have to set the thread state 
manually before entering.
  JavaThread* thread = JavaThread::current();// 
<- we returned the JavaThread, same thread 
created in attach

  ThreadStateTransition::transition_from_native(thread, _thread_in_vm);
  if (Threads::destroy_vm()) {
// Should not change thread state, VM is gone
vm_created = false;
res = JNI_OK;
return res;
  } else {
ThreadStateTransition::transition_and_fence(thread, _thread_in_vm, 
_thread_in_native);

res = JNI_ERR;
return res;
  }

(gdb) print *(OSThread*)0x7000be70
$3 = {> = { = {_vptr.AllocatedObj 
= 0x77ca3870}, }, _start_proc = 0,
  _start_parm = 0x0, _state = RUNNABLE, _interrupted = 0, _thread_type 
= -235802127, _pthread_id = 140737326790400, _caller_sigmask = {
__val = {0, 140737338666947, 4294967296, 140737344153969, 0, 
140737266533664, 140737326787424, 140737338942742, 140737219968624,
  140737219968624, 0, 140737219968624, 140737326787472, 
140737338949853, 0, 140737338944081}}, sr = {
_state = os::SuspendResume::SR_RUNNING}, _siginfo = 0x0, _ucontext 
= 0x0, _expanding_stack = 0, _alt_sig_stack = 0x0,

  _startThread_lock = 0x72c74520, _thread_id = 24590}

Note the thread_id is 24590.

As how the name is still 'main' in stack trace still not known.

Thanks
Yumin





On 10/17/2014 10:54 PM, David Holmes wrote:

Hi Yumin,

Quick response ... when shutdown is initiated the Shutdown class will 
be loaded and initialized:


at java.lang.Shutdown.(Shutdown.java:61)

Presumably this static initialization is what triggers the involvement 
of the agent to do the transform, and hence encounters the exception. 
Though I'm unclear how it still reports "main" as the name when it has 
now become "DestroyJavaVM"


David

On 18/10/2014 3:08 PM, Yumin Qi wrote:

David,  (cc Karen)

   I think I got why it throws CircularityError in 'main' thread.
   The CircularityError thrown in TestThread, which was handled in
classloading, the loading class is put into unresolved list. Note we
clean pending exception and return null to caller, which in the search
next will load the instance class. There is no exception in java level
be caught in TestThread.
   When main ended,  we create a JavaThread named 'DestroyJavaVM' and
give the thread id the current thread id, which is the main thread id.
Since the All JavaThread object should be freed when this last
JavaThread exit, I have no idea how the 'DestroyJavaVM' thread saw the
exception,  from the stack trace, the calling begins with

ShutDown.java:

 /* The preceding static fields are protected by this lock */
 private static class Lock { };
 private static Object lock = new Lock();
//<<< line 61

   How come the call via agent and call transform? At shutdown time, do
we need to turn down the request to agent at this time?

Thanks
Yumin


   java.lang.Exception: Stack trace
 at java.lang.Thread.dumpStack(Thread.java:1329)
 at
ParallelTransformerLoaderAgent$TestTransformer.transform(ParallelTransformerLoaderAgent.java:92) 



 at
sun.instrument.TransformerManager.transform(TransformerManager.java:188)
 at
sun.instrument.InstrumentationImpl.transform(InstrumentationImpl.java:428) 


java.lang.Exception: Stack trace
 at java.lang.Thread.dumpStack(Thread.java:1329)
 at
ParallelTransformerLoaderAgent$TestTransformer.transform(ParallelTransformerLoaderAgent.java:92) 



 at
sun.instrument.TransformerManager.transform(TransformerManager.java:188)
 at
sun.instrument.InstrumentationImpl.transform(InstrumentationImpl.java:428) 


 at java.lang.Shutdown.(Shutdown.java:61)

This output in





On 10/15/2014 9:58 AM, Yumin Qi wrote:

David,

  I will take another detail trace to see where the exception begins
in main thread, it should not thrown in main thread. I only saw it is
thrown in TestThread, not main, not DestroyJavaVM. If that happens,
maybe something wrong in vm.
  The output in all

Re: RFR: 8038468: java/lang/instrument/ParallelTransformerLoader.sh fails with ClassCircularityError

[Yumin Qi]

David,  (cc Karen)

  I think I got why it throws CircularityError in 'main' thread.
  The CircularityError thrown in TestThread, which was handled in 
classloading, the loading class is put into unresolved list. Note we 
clean pending exception and return null to caller, which in the search 
next will load the instance class. There is no exception in java level 
be caught in TestThread.
  When main ended,  we create a JavaThread named 'DestroyJavaVM' and 
give the thread id the current thread id, which is the main thread id. 
Since the All JavaThread object should be freed when this last 
JavaThread exit, I have no idea how the 'DestroyJavaVM' thread saw the 
exception,  from the stack trace, the calling begins with


ShutDown.java:

/* The preceding static fields are protected by this lock */
private static class Lock { };
private static Object lock = new Lock(); 
//<<< line 61


  How come the call via agent and call transform? At shutdown time, do 
we need to turn down the request to agent at this time?


   Thanks
   Yumin


  java.lang.Exception: Stack trace
at java.lang.Thread.dumpStack(Thread.java:1329)
at 
ParallelTransformerLoaderAgent$TestTransformer.transform(ParallelTransformerLoaderAgent.java:92)
at 
sun.instrument.TransformerManager.transform(TransformerManager.java:188)
at 
sun.instrument.InstrumentationImpl.transform(InstrumentationImpl.java:428)

java.lang.Exception: Stack trace
at java.lang.Thread.dumpStack(Thread.java:1329)
at 
ParallelTransformerLoaderAgent$TestTransformer.transform(ParallelTransformerLoaderAgent.java:92)
at 
sun.instrument.TransformerManager.transform(TransformerManager.java:188)
at 
sun.instrument.InstrumentationImpl.transform(InstrumentationImpl.java:428)

at java.lang.Shutdown.(Shutdown.java:61)

This output in





On 10/15/2014 9:58 AM, Yumin Qi wrote:

David,

  I will take another detail trace to see where the exception begins 
in main thread, it should not thrown in main thread. I only saw it is 
thrown in TestThread, not main, not DestroyJavaVM. If that happens, 
maybe something wrong in vm.
  The output in all 'failed' case (many failed not cause exception 
output, not caught), the main thread got the exception. That is not 
right.


Thanks
Yumin

On 10/14/2014 5:28 PM, David Holmes wrote:

Hi Yumin,

On 15/10/2014 4:40 AM, Yumin Qi wrote:

David,  Thanks for the comment. See embedded.


On 10/13/2014 7:30 PM, David Holmes wrote:

Hi Yumin,

jdk9-dev is not the best place for code review requests.
serviceability-dev would be better for this test.

On 14/10/2014 8:58 AM, Yumin Qi wrote:

bug: https://bugs.openjdk.java.net/browse/JDK-8038468
webrev:*http://cr.openjdk.java.net/~minqi/8038468/webrev00/

the bug marked as confidential so post the webrev internally.


Not any more :)

Thanks. I changed to non security related bug. Usually when test 
failed,
a confidential bug is filed. I would like to create bug open if the 
test

is in open part.
Problem: The test case tries to load a class from the same jar via 
agent

in the middle of loading another class from the jar via same class
loader in same thread. The call happens in transform which is a rare
case --- in middle of loading class, loading another class. The 
result

is a CircularityError. When first class is in loading, in vm we put
JarLoader$2 on place holder table, then we start the defineClass, 
which

calls transform, begins loading the second class so go along the same
routine for loading JarLoader$2 first, found it already in 
placeholder

table. A CircularityError is thrown.
Fix: The test case should not call loading class with same class 
loader
in same thread from same jar in 'transform' method. I modify it 
loading

with system class loader and we expect see ClassNotFoundException.
Detail see bug comments.


It is not clear to me that the test is incorrect. It is also unclear
why such an old test is now failing - we must have changed something.
And it's unclear whether what the test does with your change is
actually testing what the test wanted to test.

It seems to me that the actual problem in the test is the reference to
the "main" thread ie:

 if (!tName.equals("main"))

The test knows not to do the loading in the main thread, but has
overlooked the fact that the main thread, upon the end of main()
becomes the DestroyJavaVM thread - and it is that thread which
encounters the ClassCircularityError:

Starting test with 1000 iterations
Thread 'DestroyJavaVM' has called transform()

So perhaps the right fix is to expand the above to:

 if (!tName.equals("main") && !tName.equals("DestroyJavaVM"))

? I admit I'm having trouble seeing the full picture in this test.


It is not DestroyJavaVM thread cause CircularityError. It is TestThread
cause CircularityErro

Re: RFR: 8038468: java/lang/instrument/ParallelTransformerLoader.sh fails with ClassCircularityError

[Yumin Qi]

David,

  I will take another detail trace to see where the exception begins in 
main thread, it should not thrown in main thread. I only saw it is 
thrown in TestThread, not main, not DestroyJavaVM. If that happens, 
maybe something wrong in vm.
  The output in all 'failed' case (many failed not cause exception 
output, not caught), the main thread got the exception. That is not right.


Thanks
Yumin

On 10/14/2014 5:28 PM, David Holmes wrote:

Hi Yumin,

On 15/10/2014 4:40 AM, Yumin Qi wrote:

David,  Thanks for the comment. See embedded.


On 10/13/2014 7:30 PM, David Holmes wrote:

Hi Yumin,

jdk9-dev is not the best place for code review requests.
serviceability-dev would be better for this test.

On 14/10/2014 8:58 AM, Yumin Qi wrote:

bug: https://bugs.openjdk.java.net/browse/JDK-8038468
webrev:*http://cr.openjdk.java.net/~minqi/8038468/webrev00/

the bug marked as confidential so post the webrev internally.


Not any more :)


Thanks. I changed to non security related bug. Usually when test failed,
a confidential bug is filed. I would like to create bug open if the test
is in open part.
Problem: The test case tries to load a class from the same jar via 
agent

in the middle of loading another class from the jar via same class
loader in same thread. The call happens in transform which is a rare
case --- in middle of loading class, loading another class. The result
is a CircularityError. When first class is in loading, in vm we put
JarLoader$2 on place holder table, then we start the defineClass, 
which

calls transform, begins loading the second class so go along the same
routine for loading JarLoader$2 first, found it already in placeholder
table. A CircularityError is thrown.
Fix: The test case should not call loading class with same class 
loader
in same thread from same jar in 'transform' method. I modify it 
loading

with system class loader and we expect see ClassNotFoundException.
Detail see bug comments.


It is not clear to me that the test is incorrect. It is also unclear
why such an old test is now failing - we must have changed something.
And it's unclear whether what the test does with your change is
actually testing what the test wanted to test.

It seems to me that the actual problem in the test is the reference to
the "main" thread ie:

 if (!tName.equals("main"))

The test knows not to do the loading in the main thread, but has
overlooked the fact that the main thread, upon the end of main()
becomes the DestroyJavaVM thread - and it is that thread which
encounters the ClassCircularityError:

Starting test with 1000 iterations
Thread 'DestroyJavaVM' has called transform()

So perhaps the right fix is to expand the above to:

 if (!tName.equals("main") && !tName.equals("DestroyJavaVM"))

? I admit I'm having trouble seeing the full picture in this test.


It is not DestroyJavaVM thread cause CircularityError. It is TestThread
cause CircularityError.


Not according to the bug report:

Starting test with 1000 iterationsThread 'DestroyJavaVM' has called 
transform()

Thread 'DestroyJavaVM' has called transform()
result=1
--System.err:(14/920)--
Exception in thread "main" java.lang.ClassCircularityError: 
sun/misc/URLClassPath$JarLoader$2


This shows that "main" got the CCE. Which in itself is confusing given 
we also report "Thread 'DestroyJavaVM' has called transform()" and 
they are in fact the same thread!


David
-



In TestThread (DestroyJavaVM may cause same I think, but not seen in
debug):

 forName("TestClass2", true, classLoader);  < the loader is
customer loader which is obtained from agent code.
  -->.. transform(...)
  -->defineClass(...)
-->.. call into vm, we need to load JarLoader$2
since JarLoader$1 used
->resolve_instance_class_or_null
// here we create PlaceTableEntry for
JarLoader$2, put into place holder table
-->..
--->forName("TestClass3", true,
classLoader);
-->... transform(...)
-->defineClass(...)
-->.. call into vm
again. Now JarLoader$2 is not loaded, but it is in placeholder table, so
throw_circularity_error set and throw.
   ...
  With custom loader, agent's transform will be called, then it
loads TestClass3, repeat the same steps as loading TestClass2. The
problem is JarLoader$2 has not been loaded yet but in place holder table
(this is for checking CircularityError), then begins loading TestClass3,
this is a recursive and embedded case.  The non-failed case also saw
CircularityError thrown, but somehow the test 

Re: RFR: 8038468: java/lang/instrument/ParallelTransformerLoader.sh fails with ClassCircularityError

[Yumin Qi]

I have to make a correction: DestroyJavaVM thread will not load 
TestClass2 so no CircularityError happened with it. When it is created 
and run, the loading of TestClass2 already finished.


On 10/14/2014 11:40 AM, Yumin Qi wrote:

David, Thanks for the comment. See embedded.


On 10/13/2014 7:30 PM, David Holmes wrote:

Hi Yumin,

jdk9-dev is not the best place for code review requests. 
serviceability-dev would be better for this test.


On 14/10/2014 8:58 AM, Yumin Qi wrote:

bug: https://bugs.openjdk.java.net/browse/JDK-8038468
webrev:*http://cr.openjdk.java.net/~minqi/8038468/webrev00/

the bug marked as confidential so post the webrev internally.


Not any more :)

Thanks. I changed to non security related bug. Usually when test 
failed, a confidential bug is filed. I would like to create bug open 
if the test is in open part.
Problem: The test case tries to load a class from the same jar via 
agent

in the middle of loading another class from the jar via same class
loader in same thread. The call happens in transform which is a rare
case --- in middle of loading class, loading another class. The result
is a CircularityError. When first class is in loading, in vm we put
JarLoader$2 on place holder table, then we start the defineClass, which
calls transform, begins loading the second class so go along the same
routine for loading JarLoader$2 first, found it already in placeholder
table. A CircularityError is thrown.
Fix: The test case should not call loading class with same class loader
in same thread from same jar in 'transform' method. I modify it loading
with system class loader and we expect see ClassNotFoundException.
Detail see bug comments.


It is not clear to me that the test is incorrect. It is also unclear 
why such an old test is now failing - we must have changed something.
And it's unclear whether what the test does with your change is 
actually testing what the test wanted to test.


It seems to me that the actual problem in the test is the reference 
to the "main" thread ie:


 if (!tName.equals("main"))

The test knows not to do the loading in the main thread, but has 
overlooked the fact that the main thread, upon the end of main() 
becomes the DestroyJavaVM thread - and it is that thread which 
encounters the ClassCircularityError:


Starting test with 1000 iterations
Thread 'DestroyJavaVM' has called transform()

So perhaps the right fix is to expand the above to:

 if (!tName.equals("main") && !tName.equals("DestroyJavaVM"))

? I admit I'm having trouble seeing the full picture in this test.

It is not DestroyJavaVM thread cause CircularityError. It is 
TestThread cause CircularityError.
In TestThread (DestroyJavaVM may cause same I think, but not seen in 
debug):


forName("TestClass2", true, classLoader);  < the loader is 
customer loader which is obtained from agent code.

 -->.. transform(...)
 -->defineClass(...)
   -->.. call into vm, we need to load JarLoader$2 
since JarLoader$1 used

   ->resolve_instance_class_or_null
   // here we create PlaceTableEntry for 
JarLoader$2, put into place holder table

   -->..
   --->forName("TestClass3", true, 
classLoader);

   -->... transform(...)
   -->defineClass(...)
   -->.. call into vm 
again. Now JarLoader$2 is not loaded, but it is in placeholder table, 
so throw_circularity_error set and throw.

  ...
 With custom loader, agent's transform will be called, then it 
loads TestClass3, repeat the same steps as loading TestClass2. The 
problem is JarLoader$2 has not been loaded yet but in place holder 
table (this is for checking CircularityError), then begins loading 
TestClass3, this is a recursive and embedded case.  The non-failed 
case also saw CircularityError thrown, but somehow the test case did 
not fail.  Design like this will cause call transform in transform 
which is the reason CircularityError thrown.


   I have no idea about the original desin of the test case, but think 
it should do this.




Looking at your change, don't leave commented out lines in the code:
 115 // ClassLoader loader = 
ParallelTransformerLoaderAgent.getClassLoader();
 118 //Class.forName("TestClass" + 
index, true, loader);



Will remove

Thanks
Yumin

Thanks,
David


Thanks
Yumin *

Re: RFR: 8038468: java/lang/instrument/ParallelTransformerLoader.sh fails with ClassCircularityError

[Yumin Qi]

David,  Thanks for the comment. See embedded.


On 10/13/2014 7:30 PM, David Holmes wrote:

Hi Yumin,

jdk9-dev is not the best place for code review requests. 
serviceability-dev would be better for this test.


On 14/10/2014 8:58 AM, Yumin Qi wrote:

bug: https://bugs.openjdk.java.net/browse/JDK-8038468
webrev:*http://cr.openjdk.java.net/~minqi/8038468/webrev00/

the bug marked as confidential so post the webrev internally.


Not any more :)

Thanks. I changed to non security related bug. Usually when test failed, 
a confidential bug is filed. I would like to create bug open if the test 
is in open part.

Problem: The test case tries to load a class from the same jar via agent
in the middle of loading another class from the jar via same class
loader in same thread. The call happens in transform which is a rare
case --- in middle of loading class, loading another class. The result
is a CircularityError. When first class is in loading, in vm we put
JarLoader$2 on place holder table, then we start the defineClass, which
calls transform, begins loading the second class so go along the same
routine for loading JarLoader$2 first, found it already in placeholder
table. A CircularityError is thrown.
Fix: The test case should not call loading class with same class loader
in same thread from same jar in 'transform' method. I modify it loading
with system class loader and we expect see ClassNotFoundException.
Detail see bug comments.


It is not clear to me that the test is incorrect. It is also unclear 
why such an old test is now failing - we must have changed something.
And it's unclear whether what the test does with your change is 
actually testing what the test wanted to test.


It seems to me that the actual problem in the test is the reference to 
the "main" thread ie:


 if (!tName.equals("main"))

The test knows not to do the loading in the main thread, but has 
overlooked the fact that the main thread, upon the end of main() 
becomes the DestroyJavaVM thread - and it is that thread which 
encounters the ClassCircularityError:


Starting test with 1000 iterations
Thread 'DestroyJavaVM' has called transform()

So perhaps the right fix is to expand the above to:

 if (!tName.equals("main") && !tName.equals("DestroyJavaVM"))

? I admit I'm having trouble seeing the full picture in this test.

It is not DestroyJavaVM thread cause CircularityError. It is TestThread 
cause CircularityError.

In TestThread (DestroyJavaVM may cause same I think, but not seen in debug):

forName("TestClass2", true, classLoader);  < the loader is 
customer loader which is obtained from agent code.

 -->.. transform(...)
 -->defineClass(...)
   -->.. call into vm, we need to load JarLoader$2 
since JarLoader$1 used

   ->resolve_instance_class_or_null
   // here we create PlaceTableEntry for 
JarLoader$2, put into place holder table

   -->..
   --->forName("TestClass3", true, 
classLoader);

   -->... transform(...)
   -->defineClass(...)
   -->.. call into vm 
again. Now JarLoader$2 is not loaded, but it is in placeholder table, so 
throw_circularity_error set and throw.

  ...
 With custom loader, agent's transform will be called, then it 
loads TestClass3, repeat the same steps as loading TestClass2. The 
problem is JarLoader$2 has not been loaded yet but in place holder table 
(this is for checking CircularityError), then begins loading TestClass3, 
this is a recursive and embedded case.  The non-failed case also saw 
CircularityError thrown, but somehow the test case did not fail.  Design 
like this will cause call transform in transform which is the reason 
CircularityError thrown.


   I have no idea about the original desin of the test case, but think 
it should do this.




Looking at your change, don't leave commented out lines in the code:
 115 // ClassLoader loader = 
ParallelTransformerLoaderAgent.getClassLoader();
 118 //Class.forName("TestClass" + 
index, true, loader);



Will remove

Thanks
Yumin

Thanks,
David


Thanks
Yumin *

Re: JDK-7090324: gclog rotation via external tool

[Yumin Qi]

Yasumasa,

  This looks good. I will sponsor the integration --- since it adds new 
feature to jcmd, need internal process approval so after it is approved, 
I will do the push.


Thanks
Yumin

On 3/5/2014 12:32 AM, Yasumasa Suenaga wrote:

Hi Erik, Yumin,

Thank you for comments.

I've updated brief of GCLogFileSize.
New webrev is here:
http://cr.openjdk.java.net/~ysuenaga/JDK-7090324/webrev.07/

Could you review again?


Thanks,

Yasumasa

On 03/05/2014 03:47 PM, Erik Helin wrote:

Hi Yasu,

I think this looks good except for the comment in globals.hpp for 
GCLogFileSize which needs to be updated.


What do you think about the following?

product(uintx, GCLogFileSize, 8*K,  \
"GC log file size, requires UseGCLogFileRotation. " \
"Set to 0 to only trigger rotation via jcmd")   \

Thanks,
Erik

On 03/05/2014 04:52 AM, Yasumasa Suenaga wrote:

Hi Yumin,

Thank you for comments.
I've updated webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-7090324/webrev.06/

Could you review this?


Thanks,

Yasumasa


On 03/04/2014 03:29 PM, Yumin Qi wrote:

Yasumasa,

   Seems no comments for this email so far. I have some comments of
the changes:

1) arguments.cpp:

a) line 1896 & 1897, please merge these two lines into one, the
length will not exceeds longest lines in this file.
b) line 1898 - 1990, move line back 4 spaces, in hotspot we use
two space indentation. The original version is not right.

2) diagnosticCommand.cpp:
  line 665, 672 is empty, can be removed. See other function 
formats.


3)  diagnosticCommand.hpp:
  remove one extra empty line above class RotateGCDcmd
  The empty lines inside the class, between functions can be
removed too. This style changed the former style since class
JMXStartRemoteDCmd added.

4)  TestGCLogRotationViaJcmd.java:
  Java code default indentation is 4 spaces.

5)  ostream.hpp:
 I would like to add some comments to how to use 'force' like:
/*true, force log file rotation from outside JVM */

Thanks
Yumin

On 2/12/2014 6:32 AM, Yasumasa Suenaga wrote:

Hi all,

I've uploaded new webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-7090324/webrev.05/

Erik pointed me that my patch changes current behavior for
GCLogFileSize.

In current implementation, default value of GCLogFileSize is set to
"0" and
if user set this value to less than 8K, JVM adjust it to 8K.


Below are the scenarios:

  1. -Xloggc:test.log -XX:+UseGCLogFileRotation 
-XX:NumberOfGCLogFiles=3

   Should result in GCLogFileSize "0" (GC log rotation will be
turned off)

  2. -Xloggc:test.log -XX:+UseGCLogFileRotation
-XX:NumberOfGCLogFiles=3 -XX:GCLogFileSize=10K
   Should result in GCLogFileSize 10K

  3. -Xloggc:test.log -XX:+UseGCLogFileRotation
-XX:NumberOfGCLogFiles=3 -XX:GCLogFileSize=2K
   Should result in GCLogFileSize 8K

From the result of 3, we can think that GCLogFileSize is set to 8K by
default.
So I want to change default value of this to 8K in globals.hpp .

And I want to treat "0" as special number for rotating by external
trigger.
From the result of 1, if GCLogFileSIze is set to "0",
UseGCLogFileRotation is set to false.
Definition of GCLogFileSize in globals.hpp, "0" means "no rotation" .
Thus I think this changes does not make different behavior from
current implementation.

  product(uintx, GCLogFileSize,
0,  \
  "GC log file size (default: 0 bytes, no rotation).
"  \
  "It requires
UseGCLogFileRotation")   \



Could you review this ?


Thanks,

Yasumasa


On 02/05/2014 09:13 PM, Yasumasa Suenaga wrote:

Sorry, I forgot to paste URL of new webrev :-P
http://cr.openjdk.java.net/~ysuenaga/JDK-7090324/webrev.04/


Yasumasa

On 02/05/2014 09:09 PM, Yasumasa Suenaga wrote:

Hi Erik,

Thank you for reviewing again!
I've updated new webrev.

On 02/05/2014 07:40 PM, Erik Helin wrote:

Hi Yasumasa,

I've looked through the latest patch, it is much better! I just
have two comments:

- ostream.hpp:
  Why did you add GCLogFileSize != 0 in should_rotate? The old 
check

  just checked that _bytes_written > GCLogFileSize.


Default value of GCLogFileSIze is "0" in globals.hpp .
So if this state is missed, should_rotate() returns true in 
anytime.




- TestGCLogRotationViaJcmd.java:
  Could you use the helper class JDKToolLauncher to start jmap? 
The

  code would then be slightly easier to read:

for (int times = 1; times < NUM_LOGS; times++) {
// Run jcmd  GC.rotate_log
JDKToolLauncher jmap = JDKToolLauncher.create("jmap")
.addToolArg(pid)
.addToolArg("GC.rotate_log");
ProcessBuilder pb = new ProcessBuilder(jmap.getCommand());

// Make sure we didn't crash
OutputAnalyzer output = new Out

Re: JDK-7090324: gclog rotation via external tool

[Yumin Qi]

Yasumasa,

   Seems no comments for this email so far. I have some comments of the 
changes:


1) arguments.cpp:

a) line 1896 & 1897, please merge these two lines into one, the 
length will not exceeds longest lines in this file.
b) line 1898 - 1990, move line back 4 spaces, in hotspot we use two 
space indentation. The original version is not right.


2) diagnosticCommand.cpp:
  line 665, 672 is empty, can be removed. See other function formats.

3)  diagnosticCommand.hpp:
  remove one extra empty line above class RotateGCDcmd
  The empty lines inside the class, between functions can be 
removed too. This style changed the former style since class 
JMXStartRemoteDCmd added.


4)  TestGCLogRotationViaJcmd.java:
  Java code default indentation is 4 spaces.

5)  ostream.hpp:
 I would like to add some comments to how to use 'force' like: 
/*true, force log file rotation from outside JVM */


Thanks
Yumin

On 2/12/2014 6:32 AM, Yasumasa Suenaga wrote:

Hi all,

I've uploaded new webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-7090324/webrev.05/

Erik pointed me that my patch changes current behavior for GCLogFileSize.

In current implementation, default value of GCLogFileSize is set to 
"0" and

if user set this value to less than 8K, JVM adjust it to 8K.


Below are the scenarios:

  1. -Xloggc:test.log -XX:+UseGCLogFileRotation -XX:NumberOfGCLogFiles=3
   Should result in GCLogFileSize "0" (GC log rotation will be 
turned off)


  2. -Xloggc:test.log -XX:+UseGCLogFileRotation 
-XX:NumberOfGCLogFiles=3 -XX:GCLogFileSize=10K

   Should result in GCLogFileSize 10K

  3. -Xloggc:test.log -XX:+UseGCLogFileRotation 
-XX:NumberOfGCLogFiles=3 -XX:GCLogFileSize=2K

   Should result in GCLogFileSize 8K

From the result of 3, we can think that GCLogFileSize is set to 8K by 
default.

So I want to change default value of this to 8K in globals.hpp .

And I want to treat "0" as special number for rotating by external 
trigger.
From the result of 1, if GCLogFileSIze is set to "0", 
UseGCLogFileRotation is set to false.

Definition of GCLogFileSize in globals.hpp, "0" means "no rotation" .
Thus I think this changes does not make different behavior from 
current implementation.


  product(uintx, GCLogFileSize, 
0,  \
  "GC log file size (default: 0 bytes, no rotation). 
"  \
  "It requires 
UseGCLogFileRotation")   \




Could you review this ?


Thanks,

Yasumasa


On 02/05/2014 09:13 PM, Yasumasa Suenaga wrote:

Sorry, I forgot to paste URL of new webrev :-P
http://cr.openjdk.java.net/~ysuenaga/JDK-7090324/webrev.04/


Yasumasa

On 02/05/2014 09:09 PM, Yasumasa Suenaga wrote:

Hi Erik,

Thank you for reviewing again!
I've updated new webrev.

On 02/05/2014 07:40 PM, Erik Helin wrote:

Hi Yasumasa,

I've looked through the latest patch, it is much better! I just 
have two comments:


- ostream.hpp:
  Why did you add GCLogFileSize != 0 in should_rotate? The old check
  just checked that _bytes_written > GCLogFileSize.


Default value of GCLogFileSIze is "0" in globals.hpp .
So if this state is missed, should_rotate() returns true in anytime.



- TestGCLogRotationViaJcmd.java:
  Could you use the helper class JDKToolLauncher to start jmap? The
  code would then be slightly easier to read:

for (int times = 1; times < NUM_LOGS; times++) {
// Run jcmd  GC.rotate_log
JDKToolLauncher jmap = JDKToolLauncher.create("jmap")
  .addToolArg(pid)
.addToolArg("GC.rotate_log");
ProcessBuilder pb = new ProcessBuilder(jmap.getCommand());

// Make sure we didn't crash
OutputAnalyzer output = new OutputAnalyzer(pb.start());
output.shouldHaveExitValue(0);
}


I've fixed. Could you check the patch?


Thanks,

Yasumasa



Thanks,
Erik

On 01/30/2014 12:12 PM, Yasumasa Suenaga wrote:

Hi Staffan,

I've uploaded new webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-7090324/webrev.03/

On 2014/01/30 17:23, Staffan Larsen wrote:
Would it be possible for the Diagnostic Command to output the 
location
of the rotated log? When invoking the command it would be good to 
get

some kind of feedback.


I changed rotate_log() to redirect messages to jcmd.
If GC.rotate_log is executed, we can get messages on jcmd console 
as below:


$ jcmd 18976 GC.rotate_log
18976:
2014-01-30 19:59:39 GC log rotation request has been received. 
Saved as

test.log.0
2014-01-30 19:59:39 GC log file created test.log.1




test/gc/7090324/Test7090324.java:
- I think this needs to have the Oracle copyright notice as well.
- Tests should now use descriptive names, not bug numbers:
https://wiki.openjdk.java.net/display/HotSpot/Naming+HotSpot+JTReg+Tests 


- nits: lots of missing spaces before ‘{‘, and after ‘for’, ‘if’
- line 47: you don’t need to clean up old files, jtreg will give 
you a

fresh scratch dire

Re: JDK-7090324: gclog rotation via external tool

[Yumin Qi]

Yasumasa,

I  can sponsor this for you --- will start from next week.

Thanks
Yumin

On 1/24/2014 5:50 AM, Yasumasa Suenaga wrote:

Hi all,

I've created webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-7090324/webrev.00/

This patch works fine on current jdk9/hs-rt .
Could you review this?


I am just an Author. So I need a sponsor.
Could you help me?


Please cooperate.


Thanks,

Yasumasa


On 2013/12/06 0:05, Yasumasa Suenaga wrote:

Hi all,

Did someone read my email?
I really hope to merge "JDK-7090324: gclog rotation via external tool" .

I hear that someone need this RFE. So I want to discuss about this.


Thanks,

Yasumasa

On 2013/11/08 21:47, Yasumasa Suenaga wrote:

Hi all,

Did someone read my mail?

I think that this RFE helps us to watch Java heap on production system.
Also I think this RFE is able to be part of the JEP 158 (Unified JVM 
Logging) .


I want to update this RFE in JDK Bug System, but I don't have account.
So I've posted email at first.


Thanks,

Yasumasa


On 2013/09/30 21:10, Yasumasa Suenaga wrote:

In previous email, I've attached new patch for this RFE.
It works fine with current hsx.


Yasumasa

On 2013/09/29 23:40, Yasu wrote:

Hi all,

We are using "logrotate" tool on RHEL for various log rotation.
Current HotSpot has gclog rotation function for log size base,
however I need to rotate gc log synchronizing with logrotate tool.

So I've created RFE as "JDK-7090324: gclog rotation via external 
tool" .
And Sr. Engineering Manager in Oracle said he use the essence of 
my patch in one

of the jcmd subcommands.
http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2011-September/003274.html 



2 years ago, I posted a patch for this RFE.
But this patch is too old to apply for current HotSpot.

In last month, a similar discussion was appeared in ML.
So I think it's time to discuss this RFE.
http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2013-August/008029.html 




Please cooperate.

Best regards,
Yasumasa

hg: hsx/hotspot-rt/hotspot: 2 new changesets

[yumin . qi]

Changeset: cdf20166ec45
Author:minqi
Date:  2013-11-13 16:24 -0800
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/cdf20166ec45

8025632: Remove all references to MagicLambdaImpl from Hotspot
Summary: MagicLambdaImpl was removed from jdk side, this should be done in vm 
side too
Reviewed-by: coleenp, hseigel, rdurbin

! src/share/vm/classfile/systemDictionary.hpp
! src/share/vm/classfile/verifier.cpp
! src/share/vm/classfile/vmSymbols.hpp
! src/share/vm/runtime/globals.hpp
! src/share/vm/runtime/reflection.cpp
! test/compiler/jsr292/ConcurrentClassLoadingTest.java

Changeset: 3edddbff4865
Author:minqi
Date:  2013-11-13 16:35 -0800
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/3edddbff4865

Merge

Re: RR(S): JDK-8005810: SA: Update Hotspot Serviceability Agent for Method Parameter Reflection and Generic Type Signature Data

[yumin . qi]

 I think in SA, the command 'vmstructsdump' will trigger your code?

Thanks
Yumin

On 10/9/2013 1:59 PM, Dmitry Samersoff wrote:

Staffan,

I don't have specific test, but I rebuild vm testbase with -parameters
and then run nsk.sajdi.

-Dmitry

On 2013-10-09 22:04, Staffan Larsen wrote:

Dmitry,

Are there any tests that exercise this functionality?

/Staffan

On 9 okt 2013, at 17:35, Dmitry Samersoff  wrote:


Hi Everybody,

Please, review the changes for:

JDK-8005810: Update Hotspot Serviceability Agent for Method Parameter
Reflection and Generic Type Signature Data

The fix contributed by Eric McCorkle.

http://cr.openjdk.java.net/~dsamersoff/JDK-8005810/webrev.01/

https://bugs.openjdk.java.net/browse/JDK-8005810

-Dmitry

--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: Fwd: RR:(M): JDK-8025250 Sync linux and bsd versions of ps_core file

[yumin . qi]

 Hi, Dmitry

  Changes looks good. A minor comment for comment:

 agent/src/os/bsd/ps_core.c:233
Please remove the trailing "For"
 agent/src/os/linux/ps_core.c:232
Please remove the trailing "For"

Thanks
Yumin



On 9/25/2013 12:14 AM, Dmitry Samersoff wrote:

Yumin,

Could you take a look?

-Dmitry


 Original Message 
Subject: RR:(M): JDK-8025250 Sync linux and bsd versions of ps_core file
Date: Mon, 23 Sep 2013 19:50:00 +0400
From: Dmitry Samersoff
To: serviceability-dev@openjdk.java.net

Please, review a changes.

http://cr.openjdk.java.net/~dsamersoff/JDK-8025250/webrev.01/

Problem:

agent/os/linux/ps_core.c and agent/os/bsd/ps_core.c has lots of common
code but this code has different indentation, bracketing, comments etc.

Solution:

sync formatting between these two files to simplify further maintenance

Re: RR(S): JDK-8022616 u4 should not be used as a type for thread_id

[Yumin Qi]

Looks good.

On 9/24/2013 12:20 PM, Dmitry Samersoff wrote:

Staffan and Yumin,

Thank you for feedback.

Fixed (in place, please press shift-reload)

http://cr.openjdk.java.net/~dsamersoff/JDK-8022616/webrev.02/

-Dmitry

On 2013-09-24 11:57, Dmitry Samersoff wrote:

Please review changes:

http://cr.openjdk.java.net/~dsamersoff/JDK-8022616/webrev.02/

Story:

Tracing framework expect u4 as an id of thread

pthread_t chosen as a tread id for variety of BSD platforms couldn't be
converted to u4 so it cause compilation failure on BSD x64

Solution:

Change thread_id to pid_t and get this id  directly from kernel, the
same manner as Linux code does. Mac Os X still uses mach_port instead of
thread id.

Tested on FreeBSD and OpenBSD and also code passed jprt.

-Dmitry

Re: RR(S): JDK-8022616 u4 should not be used as a type for thread_id

[Yumin Qi]

Dmitry,

  Looks good. Minor comments:
 os_bsd.cpp:

+ #elif __APPLE__ //XNU kernel
+   // despite the fact mach port is actually not a thread id use it
+   // instead of syscall(SYS_thread_selfid) as it certenly fit to u4

Is it  a typo for 'certainly' here?
certenly => certainly
fit => fits

Thanks
Yumin

On 9/24/2013 12:57 AM, Dmitry Samersoff wrote:

Please review changes:

http://cr.openjdk.java.net/~dsamersoff/JDK-8022616/webrev.02/

Story:

Tracing framework expect u4 as an id of thread

pthread_t chosen as a tread id for variety of BSD platforms couldn't be
converted to u4 so it cause compilation failure on BSD x64

Solution:

Change thread_id to pid_t and get this id  directly from kernel, the
same manner as Linux code does. Mac Os X still uses mach_port instead of
thread id.

Tested on FreeBSD and OpenBSD and also code passed jprt.

-Dmitry

hg: hsx/hotspot-rt/hotspot: 2 new changesets

[yumin . qi]

Changeset: 621eda7235d2
Author:minqi
Date:  2013-09-16 15:35 -0700
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/621eda7235d2

7164841: Improvements to the GC log file rotation
Summary: made changes to easily identify current log file in rotation. 
Parameterize the input with %t for time replacement in file name.
Reviewed-by: ccheung, tschatzl, tamao, zgu
Contributed-by: yumin...@oracle.com

! src/share/vm/prims/jni.cpp
! src/share/vm/runtime/arguments.cpp
! src/share/vm/utilities/ostream.cpp
! src/share/vm/utilities/ostream.hpp

Changeset: 535973ddf22c
Author:minqi
Date:  2013-09-16 18:39 -0700
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/535973ddf22c

Merge

Re: RFR: 7164841: Improvements to the GC log file rotation

[Yumin Qi]

Hi, loise

  Thanks for your review. I will send out a new webrev soon.
  For your concern, see embedded answers.


arguments.cpp - looks good, no comments
I will add another functions, is_filename_valid, and in this functions, 
check if the given filename is 'legal' function name, currently we did 
not check for this.


ostream.hpp - looks good, no comments

ostream.cpp -

- Have you tried a file name that is 255 characters?  It would 
seem that after you appended the pid + timestamp + .current + # you 
could overrun this buffer?


439 #define FILENAMEBUFLEN 256
and subsequent use at
466 char tempbuf[FILENAMEBUFLEN];
467 jio_snprintf(tempbuf, sizeof(tempbuf), "%s.%d"
CURRENTAPPX, _file_name, _cur_file_num);

- I would also like to point out in line #467, there may not be a 
need for "sizeof(tempbuf)", isn't it just FILENAMEBUFLEN?
  Please check the use of "sizeof()" in the jio_sprintf 
statements, I think all are known.
The FILENAMEBUFLEN will change to 1024 which is suffice for most of use 
cases.
for using sizeof(name of char[]), this way can save time for case that  
FILENAMEBUFLEN changed. Certainly, usually we don't change that.


   - Related to my first comment.  If you have a time_msg that is 
FILENAMEBUFLEN and you try to concatenate it with a file_name that is 
FILENAMEBUFLEN + the

 os::local_time_string, you've overrun your buffer.

493 char time_msg[FILENAMEBUFLEN];
 494 char time_str[EXTRACHARLEN];
 495 char current_file_name[FILENAMEBUFLEN];
496 char renamed_file_name[FILENAMEBUFLEN];
 ...
 530 jio_snprintf(time_msg, sizeof(time_msg), "%s GC log file
has reached the"
 531 " maximum size.
Saved as %s\n",
 532 os::local_time_string((char *)time_str, sizeof(time_str)),
 533 renamed_file_name);


As above mentioned, now the buffer size is 1024 bytes.


- Line #538 dealing with the rename of .current.#.  I 
don't prefer the use of .current. Take for example a user
  specified on the command line -XX:NumberOfGCLogFiles=5, but 
there is enough -Xloggc info generated to fit in 7 files.  This
  situation will cause the log file rotation to rotate back on 
itself.  So, file #0 will be reopened and used as the 6th file, and
  then the rotation will progress and finish dumping Xloggc 
information into the last file which would be .current.1,

  correct?  So a user would be left with the following files.

file_name.0 (which now has a later timestamp 
in its name than file # 1 thru 4)

file_name.1
file_name.2
file_name.3
file_name.4
file_name.current.1

  I find this confusing, would you consider having the -Xloggc 
information be dumped into the current #'ed file directly?



The current design is like this:
If rotate in same file, that is the file given by -Xloggc:, 
the file name will be extended according to '%p" and '%t'.
If rotate in multiple files, user need to quickly identify which one is 
the current file, so this is why I append .current to the file name.

For example, if -XX:NumberOfGCLogFiles=5,  it will be like:
file_name.0
file_name.1.current
file_name.2
file_name.3
file_name.4

The oldest file is file_name.1 which is erased when new file 
file_name.1.current created. The current logging file is the one with 
.current appended so it is easily to tell. If without this appendix, 
user only sees bunch of log files and not easy to spot which one is current.



- Line 587 FLAG_SET_DEFAULT(UseGCLogFileRotation, false);
  I like that you implemented the idea to turn off GC log file 
rotation and continue with the current file if you can't open the next 
file, thank you.


That turned rotation off --- if the current file can grow, it will grow, 
just like no rotation case; if it can not grow, VM may or may not 
continue which depends on what happens.


Thanks
Yumin

Thank you,
Lois

On 8/27/2013 11:32 PM, Yumin Qi wrote:

Hi,

 Based on the discussion, I updated the webrev, and in this version
1) deleted unused rotatingFileStream  constructor, which keep code 
shorter.

2) removed reformat_filename in previous version.
3) still keep original design, that if no rotation, just use file 
name given by -Xloggc:.


Others are basically same.

Please take a look and comment.

http://cr.openjdk.java.net/~minqi/7164841/webrev02 
<http://cr.openjdk.java.net/%7Eminqi/7164841/webrev02>


Thanks
Yumin

On 8/27/2013 10:13 AM, Tao Mao wrote:



On 8/27/13 1:01 AM, Bengt Rutisson wrote:


Yumin,

On 8/26/13 7:01 PM, Yumin Qi wrote:

Bengt,

  Thanks for your comments.
  Yes, as you said, the purpose of rotating logs is primarily 
targeted for saving disk space. This bu

Re: RFR: 7164841: Improvements to the GC log file rotation

[Yumin Qi]

Hi,

 Based on the discussion, I updated the webrev, and in this version
1) deleted unused rotatingFileStream  constructor, which keep code shorter.
2) removed reformat_filename in previous version.
3) still keep original design, that if no rotation, just use file name 
given by -Xloggc:.


Others are basically same.

Please take a look and comment.

http://cr.openjdk.java.net/~minqi/7164841/webrev02 
<http://cr.openjdk.java.net/%7Eminqi/7164841/webrev02>


Thanks
Yumin

On 8/27/2013 10:13 AM, Tao Mao wrote:



On 8/27/13 1:01 AM, Bengt Rutisson wrote:


Yumin,

On 8/26/13 7:01 PM, Yumin Qi wrote:

Bengt,

  Thanks for your comments.
  Yes, as you said, the purpose of rotating logs is primarily 
targeted for saving disk space. This bug is supplying customers 
another option to prevent the previous gc logs from removed by 
restart app without making copy. Now with this pid and time stamp 
included in file name,  we have more options for users. It is up to 
user to make decision to or not to keep the logs. We cannot handle 
all the requests in JVM, but we can offer the choices for users I 
think. Any way, with either the previous rotating version, or the 
one I am working, the logs will not grow constantly without limit to 
blow storage out as long as users give enough attention.


  My concern is for no log rotation, should we still use time stamp 
in file name? I have one version for this, I hope get more comments 
for that.


Sorry if I wasn't clear before. I am not worried about the case when 
log rotation is turned on. What I was worried about was the case 
where a user is not using log rotation but is still piping the GC 
output to a file. That file will be overwritten every time the VM is 
restarted. If we add time stamps to the file name for this case then 
the file will not be overwritten. I think that is a bit of a scary 
change. That's all.


If you are worried about the case where a user is not using log 
rotation but creating a new file for each restart, you should be 
almost equivalently worried about the case where a user is using log 
rotation and having many rotated logs created which are different for 
each VM restart. Basically, we are creating even more files over time, 
which falls into your concern.


At this point, I'm fine with either choice for they have pros and 
cons. If we always append date and time to log file names, customers 
may say "the logs are 'eating' my disk"; if you don't do that, the 
customers would still complain the log is overwritten after a restart 
(I saw these kinds of CR's twice).


Thanks.
Tao



Bengt



  More comments are appreciated by sending to more wide audience, 
especially sustaining, they have more experience with customer request.


Thanks
Yumin



On 8/26/2013 4:47 AM, Bengt Rutisson wrote:


Hi Yumin and Tao,

I have not reviewed the code change but I have a comment inlined 
below.


On 8/24/13 1:05 AM, Yumin Qi wrote:

Tao,

  Thanks for your review.

On 8/23/2013 3:33 PM, Tao Mao wrote:

Hi,

I reviewed most of the code and test-ran a build from it. It's a 
very cool and important improvement.


Thank you for putting together these on our wishlist for long.

Below are some comments.

1. src/share/vm/runtime/arguments.cpp

- 1853 "To enable GC log rotation, use -Xloggc: 
-XX:+UseGCLogFileRotation -XX:NumberOfGCLogFiles= 
-XX:GCLogFileSize=[k|K|m|M]\n"
+ 1853 "To enable GC log rotation, use -Xloggc: 
-XX:+UseGCLogFileRotation -XX:NumberOfGCLogFiles= 
-XX:GCLogFileSize=[k|K|m|M|g|G]\n"


Please consider adding [g|G] to GCLogFileSize suggestion.

I worked on a problem of enabling gc log limit over 2G 
(JDK-712). So it seems that customers sometimes want gc logs 
to be very large.



Sure, will add.

2. src/share/vm/runtime/arguments.hpp

(1) with the current changeset, we only append date&time to 
file_name w/ +UseGCLogFileRotation; if not, we won't have 
date&time info.


I think it would be useful to let both cases (w/ and w/o 
UseGCLogFileRotation) have date&time in order to solve the 
overwritten problem (e.g. JDK-8020667). In fact, having that, we 
actually solve JDK-8020667.


If you want to have that, basically you need to work on the 
FileStream constructor methods fileStream().


I can change that, if no objection from others. This also will 
simplify the setting of file name here.


I think appending a timestamp to the log file name is a nice idea, 
but I think it is also a bit scary. There are users who restart 
their applications regularly. With the suggested idea such users 
will now risk filling up their disk space with log files. I imagine 
that that will not be appreciated by everyone. Such a change should 
probably be discussed more thoroughly than just in a review request.


Thanks,
Bengt




(2) Would it be better to rename method name reformat_filename() 
to extend_filename()?


(3) Typos and suggestion
537 // rotate file in name

Re: RFR: 7164841: Improvements to the GC log file rotation

[Yumin Qi]

Hi, Bernd

On 8/27/2013 10:27 AM, Bernd Eckenfels wrote:

Hello,

no matter what you decide (for example configurable pattern like J9 is 
doing it) please add enough header informatiomn to the logfile that it 
answers common gc diagnostic questions (version, command line 
settings, ram/swap size) and the start wallclock of the segment (for 
calculating abolute times without using the datestamp option).


The log file will log what ever the gc logging is currently writing to 
the file. In this fix, there is no changes to such information. The only 
additional information added to the log file is that at rotation moment, 
file name and create time information is logged to head of file for new 
file, like:


2013-08-27 12:04:13 GC log file created test-pid27685-2013-08-27_12-02-15.1
117.334: [GC (Allocation Failure) 117.334: [ParNew: 17305K->2K(19648K), 
0.0040920 secs] 98474K->81171K(182528K), 0.0044070 secs] [Times: 
user=0.01 sys=0.00, real=0.01 secs]
117.342: [GC (Allocation Failure) 117.342: [ParNew: 17305K->2K(19648K), 
0.0038690 secs] 98474K->81171K(182528K), 0.0041920 secs] [Times: 
user=0.01 sys=0.00, real=0.00 secs]


You can see that at the beginning, wall clock of the file create time 
and file name are logged.


For tail:

117.318: [GC (Allocation Failure) 117.318: [ParNew: 17305K->2K(19648K), 
0.0038870 secs] 98474K->81171K(182528K), 0.0042070 secs] [Times: 
user=0.01 sys=0.00, real=0.00 secs]
117.326: [GC (Allocation Failure) 117.326: [ParNew: 17305K->2K(19648K), 
0.0040940 secs] 98474K->81171K(182528K), 0.0044230 secs] [Times: 
user=0.01 sys=0.00, real=0.01 secs]
2013-08-27 12:04:13 GC log file has reached the maximum size. Saved as 
test-pid27685-2013-08-27_12-02-15.0


This is previous log file.

Thanks
Yumin


Greetings
Bernd

hg: hsx/hotspot-rt/hotspot: 2 new changesets

[yumin . qi]

Changeset: 7e7dd25666da
Author:ccheung
Date:  2013-08-26 14:11 -0700
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/7e7dd25666da

8020675: invalid jar file in the bootclasspath could lead to jvm fatal error
Summary: removed offending EXCEPTION_MARK calls and code cleanup
Reviewed-by: dholmes, iklam, coleenp, mseledtsov

! src/share/vm/classfile/classLoader.cpp
! src/share/vm/classfile/classLoader.hpp
+ test/runtime/LoadClass/LoadClassNegative.java
+ test/runtime/LoadClass/TestForName.java
+ test/runtime/LoadClass/dummy.jar

Changeset: 5351fe805c12
Author:minqi
Date:  2013-08-27 07:54 -0700
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/5351fe805c12

Merge

Re: RFR: 7164841: Improvements to the GC log file rotation

[Yumin Qi]

Bengt,

  Thanks for your comments.
  Yes, as you said, the purpose of rotating logs is primarily targeted 
for saving disk space. This bug is supplying  customers another option 
to prevent the previous gc logs from removed by restart app without 
making copy. Now with this pid and time stamp included in file name,  we 
have more options for users. It is up to user to make decision to or not 
to keep the logs. We cannot handle all the requests in JVM, but we can 
offer the choices for users I think. Any way, with either the previous 
rotating version, or the one I am working, the logs will not grow 
constantly without limit to blow storage out as long as users give 
enough attention.


  My concern is for no log rotation, should we still use time stamp in 
file name? I have one version for this, I hope get more comments for that.


  More comments are appreciated by sending to more wide audience, 
especially sustaining, they have more experience with customer request.


Thanks
Yumin



On 8/26/2013 4:47 AM, Bengt Rutisson wrote:


Hi Yumin and Tao,

I have not reviewed the code change but I have a comment inlined below.

On 8/24/13 1:05 AM, Yumin Qi wrote:

Tao,

  Thanks for your review.

On 8/23/2013 3:33 PM, Tao Mao wrote:

Hi,

I reviewed most of the code and test-ran a build from it. It's a 
very cool and important improvement.


Thank you for putting together these on our wishlist for long.

Below are some comments.

1. src/share/vm/runtime/arguments.cpp

- 1853 "To enable GC log rotation, use -Xloggc: 
-XX:+UseGCLogFileRotation -XX:NumberOfGCLogFiles= 
-XX:GCLogFileSize=[k|K|m|M]\n"
+ 1853 "To enable GC log rotation, use -Xloggc: 
-XX:+UseGCLogFileRotation -XX:NumberOfGCLogFiles= 
-XX:GCLogFileSize=[k|K|m|M|g|G]\n"


Please consider adding [g|G] to GCLogFileSize suggestion.

I worked on a problem of enabling gc log limit over 2G 
(JDK-712). So it seems that customers sometimes want gc logs to 
be very large.



Sure, will add.

2. src/share/vm/runtime/arguments.hpp

(1) with the current changeset, we only append date&time to 
file_name w/ +UseGCLogFileRotation; if not, we won't have date&time 
info.


I think it would be useful to let both cases (w/ and w/o 
UseGCLogFileRotation) have date&time in order to solve the 
overwritten problem (e.g. JDK-8020667). In fact, having that, we 
actually solve JDK-8020667.


If you want to have that, basically you need to work on the 
FileStream constructor methods fileStream().


I can change that, if no objection from others. This also will 
simplify the setting of file name here.


I think appending a timestamp to the log file name is a nice idea, but 
I think it is also a bit scary. There are users who restart their 
applications regularly. With the suggested idea such users will now 
risk filling up their disk space with log files. I imagine that that 
will not be appreciated by everyone. Such a change should probably be 
discussed more thoroughly than just in a review request.


Thanks,
Bengt




(2) Would it be better to rename method name reformat_filename() to 
extend_filename()?


(3) Typos and suggestion
537 // rotate file in names filename.0, filename.1, ..., 
filename.

*=> extended_filename.0*

538 // filename contains pid and time when the fist file created. 
The current filename is

*=> *the*first *file created.

539 // gc_log_file_name + pid + 
-MM-DD_HH-MM-SS..current, where i is current
540 // rotation file number. After it reaches max file size, the 
file will be saved and

541 // renamed with .current removed from its tail.


Will change that.
3. For your point 5), I don't quite get it. In my test-run, I tried 
to change file permission to unreadable and unwritable, but VM would 
later change the permission back anyway.


So could you bring up some use cases of that functionality to give 
more details?


Changing file permission will not stop the file create, in this 
rotation, the file opened/saved/removed/renamed -> then repeat. What 
I have done is change the while directory to read only, then the 
create failed so rotation stopped.


4. Will you write jtreg tests for this functionality? It looks 
possible to write some tests, at least checking the format of log 
names.



Good suggestion, I will add one.


Thanks.
Tao

On 8/23/13 7:53 AM, Yumin Qi wrote:
Could I get  a GC staff review the change? Need more reviewers to 
push this in.


Thanks
Yumin

On 8/21/2013 3:43 PM, Yumin Qi wrote:

Hi, all

  This is second version after feedback from first round.
  The changes are:

  1) file name will be based on gc log file name 
(-Xloggc:filename), pid (process id) and time when the first 
rotation file created:

   -pid--MM-DD_HH-MM-SS
  2) If rotate in same file, file name is as in 1), if rotate in 
multiple files, . will append to above file name.
  3) every time file rotated, file name and time when file created 
will be logged to head/tail, this is same as first ve

Re: RFR (XS) 8023406 - [windows] build_vm_def.sh takes too long even when BUILD_WIN_SA != 1

[Yumin Qi]

Ioi,

  One question, SKIP_GENERATED, is this a environment variable or need 
to give on command?

  Others looks OK.

Thanks
Yumin


On 8/20/2013 2:11 PM, Ioi Lam wrote:

|Please review a very small fix:||
||
||http://cr.openjdk.java.net/~iklam/8023406/windows_build_vm_def_slow_001/||
||
||Bug: make/windows/build_vm_def.sh takes too long even when 
BUILD_WIN_SA != 1||

||
||http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8023406||
||https://jbs.oracle.com/bugs/browse/JDK-8023406||
||
||Summary of fix:||
||
||Reduce Windows build time to improve developer productivity.||
||
||If BUILD_WIN_SA != 1, don't bother to generate vm.def, whose sole ||
||purpose is for SA to determine the type information of C++ 
objects.||

||
||Instead, this rather eye-catching warning is printed, and 10 
seconds||

||are saved in the build cycle.||
||
||***||
||*** Not building SA: BUILD_WIN_SA != 1||
||*** C++ Vtables NOT included in vm.def||
||*** This jvm.dll will NOT work properly with SA.||
||***||
||*** When in doubt, set BUILD_WIN_SA=1, clean and rebuild.||
||***||
||
||This does not affect JPRT -- JPRT always sets BUILD_WIN_SA=1.||
||
||Result: ||
||
||Touch 1 .cpp file; rebuild: Total time is reduced 15 seconds -> 
5 seconds.||

||
||Tests:||
||
[0] Manual testing with both create.bat (IDE build) and build.bat
VS 2008 + VS2010
||[1] JPRT (windows.* only)||
||[2] I built a jvm.dll without BUILD_WIN_SA=0, and it ran 
Eclipsed without||

||any problem.||
||
||Thanks||
||- Ioi| 

hg: hsx/hotspot-rt/hotspot: 8023188: Unsafe volatile double store on bsd is broken

[yumin . qi]

Changeset: b1fd869e7df0
Author:minqi
Date:  2013-08-19 09:16 -0700
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/b1fd869e7df0

8023188: Unsafe volatile double store on bsd is broken
Reviewed-by: dcubed, dholmes
Contributed-by: yumin...@oracle.com

! src/os_cpu/bsd_x86/vm/orderAccess_bsd_x86.inline.hpp

Re: RFR: 8020962: dump loaded java classes when vm crash

[Yumin Qi]

To summarize the discussion of this change:

1) Launching SA process at the last stage of error handling caused much 
concern;
2) SA is not stable and need stabilization in future enhancement, which 
looks like a long term task;
3) With SA and available core file, we can get jar files for aftermath 
analysis but you need to become familiar with SA;


So I will closed this bug as 'no fix'.
BTW, when working on this bug, filed other two bugs which should be 
fixed to dump classes.


Thanks for the inputs from all of you.

Yumin

On 8/13/2013 3:57 AM, Staffan Larsen wrote:

I agree with those skeptical to this change. The correct approach here is to 
make sure we get core files when crashes occur and then process that core with 
SA. And I agree that SA needs to be stabilized and could use a lot more testing.

/Staffan

On 13 aug 2013, at 07:17, Christian Tornqvist  
wrote:


Hi Chris,


The SA logic would be exercised during our internal testing and could be

fixed before the release.  Right now the situation is we see a >customer
crash, we want to run the SA and notice that it's broken.  That's too late.

This is an issue with our current testing of the SA. This should be
addressed by making sure our test works and covers the functionality of the
SA, not by making the JVM dependent on the SA.

Thanks,
Christian

-Original Message-
From: hotspot-runtime-dev-boun...@openjdk.java.net
[mailto:hotspot-runtime-dev-boun...@openjdk.java.net] On Behalf Of Christian
Thalinger
Sent: Monday, August 12, 2013 9:12 PM
To: Coleen Phillimore
Cc: serviceability-dev@openjdk.java.net serviceability-dev@openjdk.java.net;
'hotspot compiler'; hotspot-runtime-...@openjdk.java.net; 'David Holmes'
Subject: Re: RFR: 8020962: dump loaded java classes when vm crash


On Aug 12, 2013, at 2:43 PM, Coleen Phillimore
 wrote:


Yumin,

I don't think this change should be added to the JVM for the following

reasons.

This change is something that I (kind of) requested from Yumin since it
would help to reproduce crashes in the compilers.  Getting replay data after
the fact can be very tricky (given all the system library interactions or a
broken SA; see below).


1. Error handling should only contain safe actions.   We have concerns

that the SA is not that stable

.which is a problem, I agree.  To make it more stable we need to use it so
we can detect problems early.

Having the SA dump data when we crash in the compiler can be one of these
usages.  The SA logic would be exercised during our internal testing and
could be fixed before the release.  Right now the situation is we see a
customer crash, we want to run the SA and notice that it's broken.  That's
too late.


and would prevent getting a real core file in many error situations.   You

couldn't have tested all error situations because these are usually the
things we don't know about.   You also mention that there are currently bugs
preventing this feature from working in your first email.

2. I am not convinced that having a separate jar file with loaded classes

(is it a list that SA generates?) would be collected by an error reporter.
If it's collected, I don't see how helpful it would be.

As mentioned above, it's required to replay compilations.


Also a customer may not want their classes disclosed in error reporting.

They don't have to if they don't want to.

-- Chris


3.  This doesn't seem important enough to include as a new feature in jdk8

release, which is feature-complete.   I don't see a customer request for it.

Coleen

On 08/12/2013 01:00 PM, Yumin Qi wrote:

Chris, David

Yes. This can be after crash with core file, but some time no core file

generated (also if the error could not repeat, we will never got information
again), so dumping  target process is also a choice. To avoid more
confusion, this step was launched at the very last moment, just before raise
abort to exit. At this moment, if client process could not attach to the
target process it will exit right away.

Answers to David:

Note that the SA is only present in a full JDK, not a JRE (full or

compact profile).

Yes, in code, if no sa-jdi.jar found, skip fork.

- What mechanism will the SA try to use to query the VM?

After successfully attached to target process, SA will read via proc

APIs and vmStructs (named TYPEDB) to iterate  memory of system dictionary
read (only)  from target process to dump class information.

- What if the state of the crashed VM stops the SA from being able to

attach properly (ie both processes hang)?

That should be an attaching API problem. We haven't watched such case

happened so far.

- What if it the SA also crashes, will it launch a third VM then a fourth

etc?

Definitely don't want to see this happened in a chain. The solution
may use a property such as
sun.jvm.hotspot.DumpLoadedClasses.dumpingInProcess=true to pass into
SA process, at launc

Re: RFR: 8020962: dump loaded java classes when vm crash

[Yumin Qi]

Coleen,

  Chris Thalinger  already answered some of your concerns. For jar file 
which dumped in this change, compiler replay has added functions to SA 
to dump out replay jars against core file. Since customer is the one 
wants us to fix problems in VM, the disclosure of such jar to us should 
be OK I think, any way if we have core file, we already have the loaded 
classes.
  This should mainly be focused on dump jars as possible at the last 
stage of error reporter. I chose using SA since with c++ to implement 
same functionality I have to use zip file operation to do so, which is 
not a fit at such condition, but in SA the same operation ready for use. 
The only case we could not get jar files is no core file available, in 
such case, this change will supply an alternate.


  Looks this need more discussion for further decision.

Thanks
Yumin


On 8/12/2013 2:43 PM, Coleen Phillimore wrote:


Yumin,

I don't think this change should be added to the JVM for the following 
reasons.


1. Error handling should only contain safe actions.   We have concerns 
that the SA is not that stable and would prevent getting a real core 
file in many error situations.   You couldn't have tested all error 
situations because these are usually the things we don't know about.   
You also mention that there are currently bugs preventing this feature 
from working in your first email.


2. I am not convinced that having a separate jar file with loaded 
classes (is it a list that SA generates?) would be collected by an 
error reporter.   If it's collected, I don't see how helpful it would 
be.  Also a customer may not want their classes disclosed in error 
reporting.


3.  This doesn't seem important enough to include as a new feature in 
jdk8 release, which is feature-complete.   I don't see a customer 
request for it.


Coleen

On 08/12/2013 01:00 PM, Yumin Qi wrote:

Chris, David

  Yes. This can be after crash with core file, but some time no core 
file generated (also if the error could not repeat, we will never got 
information again), so dumping  target process is also a choice. To 
avoid more confusion, this step was launched at the very last moment, 
just before raise abort to exit. At this moment, if client process 
could not attach to the target process it will exit right away.


Answers to David:

Note that the SA is only present in a full JDK, not a JRE (full or 
compact profile).

  Yes, in code, if no sa-jdi.jar found, skip fork.

- What mechanism will the SA try to use to query the VM?

  After successfully attached to target process, SA will read via 
proc APIs and vmStructs (named TYPEDB) to iterate  memory of system 
dictionary read (only)  from target process to dump class information.


- What if the state of the crashed VM stops the SA from being able to 
attach properly (ie both processes hang)?


  That should be an attaching API problem. We haven't watched such 
case happened so far.


- What if it the SA also crashes, will it launch a third VM then a 
fourth etc?


  Definitely don't want to see this happened in a chain. The solution 
may use a property such as 
sun.jvm.hotspot.DumpLoadedClasses.dumpingInProcess=true to pass into 
SA process, at launching call, check if the property set, if set, do 
not fork. When SA process died, it will generate core file first, 
note the target process still waiting for its exit, so when target 
exit, the core file (if both use default core as name) will be 
override by target. The SA process will only leave a hs_err_pid*.log 
file. (? read such property in handler is possible?)


 Also what is the nature of this dump? How big is it? Where will it go?
 The jars includes *app.jar and *boot.jar, the later usually can be 
ignored (rt.jar etc system jars) unless it's rewritten by JVMTI 
agent. The app.jar will contain all classes by customer, so we can do 
whatever we can to the jar.



Thanks
Yumin


On 8/12/2013 5:51 AM, Christian Tornqvist wrote:

Hi Yumin,

The idea is to do as little as possible in the VM error handler, 
since we've crashed for some reason we don't know what state the 
process is in and we have to be extremely careful in when we're 
gathering the information. This seems like a step that is risky for 
all of the reasons David mentioned below.


It's also information that can easily be extracted post-mortem from 
the core/mdmp using the method you described for OSX, so gathering 
this at the time of a crash seems like an unnecessary risk.


Thanks,
Christian

-Original Message-
From: hotspot-compiler-dev-boun...@openjdk.java.net 
[mailto:hotspot-compiler-dev-boun...@openjdk.java.net] On Behalf Of 
David Holmes

Sent: Monday, August 12, 2013 1:56 AM
To: Yumin Qi
Cc: hotspot compiler; hotspot-runtime-...@openjdk.java.net
Subject: Re: RFR: 8020962: dump loaded java classes when vm crash

Hi Yumin,

Note that the SA is only present in a full JDK, not a JRE (full or 
compa

Re: RFR: 8020962: dump loaded java classes when vm crash

[Yumin Qi]

Ioi,

  Thanks for the reminding, it is possible. I will use env instead.

Yumin

On 8/12/2013 11:00 AM, Ioi Lam wrote:

On 08/12/2013 10:00 AM, Yumin Qi wrote:


- What if it the SA also crashes, will it launch a third VM then a 
fourth etc?


  Definitely don't want to see this happened in a chain. The solution 
may use a property such as 
sun.jvm.hotspot.DumpLoadedClasses.dumpingInProcess=true to pass into 
SA process, at launching call, check if the property set, if set, do 
not fork. When SA process died, it will generate core file first, 
note the target process still waiting for its exit, so when target 
exit, the core file (if both use default core as name) will be 
override by target. The SA process will only leave a hs_err_pid*.log 
file. (? read such property in handler is possible?)


There is still the chance that the VM may have crashed before the 
properties are initialized. Maybe use environment variables instead?


- Ioi

Re: RFR: 8020962: dump loaded java classes when vm crash

[Yumin Qi]

Chris, David

  Yes. This can be after crash with core file, but some time no core 
file generated (also if the error could not repeat, we will never got 
information again), so dumping  target process is also a choice. To 
avoid more confusion, this step was launched at the very last moment, 
just before raise abort to exit. At this moment, if client process could 
not attach to the target process it will exit right away.


Answers to David:

Note that the SA is only present in a full JDK, not a JRE (full or 
compact profile).

  Yes, in code, if no sa-jdi.jar found, skip fork.

- What mechanism will the SA try to use to query the VM?

  After successfully attached to target process, SA will read via proc 
APIs and vmStructs (named TYPEDB) to iterate  memory of system 
dictionary read (only)  from target process to dump class information.


- What if the state of the crashed VM stops the SA from being able to 
attach properly (ie both processes hang)?


  That should be an attaching API problem. We haven't watched such case 
happened so far.


- What if it the SA also crashes, will it launch a third VM then a 
fourth etc?


  Definitely don't want to see this happened in a chain. The solution 
may use a property such as 
sun.jvm.hotspot.DumpLoadedClasses.dumpingInProcess=true to pass into SA 
process, at launching call, check if the property set, if set, do not 
fork. When SA process died, it will generate core file first, note the 
target process still waiting for its exit, so when target exit, the core 
file (if both use default core as name) will be override by target. The 
SA process will only leave a hs_err_pid*.log file. (? read such property 
in handler is possible?)


 Also what is the nature of this dump? How big is it? Where will it go?
 The jars includes *app.jar and *boot.jar, the later usually can be 
ignored (rt.jar etc system jars) unless it's rewritten by JVMTI agent. 
The app.jar will contain all classes by customer, so we can do whatever 
we can to the jar.



Thanks
Yumin


On 8/12/2013 5:51 AM, Christian Tornqvist wrote:

Hi Yumin,

The idea is to do as little as possible in the VM error handler, since we've 
crashed for some reason we don't know what state the process is in and we have 
to be extremely careful in when we're gathering the information. This seems 
like a step that is risky for all of the reasons David mentioned below.

It's also information that can easily be extracted post-mortem from the 
core/mdmp using the method you described for OSX, so gathering this at the time 
of a crash seems like an unnecessary risk.

Thanks,
Christian

-Original Message-
From: hotspot-compiler-dev-boun...@openjdk.java.net 
[mailto:hotspot-compiler-dev-boun...@openjdk.java.net] On Behalf Of David Holmes
Sent: Monday, August 12, 2013 1:56 AM
To: Yumin Qi
Cc: hotspot compiler; hotspot-runtime-...@openjdk.java.net
Subject: Re: RFR: 8020962: dump loaded java classes when vm crash

Hi Yumin,

Note that the SA is only present in a full JDK, not a JRE (full or compact 
profile).

I have quite a few concerns with this:

We already do a lot of things that are not valid to be done from a signal 
handling context but this really takes that to an extreme. Doing fork-exec from 
a signal handler seems like a recipe for disaster (Note the existing onError 
facility is typically used for synchronous failures.)

The idea of launching a second VM to try and query a VM that has crashed also 
seems somewhat problematic:
- What mechanism will the SA try to use to query the VM?
- What if the state of the crashed VM stops the SA from being able to attach 
properly (ie both processes hang)?
- What if it the SA also crashes, will it launch a third VM then a fourth etc?

Also what is the nature of this dump? How big is it? Where will it go?

Thanks,
David

On 12/08/2013 9:36 AM, Yumin Qi wrote:

Hi, all

I would like to have your review for

http://cr.openjdk.java.net/~minqi/8020962/webrev0/
<http://cr.openjdk.java.net/%7Eminqi/8020962/webrev0/>

Description: When JVM crashed, we also want to check the application
class files especially we got core file from customers.  The aftermath
analysis will benefit from all loaded java classes available. In this
change, spawn another process running SA to do the job when JVM
crashes, this way also avoid further messing up with the error report
which already in signal handler.

Note: The test has done with following two bugs worked around:
8022655: ClassDump ignored jarStream setting (This will be fixed
and integrated by Kevin Walls soon)
8011888: sa.js: TypeError: [object JSAdapter] has no such function
"__has__" (Not know when it will be integrated)
That is, without those two fixed, the jars of loaded classes will
not be successfully dumped.
Also, on MacOS it requires security access permission to attach to
another process, so omit doing so. To get loaded jar file s, with core
file available (on 

Re: RFR: 8020962: dump loaded java classes when vm crash

[Yumin Qi]

Sorry forgot to include svc.

Yumin

On 8/11/2013 4:36 PM, Yumin Qi wrote:

Hi, all

  I would like to have your review for

http://cr.openjdk.java.net/~minqi/8020962/webrev0/ 
<http://cr.openjdk.java.net/%7Eminqi/8020962/webrev0/>


Description: When JVM crashed, we also want to check the application 
class files especially we got core file from customers.  The aftermath 
analysis will benefit from all loaded java classes available. In this 
change, spawn another process running SA to do the job when JVM 
crashes, this way also avoid further messing up with the error report 
which already in

signal handler.

Note: The test has done with following two bugs worked around:
  8022655: ClassDump ignored jarStream setting (This will be fixed and 
integrated by Kevin Walls soon)
  8011888: sa.js: TypeError: [object JSAdapter] has no such function 
"__has__" (Not know when it will be integrated)
  That is, without those two fixed, the jars of loaded classes will 
not be successfully dumped.
  Also, on MacOS it requires security access permission to attach to 
another process, so omit doing so. To get loaded jar file
s, with core file available (on all platforms), one can (only after 
this change) do


  $JAVA_HOME/bin/java [-d64] -cp $JAVA_HOME/lib/sa-jdi.jar 
sun.jvm.hotspot.DumpLoadedClasses $JAVA_HOME/bin/java corefile



Thanks
Yumin

Re: RR(S): 8022655: ClassDump ignored jarStream setting

[Yumin Qi]

Looks good. Thanks for fixing it.

Yumin

On 8/9/2013 7:54 AM, Kevin Walls wrote:

Hi,

Review request for a small change in ClassDump:

http://cr.openjdk.java.net/~kevinw/8022655/webrev.00/

This was caused by moving some code from ClassDump's main method, 
breaking the intent that it has to maintain either an output director, 
OR a jarStream.  Setting of the directory could be done 
unconditionally when it was in main(), but not when in the run() method.


The tests test/compiler/ciReplay for compiler replay work (if you 
update sa.js with a suggested fix from 8011888).


Trying those tests now on Solaris, I found coreadm settings may mean 
they don't find the core: putting  a coreadm command in the test means 
we should find JTwork/scratch/core


Thanks
Kevin



On 08/08/2013 19:25, Yumin Qi wrote:

Kevin,

https://jbs.oracle.com/bugs/browse/JDK-8022655

I assigned this to you since it is caused by 8011888 fix.

Thanks
Yumin

On 7/26/2013 2:20 AM, Kevin Walls wrote:


Hi Yumin,

Sure no problem I'll look into it.   Looks like in ClassDump we 
should check classFilter != null before doing that initialisation in 
the run() method.


At the moment trying jtreg on test/compiler/ciReplay I get a failure 
in TestSA.sh, as it calls sun.jvm.hotspot.CLHSDB, and gives the 
javascript exceptions characteristic of 8011888. Rebuilding with the 
suggested new sa.js I see in that bug, I'll test and let you know...


Thanks!
Kevin



On 26/07/13 02:17, Yumin Qi wrote:

Hi, Kevin

http://cr.openjdk.java.net/~kevinw/8010278/webrev.03/ 
<http://cr.openjdk.java.net/%7Ekevinw/8010278/webrev.03/>

  I found this fix breaks C2 replay for dumping loaded java classes:

at 
sun.jvm.hotspot.tools.jcore.PackageNameFilter.canInclude(PackageNameFilter.java:55)
at 
sun.jvm.hotspot.tools.jcore.ClassDump.dumpKlass(ClassDump.java:135)
at 
sun.jvm.hotspot.tools.jcore.ClassDump.access$000(ClassDump.java:38)
at 
sun.jvm.hotspot.tools.jcore.ClassDump$1.visit(ClassDump.java:106)
at 
sun.jvm.hotspot.memory.Dictionary.classesDo(Dictionary.java:68)
at 
sun.jvm.hotspot.memory.SystemDictionary.classesDo(SystemDictionary.java:190)
at 
sun.jvm.hotspot.tools.jcore.ClassDump.run(ClassDump.java:102)
at 
sun.jvm.hotspot.DumpLoadedClasses.run(DumpLoadedClasses.java:65)
at 
sun.jvm.hotspot.DumpLoadedClasses.main(DumpLoadedClasses.java:43)


  The problem is with Replay, we set BootFilter and NonBootFilter 
for ClassDump, but this changeset ignored the existing filter.

  I filed a bug against this problem.

https://jbs.oracle.com/bugs/browse/JDK-8021444

Thanks
Yumin

hg: hsx/hotspot-rt/hotspot: 2 new changesets

[yumin . qi]

Changeset: 57ac7245594c
Author:minqi
Date:  2013-08-08 15:19 -0700
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/57ac7245594c

8019583: [TESTBUG] runtime/7107135 always passes
Summary: If java test return none zero, the value will be override by 'if' 
statement, the exit value will always '0' and pass. Fix by recording the result 
in a variable.
Reviewed-by: coleenp, dholmes, iklam
Contributed-by: yumin...@oracle.com

! test/runtime/7107135/Test7107135.sh

Changeset: 6222a021d582
Author:minqi
Date:  2013-08-08 20:13 -0700
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/6222a021d582

Merge

Re: About bug fix of 8010278

[Yumin Qi]

Kevin,

https://jbs.oracle.com/bugs/browse/JDK-8022655

I assigned this to you since it is caused by 8011888 fix.

Thanks
Yumin

On 7/26/2013 2:20 AM, Kevin Walls wrote:


Hi Yumin,

Sure no problem I'll look into it.   Looks like in ClassDump we should 
check classFilter != null before doing that initialisation in the 
run() method.


At the moment trying jtreg on test/compiler/ciReplay I get a failure 
in TestSA.sh, as it calls sun.jvm.hotspot.CLHSDB, and gives the 
javascript exceptions characteristic of 8011888. Rebuilding with the 
suggested new sa.js I see in that bug, I'll test and let you know...


Thanks!
Kevin



On 26/07/13 02:17, Yumin Qi wrote:

Hi, Kevin

http://cr.openjdk.java.net/~kevinw/8010278/webrev.03/ 
<http://cr.openjdk.java.net/%7Ekevinw/8010278/webrev.03/>

  I found this fix breaks C2 replay for dumping loaded java classes:

at 
sun.jvm.hotspot.tools.jcore.PackageNameFilter.canInclude(PackageNameFilter.java:55)
at 
sun.jvm.hotspot.tools.jcore.ClassDump.dumpKlass(ClassDump.java:135)
at 
sun.jvm.hotspot.tools.jcore.ClassDump.access$000(ClassDump.java:38)
at 
sun.jvm.hotspot.tools.jcore.ClassDump$1.visit(ClassDump.java:106)
at 
sun.jvm.hotspot.memory.Dictionary.classesDo(Dictionary.java:68)
at 
sun.jvm.hotspot.memory.SystemDictionary.classesDo(SystemDictionary.java:190)

at sun.jvm.hotspot.tools.jcore.ClassDump.run(ClassDump.java:102)
at 
sun.jvm.hotspot.DumpLoadedClasses.run(DumpLoadedClasses.java:65)
at 
sun.jvm.hotspot.DumpLoadedClasses.main(DumpLoadedClasses.java:43)


  The problem is with Replay, we set BootFilter and NonBootFilter for 
ClassDump, but this changeset ignored the existing filter.

  I filed a bug against this problem.

https://jbs.oracle.com/bugs/browse/JDK-8021444

Thanks
Yumin

hg: hsx/hotspot-rt/hotspot: 8021296: [TESTBUG] Test8017498.sh fails to find "gcc" and fails to compile on some Linux releases

[yumin . qi]

Changeset: f9ee986a9fea
Author:ccheung
Date:  2013-07-30 14:14 -0700
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/f9ee986a9fea

8021296: [TESTBUG] Test8017498.sh fails to find "gcc" and fails to compile on 
some Linux releases
Summary: Added checking for gcc and simplified the sig_handler() in the test 
case
Reviewed-by: dcubed, coleenp, minqi, dlong

! test/runtime/6929067/Test6929067.sh
! test/runtime/7107135/Test7107135.sh
! test/runtime/jsig/Test8017498.sh
! test/runtime/jsig/TestJNI.c

Re: RFR(S): 8021444: SA: ClassDump.run() should not ignore existing ClassFilter.

[Yumin Qi]

Looks good! Thanks for fixing it.

Yumin

On 7/26/2013 3:06 AM, Kevin Walls wrote:


Hi again,

That seemed to go OK.  I retitled the bug:

8021444: SA: ClassDump.run() should not ignore existing ClassFilter.

https://jbs.oracle.com/bugs/browse/JDK-8021444
http://bugs.sun.com/view_bug.do?bug_id=8021444 (in due time, I expect...)

webrev:
http://cr.openjdk.java.net/~kevinw/8021444/webrev.00/

For Compiler Replay, CLHSDB uses the ClassDump class to extract 
classes, and specifically sets its ClassFilter.  That setting was 
being ignored, and the ClassFilter was re-initialized.



Thanks
Kevin



On 26/07/13 10:20, Kevin Walls wrote:


Hi Yumin,

Sure no problem I'll look into it.   Looks like in ClassDump we 
should check classFilter != null before doing that initialisation in 
the run() method.


At the moment trying jtreg on test/compiler/ciReplay I get a failure 
in TestSA.sh, as it calls sun.jvm.hotspot.CLHSDB, and gives the 
javascript exceptions characteristic of 8011888. Rebuilding with the 
suggested new sa.js I see in that bug, I'll test and let you know...


Thanks!
Kevin



On 26/07/13 02:17, Yumin Qi wrote:

Hi, Kevin

http://cr.openjdk.java.net/~kevinw/8010278/webrev.03/ 
<http://cr.openjdk.java.net/%7Ekevinw/8010278/webrev.03/>

  I found this fix breaks C2 replay for dumping loaded java classes:

at 
sun.jvm.hotspot.tools.jcore.PackageNameFilter.canInclude(PackageNameFilter.java:55)
at 
sun.jvm.hotspot.tools.jcore.ClassDump.dumpKlass(ClassDump.java:135)
at 
sun.jvm.hotspot.tools.jcore.ClassDump.access$000(ClassDump.java:38)
at 
sun.jvm.hotspot.tools.jcore.ClassDump$1.visit(ClassDump.java:106)
at 
sun.jvm.hotspot.memory.Dictionary.classesDo(Dictionary.java:68)
at 
sun.jvm.hotspot.memory.SystemDictionary.classesDo(SystemDictionary.java:190)
at 
sun.jvm.hotspot.tools.jcore.ClassDump.run(ClassDump.java:102)
at 
sun.jvm.hotspot.DumpLoadedClasses.run(DumpLoadedClasses.java:65)
at 
sun.jvm.hotspot.DumpLoadedClasses.main(DumpLoadedClasses.java:43)


  The problem is with Replay, we set BootFilter and NonBootFilter 
for ClassDump, but this changeset ignored the existing filter.

  I filed a bug against this problem.

https://jbs.oracle.com/bugs/browse/JDK-8021444

Thanks
Yumin

About bug fix of 8010278

[Yumin Qi]

Hi, Kevin

http://cr.openjdk.java.net/~kevinw/8010278/webrev.03/ 


  I found this fix breaks C2 replay for dumping loaded java classes:

at 
sun.jvm.hotspot.tools.jcore.PackageNameFilter.canInclude(PackageNameFilter.java:55)
at 
sun.jvm.hotspot.tools.jcore.ClassDump.dumpKlass(ClassDump.java:135)
at 
sun.jvm.hotspot.tools.jcore.ClassDump.access$000(ClassDump.java:38)
at 
sun.jvm.hotspot.tools.jcore.ClassDump$1.visit(ClassDump.java:106)

at sun.jvm.hotspot.memory.Dictionary.classesDo(Dictionary.java:68)
at 
sun.jvm.hotspot.memory.SystemDictionary.classesDo(SystemDictionary.java:190)

at sun.jvm.hotspot.tools.jcore.ClassDump.run(ClassDump.java:102)
at sun.jvm.hotspot.DumpLoadedClasses.run(DumpLoadedClasses.java:65)
at 
sun.jvm.hotspot.DumpLoadedClasses.main(DumpLoadedClasses.java:43)


  The problem is with Replay, we set BootFilter and NonBootFilter for 
ClassDump, but this changeset ignored the existing filter.

  I filed a bug against this problem.

https://jbs.oracle.com/bugs/browse/JDK-8021444

Thanks
Yumin

hg: hsx/hotspot-rt/hotspot: 2 new changesets

[yumin . qi]

Changeset: 1b6395189726
Author:minqi
Date:  2013-07-19 14:43 -0700
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/1b6395189726

8012263: ciReplay: gracefully exit & report meaningful error when replay data 
parsing fails
Summary: find_method could return NULL so need explicitly check if there is 
error after parse_method, exit on error to avoid crash.
Reviewed-by: kvn, twisti
Contributed-by: yumin...@oracle.com

! src/share/vm/ci/ciReplay.cpp

Changeset: 5ad7f8179bf7
Author:minqi
Date:  2013-07-24 08:04 +0200
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/5ad7f8179bf7

Merge

hg: hsx/hotspot-rt/hotspot: 3 new changesets

[yumin . qi]

Changeset: 72727c4b6dec
Author:ccheung
Date:  2013-07-19 14:54 -0700
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/72727c4b6dec

8020791: [TESTBUG] runtime/jsig/Test8017498.sh failed to compile native code
Summary: Added -DLINUX to the gcc command and improved the .sh script
Reviewed-by: dcubed, dholmes, minqi

! test/runtime/jsig/Test8017498.sh
! test/runtime/jsig/TestJNI.c

Changeset: 5165d659cebd
Author:minqi
Date:  2013-07-22 22:21 -0700
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/5165d659cebd

Merge


Changeset: c0f353803b47
Author:minqi
Date:  2013-07-23 12:50 -0700
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/c0f353803b47

Merge

hg: hsx/hotspot-rt/hotspot: 2 new changesets

[yumin . qi]

Changeset: 4614a598dae1
Author:minqi
Date:  2013-07-19 08:34 -0700
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/4614a598dae1

8016538: volatile double access via Unsafe.cpp is not atomic
Summary: volatile jdouble load/store is not atomic, fix by using of existing 
volatile jlong operations which are atomic for jdouble.
Reviewed-by: kvn, vladidan, jrose
Contributed-by: david.hol...@oracle.com

! src/os_cpu/bsd_x86/vm/orderAccess_bsd_x86.inline.hpp
! src/os_cpu/linux_x86/vm/orderAccess_linux_x86.inline.hpp
! src/os_cpu/solaris_x86/vm/orderAccess_solaris_x86.inline.hpp
! src/os_cpu/windows_x86/vm/orderAccess_windows_x86.inline.hpp

Changeset: 55a61ceb2fe7
Author:minqi
Date:  2013-07-19 11:17 -0700
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/55a61ceb2fe7

Merge

hg: hsx/hotspot-rt/hotspot: 2 new changesets

[yumin . qi]

Changeset: 02d7aa1456c9
Author:ccheung
Date:  2013-07-18 14:57 -0700
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/02d7aa1456c9

8004872: Early loading of HashMap and StringValue under -XX:+AggressiveOpts can 
be removed
Summary: this fix also removes the -XX:+UseStringCache option
Reviewed-by: dholmes, acorn, iklam

! src/share/vm/classfile/vmSymbols.hpp
! src/share/vm/opto/bytecodeInfo.cpp
! src/share/vm/runtime/arguments.cpp
! src/share/vm/runtime/globals.hpp
! src/share/vm/runtime/thread.cpp

Changeset: 383a5e21cc2d
Author:minqi
Date:  2013-07-18 18:00 -0700
URL:   http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/383a5e21cc2d

Merge