RFR: 8252842: Extend jmap to support parallel heap dump

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

RFR: 8252842: Extend jmap to support parallel heap dump

Lin Zang-2
8252842: Extend jmap to support parallel heap dump

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

Commit messages:
 - fix white space trailing issue
 - 8252842: Extend jmap to support parallel heap dump

Changes: https://git.openjdk.java.net/jdk/pull/2261/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2261&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8252842
  Stats: 777 lines in 6 files changed: 590 ins; 57 del; 130 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2261.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2261/head:pull/2261

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

Re: RFR: 8252842: Extend jmap to support parallel heap dump

Lin Zang-2
On Wed, 27 Jan 2021 12:49:29 GMT, Lin Zang <[hidden email]> wrote:

> 8252842: Extend jmap to support parallel heap dump

src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java line 263:

> 261:         // See AttachOperation::arg_count_max in attachListener.hpp for argument count limitation.
> 262:         String more_args = compress_level + "," + parallel;
> 263:         // dumpHeap is not the same as jcmd GC.heap_dump

To-discuss:
Compatibility issue that new version of jmap work on old JDK may wrongly processing the following case:
jmap -dump:file=dump.hprof,gz=1,parallel=2 <pid>
this will cause the gz option not recognized correctly.

Can not simply change arg_count_max to 4, because it could cause JDK hang on waiting for arguments when old jmap is used.

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

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

Re: RFR: 8252842: Extend jmap to support parallel heap dump

Chris Plummer-2
In reply to this post by Lin Zang-2
On Wed, 27 Jan 2021 13:15:06 GMT, Lin Zang <[hidden email]> wrote:

> This patch introduces a new "parallel" option to jmap, so after this patch, there can be at most 4 options for jmap that can be passed to JDK.
>
> The issue is that there is a limitation in old version of jmap that at most 3 arguments can be accepted. As I commented in File JMap.java. and I can not simply enlarge the limitation to 4 because that will cause old version of jmap hang when working on new JDK.

A year or so ago we ran into this same issue. I'll see if I can find the CR for it.

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

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

Re: RFR: 8252842: Extend jmap to support parallel heap dump

Chris Plummer-2
On Wed, 27 Jan 2021 19:21:35 GMT, Chris Plummer <[hidden email]> wrote:

>> Dear All,
>> I have made a draft patch on parallel heap dump of jmap.  
>> This patch is still WIP with test case under development and code refining is in process.
>>
>> But I want to firstly ask your help to discuss a potential compatibility issue that may need to solve:
>>
>> This patch introduces a new "parallel" option to jmap, so after this patch, there can be at most 4 options for jmap that can be passed to JDK.
>>  
>> The issue is that there is a limitation in old version of jmap that at most 3 arguments can be accepted. As I commented in File JMap.java. and I can not simply enlarge the limitation to 4 because that will cause old version of jmap hang when working on new JDK.
>>
>> So I use the 3rd argument of jmap to be a combined string and parse it to JDK, and the attachListener.cpp is modified to add the  logic for parsing it to get gzlevel and parallel thread number.
>>
>> The current status is that, with current change in this patch, old jmap could work normally on new JDK, and new jmap without "parallel" could work correctly with old JDK.  
>>
>> But the problem comes when new jmap use "gz=1, parallel=2" options together to communicate to an old JDK, the "gz" option takes no effect. The root cause is that old JDK's attachListener.cpp does not have the parsing logic, so it treat "gz=1, parallel=2" as a whole argument, and hence can not recognize it.
>>
>> May I ask your suggestion to see whether this is behavior is acceptable, and if not, any advice for fixing the compatibility issue?
>>
>> Thanks!
>> Lin
>
>> This patch introduces a new "parallel" option to jmap, so after this patch, there can be at most 4 options for jmap that can be passed to JDK.
>>
>> The issue is that there is a limitation in old version of jmap that at most 3 arguments can be accepted. As I commented in File JMap.java. and I can not simply enlarge the limitation to 4 because that will cause old version of jmap hang when working on new JDK.
>
> A year or so ago we ran into this same issue. I'll see if I can find the CR for it.

It looks like [JDK-8215622](https://bugs.openjdk.java.net/browse/JDK-8215622) introduced a 4th argument to the attach API, and this caused hangs when and older JVMs tried to attach. That was fixed by [JDK-8219721](https://bugs.openjdk.java.net/browse/JDK-8219721), and seemed to revert the arg count back to 3, although it's not too clear to me how this was accomplished. The webrev is here:

http://cr.openjdk.java.net/~xiaofeya/8219721/webrev.01/

Also at the same time the following was filed, which has not yet been addressed:

[JDK-8219896](https://bugs.openjdk.java.net/browse/JDK-8219896) Parameter size mismatch between client and VM sides of the Attach API

@linzang JDK-8215622 and JDK-8219721 were your CRs, so maybe you can elaborate on them a bit more.

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

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

Re: RFR: 8252842: Extend jmap to support parallel heap dump

Lin Zang-2
On Thu, 28 Jan 2021 07:30:29 GMT, Serguei Spitsyn <[hidden email]> wrote:

>> Dear @plummercj´╝î
>> You are right, the same issue has been encountered when I was developing JDK-8215622. At that time the argument number does not actully exceed the limitation (3), and so I made a quick fix to change back arg_count_max to 3. And the reason that I originally change the arg_count_max is I was trying to add more options to jmap, and finally only "file" and "parallel" were accepted, so the arg_count_max never exceed the limitation.
>>
>> Then it comes to this patch, with "parallel" added to jmap -dump, there can be 4 arguments and we need to handle the arg count limitation.  Here are the experiments I have been tried recently (and also previously when handling JDK-8219721):
>>
>> - Enlarge arg_count_max from 3 to 4
>>
>>    This cause the same issue as describe by JDK-8219721, when using an old jcmd tools to communicate with new JDK, it hangs, which is a huge compatibitily issue, so we need to fix it.
>>
>> - Let jmap pass all arguments as a whole one, just like what jcmd does, and modify attachlistener.cpp to parse arguments from op->arg(0), and also distinguish old/new jmap by testing whether op->arg(1) is null.
>>
>>    This works well until the new jmap is used to communicate with old JDK. In this case, the old JDK doesn't parse op->arg(0), which is passed by jmap as a string containing all arguments. So all parameters are treated as dump filename incorrectly.  
>>
>> - Let jmap still passing three arguents, and combine the "gz" and "parallel" together as a string and pass it to JDK, and modify attachlistener.cpp to parse the 3rd arguments from op->arg(2).
>>
>>   This works well for old/new jmap targeting on old/new JDK when "parallel" and "gz" options are not used together. However, it has the issue when new jmap targeting to old JDK and both "parallel" and "gz" are used.  because old JDK
>> can not parse op->arg(3), so the "gz" option will not work. Although I don't think it is reasonable to use new jmap with "parallel" option on an old JDK, but it is possible, and the issue is that old JDK will not prompt any error message in this case, it just dump the heap by ignoring the "gz" value.
>>
>> - I also worked out a patch using timeout mechanism in socket.
>>
>> This solution seems work, so the arg_count_max could be enlarged to 4, and when old jmap is targeting on new JDK, the socket can be timeout on waiting for the 4th arguments while old jmap only provide 3. And then it could work as expected.  But this seems need to change the socket for different platform, and I just verified it on Linux, I am not sure whether it is acceptable.
>>
>> - I am also trying to see whether there can be other solution that mentioned in the previous discussion thread. ( links here: https://mail.openjdk.java.net/pipermail/serviceability-dev/2019-February/027240.html, FYI)
>>
>> BTW,  Do you think we need to solve this issue seperately? Maybe have a patch for  JDK-8219896?  And when it is fixed, appling the parallel dump patch on top of that?
>>
>> Thanks,
>> Lin
>
> Hi Lin,
> It is also in my memory that you actually did not have 4 arguments.
> The real incompatibility issue was that the order of arguments was swapped.
> It is why it was relatively easy to fall back and just update the constants with 3 instead of 4 and swap the order of arguments back.
> Thanks,
> Serguei

Dear Serguei,
Yes,  you are right!
However, for this PR it seems I have to deal with four arguments for jmap -d : file, live, gz=, parallal=.

BRs,
Lin

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

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

Re: RFR: 8252842: Extend jmap to support parallel heap dump

Chris Plummer-2
In reply to this post by Chris Plummer-2
On Thu, 28 Jan 2021 07:30:29 GMT, Serguei Spitsyn <[hidden email]> wrote:

>  It is also in my memory that you actually did not have 4 arguments.
> The real incompatibility issue was that the order of arguments was swapped.

Ah, now the fix makes a lot more sense. I was always wondering how changing the order allowed reducing the max args to 3, but apparently the two changes are not related. Also, it seems them that [JDK-8219721](https://bugs.openjdk.java.net/browse/JDK-8219721) does not have a correct analysis of the issue. It says:

> JDK-8215622 increased number of arguments of Attach API [1].
> Thus AttachListener will be waiting the command from client infinitely.

Perhaps it should be updated to avoid future confusion.

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

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

Re: RFR: 8252842: Extend jmap to support parallel heap dump

Chris Plummer-2
On Thu, 28 Jan 2021 21:55:41 GMT, Chris Plummer <[hidden email]> wrote:

>> Hi Lin,
>> It is also in my memory that you actually did not have 4 arguments.
>> The real incompatibility issue was that the order of arguments was swapped.
>> It is why it was relatively easy to fall back and just update the constants with 3 instead of 4 and swap the order of arguments back.
>> Thanks,
>> Serguei
>
>>  It is also in my memory that you actually did not have 4 arguments.
>> The real incompatibility issue was that the order of arguments was swapped.
>
> Ah, now the fix makes a lot more sense. I was always wondering how changing the order allowed reducing the max args to 3, but apparently the two changes are not related. Also, it seems them that [JDK-8219721](https://bugs.openjdk.java.net/browse/JDK-8219721) does not have a correct analysis of the issue. It says:
>
>> JDK-8215622 increased number of arguments of Attach API [1].
>> Thus AttachListener will be waiting the command from client infinitely.
>
> Perhaps it should be updated to avoid future confusion.

I think if you can avoid the 4th argument by just enabling parallel by default sounds like a good idea. Is there any reason not to use parallel heap dump? Also, I'm not familiar with the terms "compression backends" and "active workers". Can you explain them.

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

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

Re: RFR: 8252842: Extend jmap to support parallel heap dump

Lin Zang-2
On Thu, 28 Jan 2021 22:14:31 GMT, Chris Plummer <[hidden email]> wrote:

> I think if you can avoid the 4th argument by just enabling parallel by default sounds like a good idea. Is there any reason not to use parallel heap dump? Also, I'm not familiar with the terms "compression backends" and "active workers". Can you explain them.

For parallel heap dump, I think expose the "parallel" option is useful when user want to control the number of threads that works on parallel dumping. One reason is to control the CPU load to not affect too much on other processes on system.

The "active workers" is the value that returned by gang->active_workers(). And I think "active workers" is the number of available threads that could be used for parallally working on something. my understanding is that this value could be dynamically changed at runtime, but it is limited by the JVM option "-XX:ParallelGCThreads".

The "compression backend" is introduced by "gz=" option, it is actually a "backend" that write heap dump data to file, and if there is compression level specified, the "backend" will do compression first and then write to file. The whole "backend" works parallelly, before this PR, the backend thread number is dicided by "active workers".

With this PR,  the relation of parallel heap iteration threads (parallel dumper),  the file writing threads (compression backend) and the "active_works" is like following:
          num_of_dumper_threads + num_of_compression_backend = active_workers.

BRs,
Lin

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

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

Re: RFR: 8252842: Extend jmap to support parallel heap dump [v3]

Lin Zang-2
In reply to this post by Lin Zang-2
> 8252842: Extend jmap to support parallel heap dump

Lin Zang has updated the pull request incrementally with one additional commit since the last revision:

  fix buffer queue issue

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2261/files
  - new: https://git.openjdk.java.net/jdk/pull/2261/files/03c0a965..fb28c13e

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

  Stats: 50 lines in 1 file changed: 11 ins; 31 del; 8 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2261.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2261/head:pull/2261

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

Re: RFR: 8252842: Extend jmap to support parallel heap dump

Lin Zang-2
In reply to this post by Lin Zang-2
On Fri, 29 Jan 2021 00:36:54 GMT, Lin Zang <[hidden email]> wrote:

>> I think if you can avoid the 4th argument by just enabling parallel by default sounds like a good idea. Is there any reason not to use parallel heap dump? Also, I'm not familiar with the terms "compression backends" and "active workers". Can you explain them.
>
>> I think if you can avoid the 4th argument by just enabling parallel by default sounds like a good idea. Is there any reason not to use parallel heap dump? Also, I'm not familiar with the terms "compression backends" and "active workers". Can you explain them.
>
> For parallel heap dump, I think expose the "parallel" option is useful when user want to control the number of threads that works on parallel dumping. One reason is to control the CPU load to not affect too much on other processes on system.
>
> The "active workers" is the value that returned by gang->active_workers(). And I think "active workers" is the number of available threads that could be used for parallally working on something. my understanding is that this value could be dynamically changed at runtime, but it is limited by the JVM option "-XX:ParallelGCThreads".
>
> The "compression backend" is introduced by "gz=" option, it is actually a "backend" that write heap dump data to file, and if there is compression level specified, the "backend" will do compression first and then write to file. The whole "backend" works parallelly, before this PR, the backend thread number is dicided by "active workers".
>
> With this PR,  the relation of parallel heap iteration threads (parallel dumper),  the file writing threads (compression backend) and the "active_works" is like following:
>           num_of_dumper_threads + num_of_compression_backend = active_workers.
>
> BRs,
> Lin

Hi All,
As discussed in this PR, it is reasonable to enable parallel heap dump by default rather than introducing a new "parallel" option. I would like to close the CSR at https://bugs.openjdk.java.net/browse/JDK-8260424.

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

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

Re: RFR: 8252842: Extend jmap to support parallel heap dump [v4]

Lin Zang-2
In reply to this post by Lin Zang-2
> 8252842: Extend jmap to support parallel heap dump

Lin Zang 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 six additional commits since the last revision:

 - Merge branch 'master' into par-dump
 - fix buffer queue issue
 - code refine
 - Do not expose parallel thread number to user
 - fix white space trailing issue
 - 8252842: Extend jmap to support parallel heap dump

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2261/files
  - new: https://git.openjdk.java.net/jdk/pull/2261/files/fb28c13e..06719159

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

  Stats: 25865 lines in 665 files changed: 9292 ins; 6222 del; 10351 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2261.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2261/head:pull/2261

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

Re: RFR: 8252842: Extend jmap to support parallel heap dump [v5]

Lin Zang-2
In reply to this post by Lin Zang-2
> 8252842: Extend jmap to support parallel heap dump

Lin Zang has updated the pull request incrementally with one additional commit since the last revision:

  Fix logic in thread number calculation and code refine

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2261/files
  - new: https://git.openjdk.java.net/jdk/pull/2261/files/06719159..016218ba

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

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

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

Re: RFR: 8252842: Extend jmap to support parallel heap dump

Chris Plummer-2
In reply to this post by Lin Zang-2
On Wed, 3 Feb 2021 21:52:00 GMT, Chris Plummer <[hidden email]> wrote:

>> Dear All,
>> I have updated the patch and remove the introduction of new "parallel" option. So there can be no backward-compatibility issue for jmap -dump command.  May I ask your help to remove the CSR label if agreed?
>>
>> BRs,
>> Lin
>
> Hi Lin, When I suggested that it would ok just to just always use the default number of parallel threads, I wasn't considering that we also have `jmap -histo`, which also supports parallel threads, but includes an argument to specify it. For example, `jmap -histo:parallel=<n>`. Now this PR adds parallel support to `jmap -dump`, but you are suggesting not to have a parallel option to specify it, and use a default number of threads, which would be the same as what you would get with `jmap -histo:parallel=0`.  So this is inconsistent. If `-histo` is going to allow you to control the number of parallel threads, then `-dump` should also. I guess an option would be to not allow `-histo` to control the number of threads, and have it work just like you are proposing for `-dump`.

Here's another thought. What if we added a new Attach API command called something like `setparallel`. Then when you specify `parallel=<n>` for  `jmap -histo` or `jmap -dump`, first JMap would send the `setparallel` command, and then it would send the `dumpheap` or `inspectheap` commands, and they would use the parallel setting from the previous `setparallel` command. I assume of `setparallel` is sent to an older JDK, I think you just get an error message back that can be ignored.

Anyway, just throwing this idea out there. I'm not so sure I like it myself, but given we don't have a good solution yet, it may be as good as any of the others.

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

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

Re: RFR: 8252842: Extend jmap to support parallel heap dump

Chris Plummer-2
On Thu, 4 Feb 2021 03:17:28 GMT, Lin Zang <[hidden email]> wrote:

>  IMO, there can be two ways which may help solve the compatibility issue of introducing "parallel" option:
>
>     * unblock socket with timeout. In this way, old Jmap could work with new JDK, but it maybe complicated because of different implementation for different OS.
>
>     * Two times communication as you mentioned,  it sounds a reasonable solution, I will work out a prototype and then discuss with you .
>
A 3rd would be to create a new `dumpheap` command for the Attach API, maybe `dumpheap_parallel`. If you try using it with an old JVM, it should immediately return an error:

        st.print("Operation %s not recognized!", op->name());
        res = JNI_ERR;
If it does, you can fallback to the regular `dumpheap` command. If an old JVM attaches and uses `dumpheap`, you can choose to make it parallel by default if you wish. A slightly different variant of this idea is have something like `dumpheap_extraargs` or `dumpheap_ext`, leaving room to add even more args in the future (possibly a single argument with a list of args as you suggested before)

BTW, one thing to keep in mind that whatever we do with `dumpheap`, the jcmd version will support fine control over parallelism. So for that reason it wouldn't be so bad to just have `dumpheap` and `inspectheap` always use some default parallelism, and allow tuning it with the jcmd versions. But since the `inspectheap` parallel support already went into 16, that might be hard to undo.

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

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

Re: RFR: 8252842: Extend jmap to support parallel heap dump

Hamlin Li-2
On Thu, 4 Feb 2021 03:52:23 GMT, Chris Plummer <[hidden email]> wrote:

> BTW, one thing to keep in mind that whatever we do with `dumpheap`, the jcmd version will support fine control over parallelism. So for that reason it wouldn't be so bad to just have `dumpheap` and `inspectheap` always use some default parallelism, and allow tuning it with the jcmd versions. But since the `inspectheap` parallel support already went into 16, that might be hard to undo.

Hi Chris, Lin,
IMHO, 16 is not an LTS, maybe it's better for us to adjust the behavior of jmap -histo in 17 which is LTS? I just filed https://bugs.openjdk.java.net/browse/JDK-8261482.
Thanks.

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

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

Re: RFR: 8252842: Extend jmap to support parallel heap dump [v6]

Lin Zang-2
In reply to this post by Lin Zang-2
> 8252842: Extend jmap to support parallel heap dump

Lin Zang has updated the pull request incrementally with one additional commit since the last revision:

  Add dumpheapext command for extra argument passing

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2261/files
  - new: https://git.openjdk.java.net/jdk/pull/2261/files/016218ba..b4776658

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

  Stats: 120 lines in 2 files changed: 111 ins; 0 del; 9 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2261.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2261/head:pull/2261

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

Re: RFR: 8252842: Extend jmap to support parallel heap dump

Lin Zang-2
In reply to this post by Hamlin Li-2
On Wed, 10 Feb 2021 02:00:11 GMT, Hamlin Li <[hidden email]> wrote:

>>>  IMO, there can be two ways which may help solve the compatibility issue of introducing "parallel" option:
>>>
>>>     * unblock socket with timeout. In this way, old Jmap could work with new JDK, but it maybe complicated because of different implementation for different OS.
>>>
>>>     * Two times communication as you mentioned,  it sounds a reasonable solution, I will work out a prototype and then discuss with you .
>>>
>> A 3rd would be to create a new `dumpheap` command for the Attach API, maybe `dumpheap_parallel`. If you try using it with an old JVM, it should immediately return an error:
>>
>>         st.print("Operation %s not recognized!", op->name());
>>         res = JNI_ERR;
>> If it does, you can fallback to the regular `dumpheap` command. If an old JVM attaches and uses `dumpheap`, you can choose to make it parallel by default if you wish. A slightly different variant of this idea is have something like `dumpheap_extraargs` or `dumpheap_ext`, leaving room to add even more args in the future (possibly a single argument with a list of args as you suggested before)
>>
>> BTW, one thing to keep in mind that whatever we do with `dumpheap`, the jcmd version will support fine control over parallelism. So for that reason it wouldn't be so bad to just have `dumpheap` and `inspectheap` always use some default parallelism, and allow tuning it with the jcmd versions. But since the `inspectheap` parallel support already went into 16, that might be hard to undo.
>
>> BTW, one thing to keep in mind that whatever we do with `dumpheap`, the jcmd version will support fine control over parallelism. So for that reason it wouldn't be so bad to just have `dumpheap` and `inspectheap` always use some default parallelism, and allow tuning it with the jcmd versions. But since the `inspectheap` parallel support already went into 16, that might be hard to undo.
>
> Hi Chris, Lin,
> IMHO, 16 is not an LTS, maybe it's better for us to adjust the behavior of jmap -histo in 17 which is LTS? I just filed https://bugs.openjdk.java.net/browse/JDK-8261482 to address the issue if you both think it's OK to undo it.
> Thanks.

Hi Chris,
I have update the PR to add a new dumpheapext command that could handle more arguments passed from JMap.java.
And I have tested with "jmap -dump:file=dump.bin,noparallel [pid]", with this change, new jmap targeting on old hotspot would print:

Exception in thread "main" com.sun.tools.attach.AttachOperationFailedException: Operation dumpheapext not recognized!
        at jdk.attach/sun.tools.attach.VirtualMachineImpl.execute(VirtualMachineImpl.java:227)
        at jdk.attach/sun.tools.attach.HotSpotVirtualMachine.executeCommand(HotSpotVirtualMachine.java:309)
        at jdk.jcmd/sun.tools.jmap.JMap.executeCommandForPid(JMap.java:133)
        at jdk.jcmd/sun.tools.jmap.JMap.dump(JMap.java:262)
        at jdk.jcmd/sun.tools.jmap.JMap.main(JMap.java:114)

IMH, one problem is that it doesn't give out the reason clearly that "dumpheapext" is actually caused by new option "noparallel". But I think having this kind of error message is acceptable. Otherwise it seems the choice we left is remove the "parallel/noparallel" for Jmap. What do you think?

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

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

Re: RFR: 8252842: Extend jmap to support parallel heap dump

Lin Zang-2
On Fri, 12 Feb 2021 00:56:47 GMT, Lin Zang <[hidden email]> wrote:

>>> BTW, one thing to keep in mind that whatever we do with `dumpheap`, the jcmd version will support fine control over parallelism. So for that reason it wouldn't be so bad to just have `dumpheap` and `inspectheap` always use some default parallelism, and allow tuning it with the jcmd versions. But since the `inspectheap` parallel support already went into 16, that might be hard to undo.
>>
>> Hi Chris, Lin,
>> IMHO, 16 is not an LTS, maybe it's better for us to adjust the behavior of jmap -histo in 17 which is LTS? I just filed https://bugs.openjdk.java.net/browse/JDK-8261482 to address the issue if you both think it's OK to undo it.
>> Thanks.
>
> Hi Chris,
> I have update the PR to add a new dumpheapext command that could handle more arguments passed from JMap.java.
> And I have tested with "jmap -dump:file=dump.bin,noparallel [pid]", with this change, new jmap targeting on old hotspot would print:
>
> Exception in thread "main" com.sun.tools.attach.AttachOperationFailedException: Operation dumpheapext not recognized!
> at jdk.attach/sun.tools.attach.VirtualMachineImpl.execute(VirtualMachineImpl.java:227)
> at jdk.attach/sun.tools.attach.HotSpotVirtualMachine.executeCommand(HotSpotVirtualMachine.java:309)
> at jdk.jcmd/sun.tools.jmap.JMap.executeCommandForPid(JMap.java:133)
> at jdk.jcmd/sun.tools.jmap.JMap.dump(JMap.java:262)
> at jdk.jcmd/sun.tools.jmap.JMap.main(JMap.java:114)
>
> IMH, one problem is that it doesn't give out the reason clearly that "dumpheapext" is actually caused by new option "noparallel". But I think having this kind of error message is acceptable. Otherwise it seems the choice we left is remove the "parallel/noparallel" for Jmap. What do you think?

Dear All,
     I am going to create a CSR for the new internal command "dumpheapext", one problem is that seem if a new CSR is created, this PR will depends on 2 CSRs, I am not sure is this reasonable?  Thanks!

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

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

Re: RFR: 8252842: Extend jmap to support parallel heap dump [v7]

Lin Zang-2
In reply to this post by Lin Zang-2
> 8252842: Extend jmap to support parallel heap dump

Lin Zang has updated the pull request incrementally with one additional commit since the last revision:

  use parallel= option instead of noparallel

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2261/files
  - new: https://git.openjdk.java.net/jdk/pull/2261/files/b4776658..44ea7927

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

  Stats: 26 lines in 2 files changed: 14 ins; 1 del; 11 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2261.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2261/head:pull/2261

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

Re: RFR: 8252842: Extend jmap to support parallel heap dump [v8]

Lin Zang-2
In reply to this post by Lin Zang-2
> 8252842: Extend jmap to support parallel heap dump

Lin Zang 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 10 additional commits since the last revision:

 - Merge branch 'master' into pd
 - use parallel= option instead of noparallel
 - Add dumpheapext command for extra argument passing
 - Fix logic in thread number calculation and code refine
 - Merge branch 'master' into par-dump
 - fix buffer queue issue
 - code refine
 - Do not expose parallel thread number to user
 - fix white space trailing issue
 - 8252842: Extend jmap to support parallel heap dump

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2261/files
  - new: https://git.openjdk.java.net/jdk/pull/2261/files/44ea7927..375e6732

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

  Stats: 40119 lines in 1084 files changed: 24892 ins; 10354 del; 4873 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2261.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2261/head:pull/2261

PR: https://git.openjdk.java.net/jdk/pull/2261
123