RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Wed Aug 12 23:49:11 UTC 2020


Hi Lin,

Yes, this fix can pushed.

Thanks,
Serguei


On 8/12/20 16:46, linzang(臧琳) wrote:
> Dear All,
> 	Really appreciated for your time and effort for reviewing this webrev.
> 	So it got 3 approval now (From Paul, Serguei and Stefan). I think maybe it is okay to be pushed?
> 	Or If needs more review, here are the latest webrev and related info.
>
> 	Webrev:  http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev14/
> 	Bug: https://bugs.openjdk.java.net/browse/JDK-8215624
>                CSR(approved): https://bugs.openjdk.java.net/browse/JDK-8239290
>   
> BRs,
> Lin
>
> From: "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>
> Date: Thursday, August 13, 2020 at 1:06 AM
> To: "linzang(臧琳)" <linzang at tencent.com>
> Cc: "Hohensee, Paul" <hohensee at amazon.com>, Stefan Karlsson <stefan.karlsson at oracle.com>, David Holmes <david.holmes at oracle.com>, serviceability-dev <serviceability-dev at openjdk.java.net>, "hotspot-gc-dev at openjdk.java.net" <hotspot-gc-dev at openjdk.java.net>
>
> Subject: Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
>
> Hi Lin,
>
> Thanks you for testing details, it looks good.
>
> Thanks,
> Serguei
>
>
> On 8/11/20 17:22, linzang(臧琳) wrote:
> Dear Serguei,
>                  Here are the tests I have done:
>                  Generally, the new version of jmap could work with the old version of hotspot, the “parallel” option tooks no effect.
> And the old verdion of jmap could work with the new version of hotspot without parallel option, and the jvm side works in parallel heap inspection mode by default.  The old  jmap could not accept the “parallel” option, so usage printed.
>   
> |       histo options      |       Jmap version       |       hotspot version    |                           result                                    |
> |       no option            |              latest             |       1.8.0.232             |        work normally  (parallel take no effect)   |
> |              live               |              latest             |       1.8.0_232             |       work normally  (parallel take no effect)   |
> |       live, parallel=0    |              latest             |       1.8.0_232             |       work normally  (parallel take no effect)   |
> |       live, parallel=1    |              latest             |       1.8.0_232             |       work normally  (parallel take no effect)   |
> |       live, parallel=2    |              latest             |       1.8.0_232             |       work normally  (parallel take no effect)   |
> |       parallel=3           |              latest             |        1.8.0.232             |        work normally  (parallel take no effect)  |
> |       no option            |              latest             |          11.0.2               |        work normally  (parallel take no effect)   |
> |            live                 |              latest             |          11.0.2               |       work normally  (parallel take no effect)   |
> |       live, parallel=0    |              latest             |          11.0.2               |       work normally  (parallel take no effect)   |
> |       live, parallel=1    |              latest             |          11.0.2               |       work normally  (parallel take no effect)   |
> |       live, parallel=2    |              latest             |          11.0.2               |       work normally  (parallel take no effect)   |
> |       parallel=3           |              latest             |           11.0.2              |        work normally  (parallel take no effect)   |
> |       no option            |              latest             |          14.0.2               |        work normally  (parallel take no effect)  |
> |            live                 |              latest             |          14.0.2               |       work normally  (parallel take no effect)   |
> |       live, parallel=0    |              latest             |          14.0.2               |       work normally  (parallel take no effect)   |
> |       live, parallel=1    |              latest             |          14.0.2               |       work normally  (parallel take no effect)   |
> |       live, parallel=2    |              latest             |          14.0.2               |       work normally  (parallel take no effect)   |
> |       parallel=3           |              latest             |           14.0.2              |        work normally  (parallel take no effect)   |
>   
> |       no option            |            1.8.0.232        |           latest                |        work normally  (parallel by default)        |
> |            live                 |            1.8.0.232        |           latest                |        work normally  (parallel by default)        |
> |       live, parallel=0    |            1.8.0.232        |           latest                |                      usage printed                           |
> |       live, parallel=1    |            1.8.0.232        |           latest                |                      usage printed                           |
> |       live, parallel=2    |            1.8.0.232        |           latest                |                      usage printed                           |
> |       parallel=3           |            1.8.0.232        |           latest                |                      usage printed                           |
> |       no option            |              11.0.2          |           latest                |        work normally  (parallel by default)        |
> |            live                 |              11.0.2          |           latest                |        work normally  (parallel by default)        |
> |       live, parallel=0    |              11.0.2          |           latest                |                      usage printed                           |
> |       live, parallel=1    |               11.0.2         |           latest                |                      usage printed                           |
> |       live, parallel=2    |               11.0.2         |           latest                |                      usage printed                           |
> |       parallel=3           |               11.0.2         |           latest                |                      usage printed                           |
> |       no option            |              14.0.2          |           latest                |        work normally  (parallel by default)        |
> |            live                 |              14.0.2          |           latest                |        work normally  (parallel by default)        |
> |       live, parallel=0    |              14.0.2          |           latest                |                      usage printed                           |
> |       live, parallel=1    |               14.0.2         |           latest                |                      usage printed                           |
> |       live, parallel=2    |               14.0.2         |           latest                |                      usage printed                           |
> |       parallel=3           |               14.0.2         |           latest                |                      usage printed                           |
>   
>   
> BRs,
> Lin
>   
> From: mailto:serguei.spitsyn at oracle.com mailto:serguei.spitsyn at oracle.com
> Date: Wednesday, August 12, 2020 at 4:23 AM
> To: "linzang(臧琳)" mailto:linzang at tencent.com
> Cc: "Hohensee, Paul" mailto:hohensee at amazon.com, Stefan Karlsson mailto:stefan.karlsson at oracle.com, David Holmes mailto:david.holmes at oracle.com, serviceability-dev mailto:serviceability-dev at openjdk.java.net, mailto:hotspot-gc-dev at openjdk.java.net mailto:hotspot-gc-dev at openjdk.java.net
> Subject: Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
>   
> Hi Lin,
>
> The latest webrev looks good to me.
> Just want to double check, how did you check no regressions are introduced with your fix?
>
> Thanks,
> Serguei
>
>
>
> On 8/11/20 08:22, linzang(臧琳) wrote:
> Hi Serguei,
>                  Thanks a lot for your advice. I agree your concern and will take care of it in future.
>                  Here is the latest webrev based on your comments:  (delta is just retrieving the usage(1))
>                  http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev14/
>                So may I assume that the patch is OK with you now?
> Hi All,
>                  In summary, Here are the status of this change at present:
>                  * Paul and Serguei have helped review the runtime/JMap part and the changes now is Okay with them.
>                  * Stefan has helped review the GC part and it is Okay with him now.
>                  So does it need more review and approval for pushing this change?
>   
> BRs,
> Lin
>   
> On 2020/8/11, 2:40 PM, mailto:serguei.spitsyn at oracle.com mailto:serguei.spitsyn at oracle.com wrote:
>   
>      Hi Lin,
>   
>      I prefer a conservative approach and do not change things without a real
>      need.
>   
>      Thanks,
>      Serguei
>   
>      On 8/10/20 20:23, linzang(臧琳) wrote:
>      > Hi Serguei
>      >      I got your point, just thought usage may be a little verbosity, it prints almost my whole screen which could flush the error message. And I checked that other jcmd tools usually use System.exit() after print errors. So I made the change.
>      >
>     > Thanks!
>      > Lin
>      >
>      >> On Aug 11, 2020, at 11:05 AM, mailto:serguei.spitsyn at oracle.com mailto:serguei.spitsyn at oracle.com wrote:
>      >>
>      >> Hi Lin,
>      >>
>      >> I've re-reviewed the JMap.java only.
>      >> It looks good except there was no need to replace the usage(1) call with the System.exit(1).
>      >> I did not say usage is not needed, just that it is not enough.
>      >>
>      >> Thanks,
>      >> Serguei
>      >>
>      >>
>      >>> On 8/10/20 19:25, linzang(臧琳) wrote:
>      >>> Hi Serguei,
>      >>>           >> First, the CSR does not include any update for 'live' and 'all' options, does it?
>      >>>     >> If so, then I'm confused why do you need all these changes related to these two options.
>      >>>     >> Did you intend to really change anything?
>      >>>      Yes, you’re correct, CSR doesn’t mention any thing about “live” and “all”. so all those changes related become unnecessary.
>      >>>
>      >>>      Webrev: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_13
>      >>>      Delta: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_13_delta
>      >>>
>      >>>      BTW, during refining this changeset I also found an issue that jmap -dump could accept undefined options, will setup a new issue in JBS and fix it separately soon.
>      >>>
>      >>>
>      >>> BRs,
>      >>> Lin
>      >>>
>      >>> From: mailto:serguei.spitsyn at oracle.com mailto:serguei.spitsyn at oracle.com
>      >>> Date: Tuesday, August 11, 2020 at 8:40 AM
>      >>> To: "linzang(臧琳)" mailto:linzang at tencent.com, "Hohensee, Paul" mailto:hohensee at amazon.com, Stefan Karlsson mailto:stefan.karlsson at oracle.com, David Holmes mailto:david.holmes at oracle.com, serviceability-dev mailto:serviceability-dev at openjdk.java.net, mailto:hotspot-gc-dev at openjdk.java.net mailto:hotspot-gc-dev at openjdk.java.net
>      >>> Subject: Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
>      >>>
>      >>> Hi Lin,
>      >>>
>      >>> A couple of things.
>      >>>
>      >>> First, the CSR does not include any update for 'live' and 'all' options, does it?
>      >>> If so, then I'm confused why do you need all these changes related to these two options.
>      >>> Did you intend to really change anything?
>      >>>
>      >>> Second, new error messages do not look useful as they say nothing about what is wrong.
>      >>> Printing usage does not help either.
>      >>> Could these messages be more specific?
>      >>> My suggestions are:
>      >>>   188                 if (filename == null) {
>      >>>   189                     System.err.println("Fail at processing option '" + subopt +"'");
>      >>>   190                     usage(1); // invalid options or no filename
>      >>>   191                 }
>      >>>    System.err.println("Fail: invalid option or no file name: '" + subopt +"'");
>      >>>   194                if (parallel == null) {
>      >>>   195                     System.err.println("Fail at processing option '" + subopt + "'");
>      >>>   196                     usage(1);
>      >>>   197                }
>      >>>    System.err.println("Fail: no number provided in option: '" + subopt +"'");
>      >>>   198             } else {
>      >>>   199                 System.err.println("Fail at processing option '" + subopt + "'");
>      >>>   200                 usage(1);
>      >>>   201             }
>      >>>    System.err.println("Fail: invalid option: '" + subopt +"'");
>      >>>
>      >>>
>      >>> The default value is listed in the 'parallel' flag description:
>      >>>    parallel=<count> generate histogram using this many parallel threads, default 0
>      >>> It means that the flag is optionl.
>      >>>
>      >>> I'm okay to file a separate enhancement to add a clarification for 'live' and 'all' flags.
>      >>>
>      >>> Thanks,
>      >>> Serguei
>      >>>
>      >>>
>      >>> On 8/10/20 16:46, linzang(臧琳) wrote:
>      >>> And Here is the latest refined changeset
>      >>> Webrev: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_12/
>      >>> Delta: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_12_delta/
>      >>>
>      >>> BRs,
>      >>> Lin
>      >>>
>      >>> On 2020/8/11, 7:23 AM, "linzang(臧琳)" mailto:linzang at tencent.com wrote:
>      >>>
>      >>>      Dear Serguei,
>      >>>                Here is my reply for your question about non-numeric value for “parallel” (somehow the thread of replay became out of order, not sure why).
>      >>>
>      >>>      >      >> What is going to happen if the resulting 'parallel' substring above is not a number?
>      >>>      >      The error handling logic locates at http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_11/src/hotspot/share/services/attachListener.cpp.frames.html, (line 276-284)
>      >>>      >      Generally, the result is error message will be print if “parallel” is illegal.  An example output would be:
>      >>>      >     ############################
>     >>>      >          $ time jmap -histo:parallel=c 26233
>      >>>      >        Exception in thread "main" com.sun.tools.attach.AttachOperationFailedException: Invalid parallel thread number: [c]
>      >>>      >                                              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.histo(JMap.java:206)
>      >>>      >                                               at jdk.jcmd/sun.tools.jmap.JMap.main(JMap.java:112)
>      >>>      >
>      >>>      >    ############################
>      >>>      >
>      >>>      > Hi Serguei, Paul and Stefan.
>      >>>      >      Moreover, I will made a new changeset with following changes:
>      >>>      >    * Print error message + usage when parameter check fail in Jmap.java
>      >>>      >    *Retrive the histo logic that if “all” and “live” are set at same time, use “live”, rather than print error message. (not sure which one is better :P)
>      >>>
>      >>>      My last point is to retrive the behavior for compatibility.  And do you think make a separate enhancement about spec is reasonable ?
>      >>>
>      >>>      Thanks!
>      >>>
>      >>>      BRs,
>      >>>      Lin
>      >>>
>      >>>      From: mailto:serguei.spitsyn at oracle.com mailto:serguei.spitsyn at oracle.com
>      >>>      Date: Tuesday, August 11, 2020 at 5:11 AM
>      >>>      To: "linzang(臧琳)mailto:linzang at tencent.com,Hohensee, Paul" mailto:hohensee at amazon.com, Stefan Karlsson mailto:stefan.karlsson at oracle.com, David Holmes mailto:david.holmes at oracle.com, serviceability-dev mailto:serviceability-dev at openjdk.java.net, mailto:hotspot-gc-dev at openjdk.java.net mailto:hotspot-gc-dev at openjdk.java.net
>      >>>      Subject: Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
>      >>>
>      >>>      Hi Lin,
>      >>>
>      >>>
>      >>>      On 8/7/20 03:41, linzang(臧琳) wrote:
>      >>>      Dear Serguei,
>      >>>               Thanks a lot for your review!
>      >>>      >> The spec says nothing if the new option 'parallel' is mandatory or optional.
>      >>>      >> Also, (it was before your fix) the spec does not say if the options 'live' and 'all' are mutually exclusive.
>      >>>               For “parallel”,  the spec adds “parallel=0” is the default behavior. So my assumption is if parallel is not used, it will be 0.  Do you think it is ok ? is it necessary to obviously add comments like “if no parallel is set, use the default value 0”?
>      >>>
>      >>>      It'd be nice to make it clear.
>      >>>      But the CSR will need to be updated.
>      >>>      In fact, I did not want you to go through this cycle again.
>      >>>      But maybe it is worth to improve the specs in this regard.
>      >>>      May be Paul has some alternative suggestions.
>      >>>
>      >>>
>      >>>               For “live” and “all”, before the changeset , I see the logic from the code is that both of them can be set at the same time, and the “live” will take effect. IMHO this may be a little confused. So I made the change, not sure whether I should keep the same behavior as before in this change?
>      >>>
>      >>>      This is better to clearly specify what is allowed and what is the behavior.
>      >>>
>      >>>
>      >>>               And I like your idea of printing more error msg if something wrong with the options setting, but I checked that before the change, if there is not a match option, it only print usage. and not only jmap -histo but also jmap -dump has this issue, do you agree if I fix both in the changeset?
>      >>>
>      >>>      Yes, it'd be nice to make it clear in both specs.
>      >>>
>      >>>                      >> What is going to happen if null is passed in place of parallel here? :
>      >>>              The default value 0 will be used if no “parallel” option is set.
>      >>>
>      >>>      Okay, thanks.
>      >>>
>      >>>
>      >>>                                     >>  Should the lines 193-195 be moved after the line 202?
>      >>>               I don’t think so, the logic is a little different.  At line 193, the case is “parallel=<blank>”.   If move them to line 203, it mean “parallel” is not optional.
>      >>>      Okay, I see what you mean.
>      >>>      The problem is that the help/spec says nothing about the flag 'parallel' as being optional.
>      >>>
>      >>>
>      >>>      I also asked this question:
>      >>>        Q: What is going to happen if the resulting 'parallel' sub-string above is not a number?
>      >>>
>      >>>
>      >>>      Thanks,
>      >>>      Serguei
>      >>>
>      >>>
>      >>>
>      >>>              Thanks!
>      >>>
>      >>>
>      >>>      BRs,
>      >>>      Lin
>      >>>
>      >>>      From: mailto:serguei.spitsyn at oracle.com mailto:serguei.spitsyn at oracle.com
>      >>>      Date: Friday, August 7, 2020 at 3:28 PM
>      >>>      To: "linzang(臧琳)mailto:linzang at tencent.com,Hohensee, mailto:Paulmailto:hohensee at amazon.com,StefanKarlssonmailto:stefan.karlsson at oracle.com,DavidHolmesmailto:david.holmes at oracle.com,serviceability-devmailto:serviceability-dev at openjdk.java.net,mailto:hotspot-gc-dev at openjdk.java.netmailto:hotspot-gc-dev at openjdk.java.netSubject:Re:RFR(L):8215624:addparallelheapinspectionsupportforjmaphisto(G1)(Internetmail)HiLin,Notsure,Ifullyunderstandthespecupdateandtheoptionsprocessinginthefile:http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_11/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java.frames.htmlThespecsaysnothingifthenewoption'parallel'ismandatoryoroptional.Also,(itwasbeforeyourfix)thespecdoesnotsayiftheoptions'live'and'all'aremutuallyexclusive.TheJMap.javaimplementationjustprintusageintwocases:191}elseif(subopt.startsWith(parallel=")) {
>      >>>       192                parallel = subopt.substring("parallel=".length());
>      >>>       193                if (parallel == null) {
>      >>>       194                     usage(1);
>      >>>       195                }
>      >>>       ...
>      >>>       200         if (set_live && set_all) {
>      >>>       201             usage(1);
>      >>>       202         }
>      >>>      It is not that helpful as the usage does not explain anything about these corner cases.
>      >>>      Also, it allows to pass no parallel option.
>      >>>      What is going to happen if null is passed in place of parallel here? :
>      >>>       206         executeCommandForPid(pid, "inspectheap", liveopt, filename, parallel);
>      >>>
>      >>>      Should the lines 193-195 be moved after the line 202?
>      >>>
>      >>>      Thanks,
>      >>>      Serguei
>      >>>
>      >>>
>      >>>      On 8/5/20 18:59, linzang(臧琳) wrote:
>      >>>      Thanks Paul!
>      >>>      And I have verified this change could build success in windows.
>      >>>
>      >>>      BRs,
>      >>>      Lin
>      >>>
>      >>>      On 2020/8/6, 4:17 AM, "Hohensee, mailto:Paulmailto:hohensee at amazon.comwrote:Twotinynitsthatdon'tneedanewwebrev:InheapInspection.cpp,youdon'tneedtocastmissed_counttouintxinthecalltolog_info().InheapInspection.hpp,youcandeletetwoofthethreeblanklinesbefore#endif//SHARE_MEMORY_HEAPINSPECTION_HPPThanks,PaulOn8/5/20,6:46AM,linzang(臧琳)" mailto:linzang at tencent.com wrote:
>      >>>
>      >>>              Hi Paul, Stefan and Serguei,
>      >>>                  Here I uploaded a new changeset, would you like to help review again?
>      >>>                  Webrev: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_11/
>      >>>                  Delta (based on webrev10): http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_11_delta/
>      >>>
>      >>>                  P.S.  I am in process of building it on windows environment for a double check. May update result later. Thanks!
>      >>>
>      >>>
>      >>>              BRs,
>      >>>              Lin
>      >>>
>      >>>
>      >>>
>      >>>
>      >>>
>      >>
>   
>   
>
>
>
>



More information about the hotspot-gc-dev mailing list