Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
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.spit...@oracle.com" Date: Thursday, August 13, 2020 at 1:06 AM To: "linzang(臧琳)" Cc: "Hohensee, Paul" , Stefan Karlsson , David Holmes , serviceability-dev , "hotspot-gc-...@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 | | li
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
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.spit...@oracle.com" Date: Thursday, August 13, 2020 at 1:06 AM To: "linzang(臧琳)" Cc: "Hohensee, Paul" , Stefan Karlsson , David Holmes , serviceability-dev , "hotspot-gc-...@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
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
|
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:
"serguei.spit...@oracle.com"
Date: Wednesday, August 12, 2020 at 4:23 AM
To: "linzang(臧琳)"
Cc: "Hohensee, Paul" ,
Stefan Karlsson , David
Holmes , serviceability-dev
,
"hotspot-gc-...@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,
"serguei.spit...@oracle.com"
wrote:
Hi Lin,
I prefer a conservative approach
and do not change things without a real
need.
RE: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
The latest webrev looks good to me also. Thanks, Paul From: "linzang(臧琳)" Date: Tuesday, August 11, 2020 at 5:24 PM To: "serguei.spit...@oracle.com" Cc: "Hohensee, Paul" , Stefan Karlsson , David Holmes , serviceability-dev , "hotspot-gc-...@openjdk.java.net" Subject: RE: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail) 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|
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
| | 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: "serguei.spit...@oracle.com" Date: Wednesday, August 12, 2020 at 4:23 AM To: "linzang(臧琳)" Cc: "Hohensee, Paul" , Stefan Karlsson , David Holmes , serviceability-dev , "hotspot-gc-...@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, "serguei.spit...@oracle.com"<mailto:serguei.spit...@oracle.com> <mailto:serguei.spit...@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, "serguei.spit...@oracle.com"<mailto:serguei.spit...@oracle.com> <mailto:serguei.spit...@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: "serguei.spit...@oracle.com"<mailto:serguei.spit...@oracle.com> <mailto:serguei.spit...@oracle.com> >>> Date: Tuesday, August 11, 2020 at 8:40 AM >>> To: "linzang(臧琳)" <mailto:linz...@tencent.com>, "H
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,
"serguei.spit...@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, "serguei.spit...@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:
"serguei.spit...@oracle.com"
>>> Date: Tuesday, August
11, 2020 at 8:40 AM
>>> To: "linzang(臧琳)" ,
"Hohensee, Paul" , Stefan Karlsson
, David Holmes
, serviceability-dev
,
"hotspot-gc-...@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
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
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, "serguei.spit...@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, "serguei.spit...@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: "serguei.spit...@oracle.com"
>>> Date: Tuesday, August 11, 2020 at 8:40 AM
>>> To: "linzang(臧琳)" , "Hohensee, Paul"
, Stefan Karlsson , David
Holmes , serviceability-dev
, "hotspot-gc-...@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 +"'");
>>> 194if (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 + "'");
>>&
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
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, "serguei.spit...@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: "serguei.spit...@oracle.com"
Date: Tuesday, August 11, 2020 at 8:40 AM
To: "linzang(臧琳)" , "Hohensee, Paul" , Stefan Karlsson
, David Holmes , serviceability-dev ,
"hotspot-gc-...@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 +"'");
194if (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= 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:linz...@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.
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
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, "serguei.spit...@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: "serguei.spit...@oracle.com"
>> Date: Tuesday, August 11, 2020 at 8:40 AM
>> To: "linzang(臧琳)" , "Hohensee, Paul"
>> , Stefan Karlsson , David
>> Holmes , serviceability-dev
>> , "hotspot-gc-...@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
>> +"'");
>> 194if (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= 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:linz...@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 err
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
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: "serguei.spit...@oracle.com"
Date: Tuesday, August 11, 2020 at 8:40 AM
To: "linzang(臧琳)" , "Hohensee, Paul" , Stefan Karlsson
, David Holmes , serviceability-dev ,
"hotspot-gc-...@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 +"'");
194if (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= 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:linz...@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 chang
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
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: "serguei.spit...@oracle.com"
Date: Tuesday, August 11, 2020 at 8:40 AM
To: "linzang(臧琳)" , "Hohensee, Paul"
, Stefan Karlsson , David
Holmes , serviceability-dev
, "hotspot-gc-...@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 +"'");
194if (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= 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:linz...@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 la
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 +"'");
194if (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= 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(臧琳)" 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: "serguei.spit...@oracle.com"
Date: Tuesday, August 11, 2020 at 5:11 AM
To: "linzang(臧琳)" , "Hohensee, Paul" , Stefan Karlsson , David Holmes , serviceability-dev , "hotspot-gc-...@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 spe
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
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(臧琳)" 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: "serguei.spit...@oracle.com" Date: Tuesday, August 11, 2020 at 5:11 AM To: "linzang(臧琳)" , "Hohensee, Paul" , Stefan Karlsson , David Holmes , serviceability-dev , "hotspot-gc-...@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=”. 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 '
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=”. 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:
"serguei.spit...@oracle.com"
Date: Friday, August 7, 2020 at 3:28 PM
To: "linzang(臧琳)"
, "Hohensee, Paul"
, Stefan Karlsson
, David Holmes
, serviceability-dev
,
"hotspot-gc-...@openjdk.java.net"
Subject: Re: RFR(L): 8215624: add parallel heap
inspection support for jmap histo(G1)(Internet mail)
Hi Lin,
Not sure, I fully understand the spec update and the options
processing in the file:
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_11/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java.frames.html
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.
The JMap.java implementation just print usage in two cases:
191 } else if (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 pas
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
Exactly.
Thanks,
StefanK
>
> BRs,
>
> Lin
>
> *From: *"serguei.spit...@oracle.com"
> *Date: *Wednesday, August 5, 2020 at 1:02 PM
> *To: *"linzang(臧琳)" , "Hohensee, Paul"
> , Stefan Karlsson ,
> David Holmes , serviceability-dev
> , "hotspot-gc-...@openjdk.java.net"
>
> *Subject: *Re: RFR(L): 8215624: add parallel heap inspection support for
> jmap histo(G1)(Internet mail)
>
> Oh, sorry for the confusion, please, skip my question. :)
> C++ does not have the '&&=' operator.
>
> Thanks,
> Serguei
>
> On 8/4/20 21:56, serguei.spit...@oracle.com
> wrote:
>
> Hi Lin,
>
> https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/src/hotspot/share/memory/heapInspection.cpp.udiff.html
>
> +class KlassInfoTableMergeClosure : public KlassInfoClosure {
>
> +private:
>
> + KlassInfoTable* _dest;
>
> + bool _success;
>
> +public:
>
> + KlassInfoTableMergeClosure(KlassInfoTable* table) : _dest(table),
> _success(true) {}
>
> + void do_cinfo(KlassInfoEntry* cie) {
>
> +_success &= _dest->merge_entry(cie);
>
> + }
>
> The operator '&=' above looks strange.
> Did you actually want to use the operator '&&=' instead? :
>
> +_success &&= _dest->merge_entry(cie);
>
>
> Thanks,
> Serguei
>
>
>
>
> On 8/3/20 07:51, linzang(臧琳) wrote:
>
> Dear Stefan,
>
> May I ask your help to review again? I have made a delta based on the last changeset you have reviewed(webrev04),hope it could ease your reviewing work.
>
> webrev:https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/
>
> delta (vs webrev04):https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/delta_10vs04/webrev/
>
> bug:https://bugs.openjdk.java.net/browse/JDK-8215624
>
> CSR(approved):https://bugs.openjdk.java.net/browse/JDK-8239290
>
>
>
> BRs,
>
> Lin
>
> On 2020/7/30, 5:21 AM, "Hohensee, Paul"wrote:
>
> A submit repo run with this succeeded, so afaic you're good to go. Stefan, you reviewed the GC part before, it'd be great if you could ok the final version.
>
> Thanks,
>
> Paul
>
> On 7/29/20, 5:02 AM, "linzang(臧琳)"wrote:
>
> Upload a new change athttp://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/
>
> It fix an issue of windows fail :
>
>
>
> In heapInspect.cpp
>
> - size_t HeapInspection::populate_table(
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
ay, August 5, 2020 at 1:02 PM
> *To: *"linzang(臧琳)" , "Hohensee, Paul"
> , Stefan Karlsson ,
> David Holmes , serviceability-dev
> , "hotspot-gc-...@openjdk.java.net"
>
> *Subject: *Re: RFR(L): 8215624: add parallel heap inspection support for
> jmap histo(G1)(Internet mail)
>
> Oh, sorry for the confusion, please, skip my question. :)
> C++ does not have the '&&=' operator.
>
> Thanks,
> Serguei
>
> On 8/4/20 21:56, serguei.spit...@oracle.com
> wrote:
>
> Hi Lin,
>
> https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/src/hotspot/share/memory/heapInspection.cpp.udiff.html
>
> +class KlassInfoTableMergeClosure : public KlassInfoClosure {
>
> +private:
>
> + KlassInfoTable* _dest;
>
> + bool _success;
>
> +public:
>
> + KlassInfoTableMergeClosure(KlassInfoTable* table) : _dest(table),
> _success(true) {}
>
> + void do_cinfo(KlassInfoEntry* cie) {
>
> +_success &= _dest->merge_entry(cie);
>
> + }
>
> The operator '&=' above looks strange.
> Did you actually want to use the operator '&&=' instead? :
>
> +_success &&= _dest->merge_entry(cie);
>
>
> Thanks,
> Serguei
>
>
>
>
> On 8/3/20 07:51, linzang(臧琳) wrote:
>
> Dear Stefan,
>
> May I ask your help to review again? I have made a delta based on the last changeset you have reviewed(webrev04),hope it could ease your reviewing work.
>
> webrev:https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/
>
> delta (vs webrev04):https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/delta_10vs04/webrev/
>
> bug:https://bugs.openjdk.java.net/browse/JDK-8215624
>
> CSR(approved):https://bugs.openjdk.java.net/browse/JDK-8239290
>
>
>
> BRs,
>
> Lin
>
> On 2020/7/30, 5:21 AM, "Hohensee, Paul"wrote:
>
> A submit repo run with this succeeded, so afaic you're good to go. Stefan, you reviewed the GC part before, it'd be great if you could ok the final version.
>
> Thanks,
>
> Paul
>
> On 7/29/20, 5:02 AM, "linzang(臧琳)"wrote:
>
> Upload a new change athttp://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/
>
> It fix an issue of windows fail :
>
>
>
> In heapInspect.cpp
>
> - size_t HeapInspection::populate_table(KlassInfoTable* cit, BoolObjectClosure *filter, uint parallel_thread_num) {
>
> + uint HeapInspection::populate_table(KlassInfoTable* cit, BoolObjectClosure *filter, uint parallel_thread_num) {
>
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
Hi Paul,
Yes, I'm reviewing it.
Sorry for the late reply.
Thanks,
Serguei
On 8/6/20 06:49, Hohensee, Paul wrote:
And a submit repo run succeeds.
Serguei, would you be willing to review?
Thanks,
Paul
On 8/5/20, 7:00 PM, "linzang(臧琳)" wrote:
CAUTION: This email originated from outside of the organization. Do not
click links or open attachments unless you can confirm the sender and know the
content is safe.
Thanks Paul!
And I have verified this change could build success in windows.
BRs,
Lin
On 2020/8/6, 4:17 AM, "Hohensee, Paul" wrote:
Two tiny nits that don't need a new webrev:
In heapInspection.cpp, you don't need to cast missed_count to uintx in
the call to log_info().
In heapInspection.hpp, you can delete two of the three blank lines
before #endif // SHARE_MEMORY_HEAPINSPECTION_HPP
Thanks,
Paul
On 8/5/20, 6:46 AM, "linzang(臧琳)" 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
On 2020/8/5, 8:56 PM, "Hohensee, Paul" wrote:
uintx is fine with me.
Thanks,
Paul
On 8/5/20, 1:14 AM, "linzang(臧琳)" wrote:
Hi Stefan,
I got your point, thanks for explanation.
I missed the atomic part when considering it.
Hi Paul,
Do you think it is ok to use uintx? I checked it is
actually a uintptr_t type.
BRs,
Lin
On 2020/8/5, 3:39 PM, "Stefan Karlsson"
wrote:
On 2020-08-05 07:22, linzang(臧琳) wrote:
> Hi Serguei,
>
> No problem, Thanks for your reviewing :)
>
> I wll upload a new webrev later, so may I ask
your help to review it
> again?
>
> Hi Stefan,
>
> As Paul mentioned, the _/missed/_count is not a
size, so size_t may
> not be clear, what’s your opinion about uint64_t?
We typically don't restrict the usage of size_t to
only *sizes* in the
HotSpot. If you search the code you'll find many count
variables using
size_t, so I personally don't see the need to change
the type.
However, if you really do want to change it then maybe
using another
type that is 32 bits on 32-bit machines, maybe uintx?
IIRC, using
uint64_t and some of the Atomics operations are
problematic on some
32-bit platforms, so using a type that matches the
word size of the
targetted machine helps you not having to think about
that.
>
> It seems the uint overflow may happened on 64bit
machine with large
> heap, e.g. may be more than 4 Giga objects (8byte
header + 8 byte
> klassptr + 8byte field) in a heap that is larger
than 96 GB, uint64_t
> is ok in this case.
Exactly.
Thanks,
StefanK
>
> BRs,
>
> Lin
>
> *From: *"serguei.spit...@oracle.com"
> *Date: *Wednesday, August 5, 2020 at 1:02 PM
> *To: *"linzang(臧琳)" , "Hohensee,
Paul"
> , Stefan Karlsson
,
> David Holmes ,
serviceability-dev
> ,
"hotspot-gc-...@openjdk.java.net"
>
> *Subject: *Re: RFR(L): 8215624: add parallel heap
inspection support for
> jmap histo(G1)(Internet mail)
>
> Oh, sorry for the confus
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
And a submit repo run succeeds.
Serguei, would you be willing to review?
Thanks,
Paul
On 8/5/20, 7:00 PM, "linzang(臧琳)" wrote:
CAUTION: This email originated from outside of the organization. Do not
click links or open attachments unless you can confirm the sender and know the
content is safe.
Thanks Paul!
And I have verified this change could build success in windows.
BRs,
Lin
On 2020/8/6, 4:17 AM, "Hohensee, Paul" wrote:
Two tiny nits that don't need a new webrev:
In heapInspection.cpp, you don't need to cast missed_count to uintx in
the call to log_info().
In heapInspection.hpp, you can delete two of the three blank lines
before #endif // SHARE_MEMORY_HEAPINSPECTION_HPP
Thanks,
Paul
On 8/5/20, 6:46 AM, "linzang(臧琳)" 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
On 2020/8/5, 8:56 PM, "Hohensee, Paul" wrote:
uintx is fine with me.
Thanks,
Paul
On 8/5/20, 1:14 AM, "linzang(臧琳)" wrote:
Hi Stefan,
I got your point, thanks for explanation.
I missed the atomic part when considering it.
Hi Paul,
Do you think it is ok to use uintx? I checked it is
actually a uintptr_t type.
BRs,
Lin
On 2020/8/5, 3:39 PM, "Stefan Karlsson"
wrote:
On 2020-08-05 07:22, linzang(臧琳) wrote:
> Hi Serguei,
>
> No problem, Thanks for your reviewing :)
>
> I wll upload a new webrev later, so may I ask
your help to review it
> again?
>
> Hi Stefan,
>
> As Paul mentioned, the _/missed/_count is not a
size, so size_t may
> not be clear, what’s your opinion about uint64_t?
We typically don't restrict the usage of size_t to only
*sizes* in the
HotSpot. If you search the code you'll find many count
variables using
size_t, so I personally don't see the need to change
the type.
However, if you really do want to change it then maybe
using another
type that is 32 bits on 32-bit machines, maybe uintx?
IIRC, using
uint64_t and some of the Atomics operations are
problematic on some
32-bit platforms, so using a type that matches the word
size of the
targetted machine helps you not having to think about
that.
>
> It seems the uint overflow may happened on 64bit
machine with large
> heap, e.g. may be more than 4 Giga objects (8byte
header + 8 byte
> klassptr + 8byte field) in a heap that is larger than
96 GB, uint64_t
> is ok in this case.
Exactly.
Thanks,
StefanK
>
> BRs,
>
> Lin
>
> *From: *"serguei.spit...@oracle.com"
> *Date: *Wednesday, August 5, 2020 at 1:02 PM
> *To: *"linzang(臧琳)" , "Hohensee,
Paul"
> , Stefan Karlsson
,
> David Holmes ,
serviceability-dev
> ,
"hotspot-gc-...@openjdk.java.net"
>
> *Subject: *Re: RFR(L): 8215624: add parallel heap
inspection support for
> jmap histo(G1)(Internet mail)
>
> Oh, sorry for the confusion, please, skip my
question. :)
> C++ does not have the '&&=' operator.
>
> Thanks,
> Serguei
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
Thanks Paul!
And I have verified this change could build success in windows.
BRs,
Lin
On 2020/8/6, 4:17 AM, "Hohensee, Paul" wrote:
Two tiny nits that don't need a new webrev:
In heapInspection.cpp, you don't need to cast missed_count to uintx in the
call to log_info().
In heapInspection.hpp, you can delete two of the three blank lines before
#endif // SHARE_MEMORY_HEAPINSPECTION_HPP
Thanks,
Paul
On 8/5/20, 6:46 AM, "linzang(臧琳)" 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
On 2020/8/5, 8:56 PM, "Hohensee, Paul" wrote:
uintx is fine with me.
Thanks,
Paul
On 8/5/20, 1:14 AM, "linzang(臧琳)" wrote:
Hi Stefan,
I got your point, thanks for explanation.
I missed the atomic part when considering it.
Hi Paul,
Do you think it is ok to use uintx? I checked it is
actually a uintptr_t type.
BRs,
Lin
On 2020/8/5, 3:39 PM, "Stefan Karlsson"
wrote:
On 2020-08-05 07:22, linzang(臧琳) wrote:
> Hi Serguei,
>
> No problem, Thanks for your reviewing :)
>
> I wll upload a new webrev later, so may I ask your
help to review it
> again?
>
> Hi Stefan,
>
> As Paul mentioned, the _/missed/_count is not a size,
so size_t may
> not be clear, what’s your opinion about uint64_t?
We typically don't restrict the usage of size_t to only
*sizes* in the
HotSpot. If you search the code you'll find many count
variables using
size_t, so I personally don't see the need to change the
type.
However, if you really do want to change it then maybe
using another
type that is 32 bits on 32-bit machines, maybe uintx? IIRC,
using
uint64_t and some of the Atomics operations are problematic
on some
32-bit platforms, so using a type that matches the word
size of the
targetted machine helps you not having to think about that.
>
> It seems the uint overflow may happened on 64bit
machine with large
> heap, e.g. may be more than 4 Giga objects (8byte header
+ 8 byte
> klassptr + 8byte field) in a heap that is larger than 96
GB, uint64_t
> is ok in this case.
Exactly.
Thanks,
StefanK
>
> BRs,
>
> Lin
>
> *From: *"serguei.spit...@oracle.com"
> *Date: *Wednesday, August 5, 2020 at 1:02 PM
> *To: *"linzang(臧琳)" , "Hohensee,
Paul"
> , Stefan Karlsson
,
> David Holmes , serviceability-dev
> ,
"hotspot-gc-...@openjdk.java.net"
>
> *Subject: *Re: RFR(L): 8215624: add parallel heap
inspection support for
> jmap histo(G1)(Internet mail)
>
> Oh, sorry for the confusion, please, skip my question. :)
> C++ does not have the '&&=' operator.
>
> Thanks,
> Serguei
>
> On 8/4/20 21:56, serguei.spit...@oracle.com
> <mailto:serguei.spit...@oracle.com> wrote:
>
> Hi Lin,
>
>
https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/src/hotspot/share/memory/heapInspection.cpp.udiff.html
>
> +class KlassInfoTableMergeClosure : public
KlassInfoClosure {
>
> +private:
>
> + KlassInfoT
RE: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
Two tiny nits that don't need a new webrev:
In heapInspection.cpp, you don't need to cast missed_count to uintx in the call
to log_info().
In heapInspection.hpp, you can delete two of the three blank lines before
#endif // SHARE_MEMORY_HEAPINSPECTION_HPP
Thanks,
Paul
On 8/5/20, 6:46 AM, "linzang(臧琳)" 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
On 2020/8/5, 8:56 PM, "Hohensee, Paul" wrote:
uintx is fine with me.
Thanks,
Paul
On 8/5/20, 1:14 AM, "linzang(臧琳)" wrote:
Hi Stefan,
I got your point, thanks for explanation.
I missed the atomic part when considering it.
Hi Paul,
Do you think it is ok to use uintx? I checked it is actually a
uintptr_t type.
BRs,
Lin
On 2020/8/5, 3:39 PM, "Stefan Karlsson"
wrote:
On 2020-08-05 07:22, linzang(臧琳) wrote:
> Hi Serguei,
>
> No problem, Thanks for your reviewing :)
>
> I wll upload a new webrev later, so may I ask your help
to review it
> again?
>
> Hi Stefan,
>
> As Paul mentioned, the _/missed/_count is not a size, so
size_t may
> not be clear, what’s your opinion about uint64_t?
We typically don't restrict the usage of size_t to only *sizes*
in the
HotSpot. If you search the code you'll find many count
variables using
size_t, so I personally don't see the need to change the type.
However, if you really do want to change it then maybe using
another
type that is 32 bits on 32-bit machines, maybe uintx? IIRC,
using
uint64_t and some of the Atomics operations are problematic on
some
32-bit platforms, so using a type that matches the word size of
the
targetted machine helps you not having to think about that.
>
> It seems the uint overflow may happened on 64bit machine
with large
> heap, e.g. may be more than 4 Giga objects (8byte header + 8
byte
> klassptr + 8byte field) in a heap that is larger than 96 GB,
uint64_t
> is ok in this case.
Exactly.
Thanks,
StefanK
>
> BRs,
>
> Lin
>
> *From: *"serguei.spit...@oracle.com"
> *Date: *Wednesday, August 5, 2020 at 1:02 PM
> *To: *"linzang(臧琳)" , "Hohensee, Paul"
> , Stefan Karlsson
,
> David Holmes , serviceability-dev
> ,
"hotspot-gc-...@openjdk.java.net"
>
> *Subject: *Re: RFR(L): 8215624: add parallel heap inspection
support for
> jmap histo(G1)(Internet mail)
>
> Oh, sorry for the confusion, please, skip my question. :)
> C++ does not have the '&&=' operator.
>
> Thanks,
> Serguei
>
> On 8/4/20 21:56, serguei.spit...@oracle.com
> <mailto:serguei.spit...@oracle.com> wrote:
>
> Hi Lin,
>
>
https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/src/hotspot/share/memory/heapInspection.cpp.udiff.html
>
> +class KlassInfoTableMergeClosure : public
KlassInfoClosure {
>
> +private:
>
> + KlassInfoTable* _dest;
>
> + bool _success;
>
> +public:
>
> + KlassInfoTableMergeClosure(KlassInfoTable* table) :
_dest(table),
> _success(true) {}
>
> + void do_cinfo(KlassInfoEntry* cie) {
>
> +_success &= _dest->merge_entry(cie);
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
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
On 2020/8/5, 8:56 PM, "Hohensee, Paul" wrote:
uintx is fine with me.
Thanks,
Paul
On 8/5/20, 1:14 AM, "linzang(臧琳)" wrote:
Hi Stefan,
I got your point, thanks for explanation.
I missed the atomic part when considering it.
Hi Paul,
Do you think it is ok to use uintx? I checked it is actually a
uintptr_t type.
BRs,
Lin
On 2020/8/5, 3:39 PM, "Stefan Karlsson"
wrote:
On 2020-08-05 07:22, linzang(臧琳) wrote:
> Hi Serguei,
>
> No problem, Thanks for your reviewing :)
>
> I wll upload a new webrev later, so may I ask your help to
review it
> again?
>
> Hi Stefan,
>
> As Paul mentioned, the _/missed/_count is not a size, so
size_t may
> not be clear, what’s your opinion about uint64_t?
We typically don't restrict the usage of size_t to only *sizes* in
the
HotSpot. If you search the code you'll find many count variables
using
size_t, so I personally don't see the need to change the type.
However, if you really do want to change it then maybe using another
type that is 32 bits on 32-bit machines, maybe uintx? IIRC, using
uint64_t and some of the Atomics operations are problematic on some
32-bit platforms, so using a type that matches the word size of the
targetted machine helps you not having to think about that.
>
> It seems the uint overflow may happened on 64bit machine with
large
> heap, e.g. may be more than 4 Giga objects (8byte header + 8 byte
> klassptr + 8byte field) in a heap that is larger than 96 GB,
uint64_t
> is ok in this case.
Exactly.
Thanks,
StefanK
>
> BRs,
>
> Lin
>
> *From: *"serguei.spit...@oracle.com"
> *Date: *Wednesday, August 5, 2020 at 1:02 PM
> *To: *"linzang(臧琳)" , "Hohensee, Paul"
> , Stefan Karlsson
,
> David Holmes , serviceability-dev
> ,
"hotspot-gc-...@openjdk.java.net"
>
> *Subject: *Re: RFR(L): 8215624: add parallel heap inspection
support for
> jmap histo(G1)(Internet mail)
>
> Oh, sorry for the confusion, please, skip my question. :)
> C++ does not have the '&&=' operator.
>
> Thanks,
> Serguei
>
> On 8/4/20 21:56, serguei.spit...@oracle.com
> <mailto:serguei.spit...@oracle.com> wrote:
>
> Hi Lin,
>
>
https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/src/hotspot/share/memory/heapInspection.cpp.udiff.html
>
> +class KlassInfoTableMergeClosure : public KlassInfoClosure {
>
> +private:
>
> + KlassInfoTable* _dest;
>
> + bool _success;
>
> +public:
>
> + KlassInfoTableMergeClosure(KlassInfoTable* table) :
_dest(table),
> _success(true) {}
>
> + void do_cinfo(KlassInfoEntry* cie) {
>
> +_success &= _dest->merge_entry(cie);
>
> + }
>
> The operator '&=' above looks strange.
> Did you actually want to use the operator '&&=' instead? :
>
> +_success &&= _dest->merge_entry(cie);
>
>
> Thanks,
> Serguei
>
>
>
>
> On 8/3/20 07:51, linzang(臧琳) wrote:
>
> Dear Stefan,
>
> May I ask your help to review again? I have
made a delta based
RE: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
uintx is fine with me.
Thanks,
Paul
On 8/5/20, 1:14 AM, "linzang(臧琳)" wrote:
Hi Stefan,
I got your point, thanks for explanation.
I missed the atomic part when considering it.
Hi Paul,
Do you think it is ok to use uintx? I checked it is actually a
uintptr_t type.
BRs,
Lin
On 2020/8/5, 3:39 PM, "Stefan Karlsson" wrote:
On 2020-08-05 07:22, linzang(臧琳) wrote:
> Hi Serguei,
>
> No problem, Thanks for your reviewing :)
>
> I wll upload a new webrev later, so may I ask your help to review
it
> again?
>
> Hi Stefan,
>
> As Paul mentioned, the _/missed/_count is not a size, so size_t
may
> not be clear, what’s your opinion about uint64_t?
We typically don't restrict the usage of size_t to only *sizes* in the
HotSpot. If you search the code you'll find many count variables using
size_t, so I personally don't see the need to change the type.
However, if you really do want to change it then maybe using another
type that is 32 bits on 32-bit machines, maybe uintx? IIRC, using
uint64_t and some of the Atomics operations are problematic on some
32-bit platforms, so using a type that matches the word size of the
targetted machine helps you not having to think about that.
>
> It seems the uint overflow may happened on 64bit machine with
large
> heap, e.g. may be more than 4 Giga objects (8byte header + 8 byte
> klassptr + 8byte field) in a heap that is larger than 96 GB, uint64_t
> is ok in this case.
Exactly.
Thanks,
StefanK
>
> BRs,
>
> Lin
>
> *From: *"serguei.spit...@oracle.com"
> *Date: *Wednesday, August 5, 2020 at 1:02 PM
> *To: *"linzang(臧琳)" , "Hohensee, Paul"
> , Stefan Karlsson ,
> David Holmes , serviceability-dev
> ,
"hotspot-gc-...@openjdk.java.net"
>
> *Subject: *Re: RFR(L): 8215624: add parallel heap inspection support
for
> jmap histo(G1)(Internet mail)
>
> Oh, sorry for the confusion, please, skip my question. :)
> C++ does not have the '&&=' operator.
>
> Thanks,
> Serguei
>
> On 8/4/20 21:56, serguei.spit...@oracle.com
> <mailto:serguei.spit...@oracle.com> wrote:
>
> Hi Lin,
>
>
https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/src/hotspot/share/memory/heapInspection.cpp.udiff.html
>
> +class KlassInfoTableMergeClosure : public KlassInfoClosure {
>
> +private:
>
> + KlassInfoTable* _dest;
>
> + bool _success;
>
> +public:
>
> + KlassInfoTableMergeClosure(KlassInfoTable* table) :
_dest(table),
> _success(true) {}
>
> + void do_cinfo(KlassInfoEntry* cie) {
>
> +_success &= _dest->merge_entry(cie);
>
> + }
>
> The operator '&=' above looks strange.
> Did you actually want to use the operator '&&=' instead? :
>
> +_success &&= _dest->merge_entry(cie);
>
>
> Thanks,
> Serguei
>
>
>
>
> On 8/3/20 07:51, linzang(臧琳) wrote:
>
> Dear Stefan,
>
> May I ask your help to review again? I have made a
delta based on the last changeset you have reviewed(webrev04),hope it could
ease your reviewing work.
>
>
webrev:https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/
>
> delta (vs
webrev04):https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/delta_10vs04/webrev/
>
> bug:https://bugs.openjdk.java.net/browse/JDK-8215624
>
>
CSR(approved):https://bugs.openjdk.java.net/browse/JDK-8239290
>
>
>
> BRs,
>
> Lin
>
> On 2020/7/30, 5:21 AM, "Hohensee, Paul"
<mailto:hohen...@amazon.com> wrote:
>
> A submit repo run with this succeeded, so afaic you're
good to go. Stefan, you reviewed the GC part before, it'd be gre
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
Hi Stefan,
I got your point, thanks for explanation.
I missed the atomic part when considering it.
Hi Paul,
Do you think it is ok to use uintx? I checked it is actually a uintptr_t
type.
BRs,
Lin
On 2020/8/5, 3:39 PM, "Stefan Karlsson" wrote:
On 2020-08-05 07:22, linzang(臧琳) wrote:
> Hi Serguei,
>
> No problem, Thanks for your reviewing :)
>
> I wll upload a new webrev later, so may I ask your help to review it
> again?
>
> Hi Stefan,
>
> As Paul mentioned, the _/missed/_count is not a size, so size_t may
> not be clear, what’s your opinion about uint64_t?
We typically don't restrict the usage of size_t to only *sizes* in the
HotSpot. If you search the code you'll find many count variables using
size_t, so I personally don't see the need to change the type.
However, if you really do want to change it then maybe using another
type that is 32 bits on 32-bit machines, maybe uintx? IIRC, using
uint64_t and some of the Atomics operations are problematic on some
32-bit platforms, so using a type that matches the word size of the
targetted machine helps you not having to think about that.
>
> It seems the uint overflow may happened on 64bit machine with large
> heap, e.g. may be more than 4 Giga objects (8byte header + 8 byte
> klassptr + 8byte field) in a heap that is larger than 96 GB, uint64_t
> is ok in this case.
Exactly.
Thanks,
StefanK
>
> BRs,
>
> Lin
>
> *From: *"serguei.spit...@oracle.com"
> *Date: *Wednesday, August 5, 2020 at 1:02 PM
> *To: *"linzang(臧琳)" , "Hohensee, Paul"
> , Stefan Karlsson ,
> David Holmes , serviceability-dev
> , "hotspot-gc-...@openjdk.java.net"
>
> *Subject: *Re: RFR(L): 8215624: add parallel heap inspection support for
> jmap histo(G1)(Internet mail)
>
> Oh, sorry for the confusion, please, skip my question. :)
> C++ does not have the '&&=' operator.
>
> Thanks,
> Serguei
>
> On 8/4/20 21:56, serguei.spit...@oracle.com
> <mailto:serguei.spit...@oracle.com> wrote:
>
> Hi Lin,
>
>
https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/src/hotspot/share/memory/heapInspection.cpp.udiff.html
>
> +class KlassInfoTableMergeClosure : public KlassInfoClosure {
>
> +private:
>
> + KlassInfoTable* _dest;
>
> + bool _success;
>
> +public:
>
> + KlassInfoTableMergeClosure(KlassInfoTable* table) : _dest(table),
> _success(true) {}
>
> + void do_cinfo(KlassInfoEntry* cie) {
>
> +_success &= _dest->merge_entry(cie);
>
> + }
>
> The operator '&=' above looks strange.
> Did you actually want to use the operator '&&=' instead? :
>
> +_success &&= _dest->merge_entry(cie);
>
>
> Thanks,
> Serguei
>
>
>
>
> On 8/3/20 07:51, linzang(臧琳) wrote:
>
> Dear Stefan,
>
> May I ask your help to review again? I have made a
delta based on the last changeset you have reviewed(webrev04),hope it could
ease your reviewing work.
>
>
webrev:https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/
>
> delta (vs
webrev04):https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/delta_10vs04/webrev/
>
> bug:https://bugs.openjdk.java.net/browse/JDK-8215624
>
>
CSR(approved):https://bugs.openjdk.java.net/browse/JDK-8239290
>
>
>
> BRs,
>
> Lin
>
> On 2020/7/30, 5:21 AM, "Hohensee, Paul"
<mailto:hohen...@amazon.com> wrote:
>
> A submit repo run with this succeeded, so afaic you're good
to go. Stefan, you reviewed the GC part before, it'd be great if you could ok
the final version.
>
> Thanks,
>
> Paul
>
> On 7/29/20, 5:02 AM, "linzang(臧琳)"
<mailto:linz...@tencent.com> wrote:
>
> Upload a new change
athttp://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/
>
>
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
On 2020-08-05 07:22, linzang(臧琳) wrote:
Hi Serguei,
No problem, Thanks for your reviewing :)
I wll upload a new webrev later, so may I ask your help to review it
again?
Hi Stefan,
As Paul mentioned, the _/missed/_count is not a size, so size_t may
not be clear, what’s your opinion about uint64_t?
We typically don't restrict the usage of size_t to only *sizes* in the
HotSpot. If you search the code you'll find many count variables using
size_t, so I personally don't see the need to change the type.
However, if you really do want to change it then maybe using another
type that is 32 bits on 32-bit machines, maybe uintx? IIRC, using
uint64_t and some of the Atomics operations are problematic on some
32-bit platforms, so using a type that matches the word size of the
targetted machine helps you not having to think about that.
It seems the uint overflow may happened on 64bit machine with large
heap, e.g. may be more than 4 Giga objects (8byte header + 8 byte
klassptr + 8byte field) in a heap that is larger than 96 GB, uint64_t
is ok in this case.
Exactly.
Thanks,
StefanK
BRs,
Lin
*From: *"serguei.spit...@oracle.com"
*Date: *Wednesday, August 5, 2020 at 1:02 PM
*To: *"linzang(臧琳)" , "Hohensee, Paul"
, Stefan Karlsson ,
David Holmes , serviceability-dev
, "hotspot-gc-...@openjdk.java.net"
*Subject: *Re: RFR(L): 8215624: add parallel heap inspection support for
jmap histo(G1)(Internet mail)
Oh, sorry for the confusion, please, skip my question. :)
C++ does not have the '&&=' operator.
Thanks,
Serguei
On 8/4/20 21:56, serguei.spit...@oracle.com
<mailto:serguei.spit...@oracle.com> wrote:
Hi Lin,
https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/src/hotspot/share/memory/heapInspection.cpp.udiff.html
+class KlassInfoTableMergeClosure : public KlassInfoClosure {
+private:
+ KlassInfoTable* _dest;
+ bool _success;
+public:
+ KlassInfoTableMergeClosure(KlassInfoTable* table) : _dest(table),
_success(true) {}
+ void do_cinfo(KlassInfoEntry* cie) {
+ _success &= _dest->merge_entry(cie);
+ }
The operator '&=' above looks strange.
Did you actually want to use the operator '&&=' instead? :
+ _success &&= _dest->merge_entry(cie);
Thanks,
Serguei
On 8/3/20 07:51, linzang(臧琳) wrote:
Dear Stefan,
May I ask your help to review again? I have made a delta
based on the last changeset you have reviewed(webrev04),hope it could ease your
reviewing work.
webrev:https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/
delta (vs
webrev04):https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/delta_10vs04/webrev/
bug:https://bugs.openjdk.java.net/browse/JDK-8215624
CSR(approved):https://bugs.openjdk.java.net/browse/JDK-8239290
BRs,
Lin
On 2020/7/30, 5:21 AM, "Hohensee, Paul"
<mailto:hohen...@amazon.com> wrote:
A submit repo run with this succeeded, so afaic you're good to go.
Stefan, you reviewed the GC part before, it'd be great if you could ok the
final version.
Thanks,
Paul
On 7/29/20, 5:02 AM, "linzang(臧琳)"
<mailto:linz...@tencent.com> wrote:
Upload a new change
athttp://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/
It fix an issue of windows fail :
In heapInspect.cpp
- size_t HeapInspection::populate_table(KlassInfoTable* cit,
BoolObjectClosure *filter, uint parallel_thread_num) {
+ uint HeapInspection::populate_table(KlassInfoTable* cit,
BoolObjectClosure *filter, uint parallel_thread_num) {
In heapInspect.hpp
- size_t populate_table(KlassInfoTable* cit,
BoolObjectClosure* filter = NULL, uint parallel_thread_num = 1)
NOT_SERVICES_RETURN_(0);
+ uint populate_table(KlassInfoTable* cit, BoolObjectClosure*
filter = NULL, uint parallel_thread_num = 1) NOT_SERVICES_RETURN_(0);
BRs,
Lin
On 2020/7/27, 11:26 AM, "linzang(臧琳)"
<mailto:linz...@tencent.com> wrote:
I update a new change
athttp://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_09
It includes a tiny fix of build failure on windows:
In attachListener.cpp:
- uint parallel_thread_num = MAX(1,
(uint)os::initial_active_process
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
Hi Lin,
On 8/4/20 22:22, linzang(臧琳) wrote:
Hi Serguei,
No problem, Thanks for
your reviewing :)
I wll upload a new
webrev later, so may I ask your help to review it again?
Yes, of course.
Thanks,
Serguei
Hi Stefan,
As Paul mentioned,
the _missed_count is not a size, so size_t may not
be clear, what’s your opinion about uint64_t?
It seems the uint
overflow may happened on 64bit machine with large heap,
e.g. may be more than 4 Giga objects (8byte header + 8
byte klassptr + 8byte field) in a heap that is larger than
96 GB, uint64_t is ok in this case.
BRs,
Lin
From:
"serguei.spit...@oracle.com"
Date: Wednesday, August 5, 2020 at 1:02 PM
To: "linzang(臧琳)"
, "Hohensee, Paul"
, Stefan Karlsson
, David Holmes
, serviceability-dev
,
"hotspot-gc-...@openjdk.java.net"
Subject: Re: RFR(L): 8215624: add parallel heap
inspection support for jmap histo(G1)(Internet mail)
Oh, sorry for the confusion, please, skip
my question. :)
C++ does not have the '&&=' operator.
Thanks,
Serguei
On 8/4/20 21:56, serguei.spit...@oracle.com
wrote:
Hi Lin,
https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/src/hotspot/share/memory/heapInspection.cpp.udiff.html
+class KlassInfoTableMergeClosure : public KlassInfoClosure {
+private:
+ KlassInfoTable* _dest;
+ bool _success;
+public:
+ KlassInfoTableMergeClosure(KlassInfoTable* table) : _dest(table), _success(true) {}
+ void do_cinfo(KlassInfoEntry* cie) {
+ _success &= _dest->merge_entry(cie);
+ }
The operator '&=' above looks
strange.
Did you actually want to use the operator '&&='
instead? :
+ _success &&= _dest->merge_entry(cie);
Thanks,
Serguei
On 8/3/20 07:51, linzang(臧琳)
wrote:
Dear Stefan,
May I ask your help to review again? I have made a delta based on the last changeset you have reviewed(webrev04),hope it could ease your reviewing work.
webrev: https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/
delta (vs webrev04): https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/delta_10vs04/webrev/
bug: https://bugs.openjdk.java.net/browse/JDK-8215624
CSR(approved): https://bugs.openjdk.java.net/browse/JDK-8239290
BRs,
Lin
On 2020/7/30, 5:21 AM, "Hohensee, Paul" wrote:
A submit repo run with this succeeded, so afaic you're good to go. Stefan, you reviewed the GC part before, it'd be great if you could ok the final version.
Thanks,
Paul
On 7/29/20, 5:02 AM, "linzang(臧琳)" wrote:
Upload a new change at http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/
It fix an issue of windows fail :
In heapInspect.cpp
- size_t HeapInspection::populate_table(KlassInfoTable* cit, BoolObjectClosure *filter, uint parallel_thread_num) {
+ uint HeapInspection::populate_table(KlassInfoTable* cit, BoolObjectClosure *filter, uint parallel_thread_num) {
In heapInspect.hpp
- size_t populate_table(KlassInfoTable* cit, BoolObjectClosure* filter = NULL, uint parallel_thread_num = 1) NOT_SERVICES_RETURN_(0);
+ uint populate_table(KlassInfoTable* cit, BoolObjectClosure* filter = NULL, uint parallel_thread_num = 1) NOT_SERVICES_RETURN_(0);
BRs,
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
M
To: "linzang(臧琳)" , Stefan Karlsson , "serguei.spit...@oracle.com" , David Holmes , serviceability-dev , "hotspot-gc-...@openjdk.java.net"
Subject: RE: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
Just small things.
heapInspection.cpp:
In ParHeapInspectTask::work, remove the final return statement and fix the following ‘}’ indent. I.e., replace
+Atomic::store(&_success, false);
+return;
+ }
with
+Atomic::store(&_success, false);
+ }
In HeapInspection::heap_inspection, missed_count should be a uint to match other missed_count declarations, and should be initialized to the result of populate_table() rather than separately to 0.
attachListener.cpp:
In heap_inspection, initial_processor_count returns an int, so cast its result to a uint.
Similarly, parse_uintx returns a uintx, so cast its result (num) to uint when assigning to parallel_thread_num.
BasicJMapTest.java:
I took the liberty of refactoring testHisto*/histoToFile/testDump*/dump to remove redundant interposition methods and make histoToFile and dump look as similar as possible.
Webrev with the above changes in
http://cr.openjdk.java.net/~phh/8214535/webrev.01/
Thanks,
Paul
On 7/15/20, 2:13 AM, "linzang(臧琳)" wrote:
Upload a new webrev at http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_07/
It fix a potential issue that unexpected number of threads maybe calculated for "parallel" option of jmap -histo in container.
As shown at http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_07-delta/src/hotspot/share/services/attachListener.cpp.udiff.html
### attachListener.cpp
@@ -252,11 +252,11 @@
static jint heap_inspection(AttachOperation* op, outputStream* out) {
bool live_objects_only = true; // default is true to retain the behavior before this change is made
outputStream* os = out; // if path not specified or path is NULL, use out
fileStream* fs = NULL;
const char* arg0 = op->arg(0);
- uint parallel_thread_num = MAX(1, os::processor_count() * 3 / 8); // default is less than half of processors.
+ uint parallel_thread_num = MAX(1, os::initial_active_processor_count() * 3 / 8); // default is less than half of processors.
if (arg0 != NULL && (strlen(arg0) > 0)) {
if (strcmp(arg0, "-all") != 0 && strcmp(arg0, "-live") != 0) {
out->print_cr("Invalid argument to inspectheap operation: %s", arg0);
return JNI_ERR;
}
###
Thanks.
BRs,
Lin
On 2020/7/9, 3:22 PM, "linzang(臧琳)" wrote:
Hi Paul,
Thanks for reviewing!
>>
>> I'd move all the argument parsing code to JMap.java and just pass the results to Hotspot. Both histo() in JMap.java and code in attachListener.* parse the command line arguments, though the code in histo() doesn't parse the argument to "parallel". I'd upgrade the code in histo() to do a complete parse and pass the option values to executeCommandForPid as before: there would just be more of them now. That would allow you to eliminate all the parsing code in attachListener.cpp as well as the change to arguments.hpp.
>>
The reason I made the change in Jmap.java that compose all arguments as 1 string , instead of passing 3 argments, is to avoid the compatibility issue, as we discussed in http://mail.openjdk.java.net/pipermail/serviceability-dev/2019-February/thread.html#27240. The root cause of the compatibility issue is because max argument count in HotspotVirtualMachineImpl.java and attachlistener.cpp need to be enlarged (changes like http://hg.openjdk.java.net/jdk/jdk/rev/e7cf035682e3#l2.1) when jmap has more than 3 arguments. But if user use an old jcmd/jmap tool, it may stuck at socket read(), because the "max argument count" don't match.
I re-checked this change, the argument count of jmap histo is equal to 3
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
Hi Lin,
https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/src/hotspot/share/memory/heapInspection.cpp.udiff.html
+class KlassInfoTableMergeClosure : public KlassInfoClosure {
+private:
+ KlassInfoTable* _dest;
+ bool _success;
+public:
+ KlassInfoTableMergeClosure(KlassInfoTable* table) : _dest(table), _success(true) {}
+ void do_cinfo(KlassInfoEntry* cie) {
+_success &= _dest->merge_entry(cie);
+ }
The operator '&=' above looks strange.
Did you actually want to use the operator '&&=' instead? :
+_success &&= _dest->merge_entry(cie);
Thanks,
Serguei
On 8/3/20 07:51, linzang(臧琳) wrote:
Dear Stefan,
May I ask your help to review again? I have made a delta based on the last changeset you have reviewed(webrev04),hope it could ease your reviewing work.
webrev: https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/
delta (vs webrev04): https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/delta_10vs04/webrev/
bug: https://bugs.openjdk.java.net/browse/JDK-8215624
CSR(approved): https://bugs.openjdk.java.net/browse/JDK-8239290
BRs,
Lin
On 2020/7/30, 5:21 AM, "Hohensee, Paul" wrote:
A submit repo run with this succeeded, so afaic you're good to go. Stefan, you reviewed the GC part before, it'd be great if you could ok the final version.
Thanks,
Paul
On 7/29/20, 5:02 AM, "linzang(臧琳)" wrote:
Upload a new change at http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/
It fix an issue of windows fail :
In heapInspect.cpp
- size_t HeapInspection::populate_table(KlassInfoTable* cit, BoolObjectClosure *filter, uint parallel_thread_num) {
+ uint HeapInspection::populate_table(KlassInfoTable* cit, BoolObjectClosure *filter, uint parallel_thread_num) {
In heapInspect.hpp
- size_t populate_table(KlassInfoTable* cit, BoolObjectClosure* filter = NULL, uint parallel_thread_num = 1) NOT_SERVICES_RETURN_(0);
+ uint populate_table(KlassInfoTable* cit, BoolObjectClosure* filter = NULL, uint parallel_thread_num = 1) NOT_SERVICES_RETURN_(0);
BRs,
Lin
On 2020/7/27, 11:26 AM, "linzang(臧琳)" wrote:
I update a new change at http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_09
It includes a tiny fix of build failure on windows:
In attachListener.cpp:
- uint parallel_thread_num = MAX(1, (uint)os::initial_active_processor_count() * 3 / 8);
+ uint parallel_thread_num = MAX2(1, (uint)os::initial_active_processor_count() * 3 / 8);
BRs,
Lin
On 2020/7/23, 11:56 AM, "linzang(臧琳)" wrote:
Hi Paul,
Thanks for your help, that all looks good to me.
Just 2 minor changes:
• delete the final return in ParHeapInspectTask::work, you mentioned it but seems not include in the webrev :-)
• delete a unnecessary blank line in heapInspect.cpp at merge_entry()
#
--- old/src/hotspot/share/memory/heapInspection.cpp 2020-07-23 11:23:29.281666456 +0800
+++ new/src/hotspot/share/memory/heapInspection.cpp 2020-07-23 11:23:29.017666447 +0800
@@ -251,7 +251,6 @@
_size_of_instances_in_words += cie->words();
return true;
}
-
return false;
}
@@ -568,7 +567,6 @@
Atomic::add(&_missed_count, missed_count);
} else {
Atomic::store(&_success, false);
- return;
}
}
#
Here is the webrev http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_08/
BRs,
Lin
-
From: "Hohensee, Paul"
Date: Thursday, July 23, 2020 at 6:48 AM
To: "linzang(臧琳)" , Stefan Karlsson , "serguei.spit...@oracle.com" , David Holmes , serviceability-dev , "hotspot-gc-...@openjdk.java.net"
Subject: RE: RFR(L): 8215624: add parallel heap ins
RE: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
rote:
>
> I update a new change at
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_09
> It includes a tiny fix of build failure on windows:
>
> In attachListener.cpp:
> - uint parallel_thread_num = MAX(1,
(uint)os::initial_active_processor_count() * 3 / 8);
> + uint parallel_thread_num = MAX2(1,
(uint)os::initial_active_processor_count() * 3 / 8);
>
>
> BRs,
> Lin
>
> On 2020/7/23, 11:56 AM, "linzang(臧琳)"
wrote:
>
> Hi Paul,
> Thanks for your help, that all looks good to me.
> Just 2 minor changes:
> • delete the final return in
ParHeapInspectTask::work, you mentioned it but seems not include in the webrev
:-)
> • delete a unnecessary blank line in
heapInspect.cpp at merge_entry()
>
>
#
> --- old/src/hotspot/share/memory/heapInspection.cpp
2020-07-23 11:23:29.281666456 +0800
> +++ new/src/hotspot/share/memory/heapInspection.cpp
2020-07-23 11:23:29.017666447 +0800
> @@ -251,7 +251,6 @@
> _size_of_instances_in_words += cie->words();
> return true;
> }
> -
> return false;
> }
>
> @@ -568,7 +567,6 @@
> Atomic::add(&_missed_count, missed_count);
> } else {
> Atomic::store(&_success, false);
> - return;
> }
> }
>
#
>
>
> Here is the webrev
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_08/
>
> BRs,
> Lin
> -------------------------
> From: "Hohensee, Paul"
> Date: Thursday, July 23, 2020 at 6:48 AM
> To: "linzang(臧琳)" , Stefan Karlsson
, "serguei.spit...@oracle.com"
, David Holmes ,
serviceability-dev ,
"hotspot-gc-...@openjdk.java.net"
> Subject: RE: RFR(L): 8215624: add parallel heap
inspection support for jmap histo(G1)(Internet mail)
>
> Just small things.
>
> heapInspection.cpp:
>
> In ParHeapInspectTask::work, remove the final return
statement and fix the following ‘}’ indent. I.e., replace
>
> +Atomic::store(&_success, false);
> +return;
> + }
>
> with
>
> +Atomic::store(&_success, false);
> + }
>
> In HeapInspection::heap_inspection, missed_count should
be a uint to match other missed_count declarations, and should be initialized
to the result of populate_table() rather than separately to 0.
>
> attachListener.cpp:
>
> In heap_inspection, initial_processor_count returns an
int, so cast its result to a uint.
>
> Similarly, parse_uintx returns a uintx, so cast its
result (num) to uint when assigning to parallel_thread_num.
>
> BasicJMapTest.java:
>
> I took the liberty of refactoring
testHisto*/histoToFile/testDump*/dump to remove redundant interposition methods
and make histoToFile and dump look as similar as possible.
>
> Webrev with the above changes in
>
> http://cr.openjdk.java.net/~phh/8214535/webrev.01/
>
> Thanks,
> Paul
>
> On 7/15/20, 2:13 AM, "linzang(臧琳)"
wrote:
>
> Upload a new webrev at
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_07/
> It fix a potential issue that unexpected number of
threads maybe calculated for "parallel" option of jmap -histo in container.
> As shown at
http://cr.openjdk.java.net/~lz
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
inal return in ParHeapInspectTask::work,
you mentioned it but seems not include in the webrev :-)
• delete a unnecessary blank line in heapInspect.cpp
at merge_entry()
#
--- old/src/hotspot/share/memory/heapInspection.cpp
2020-07-23 11:23:29.281666456 +0800
+++ new/src/hotspot/share/memory/heapInspection.cpp
2020-07-23 11:23:29.017666447 +0800
@@ -251,7 +251,6 @@
_size_of_instances_in_words += cie->words();
return true;
}
-
return false;
}
@@ -568,7 +567,6 @@
Atomic::add(&_missed_count, missed_count);
} else {
Atomic::store(&_success, false);
- return;
}
}
#
Here is the webrev
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_08/
BRs,
Lin
-
From: "Hohensee, Paul"
Date: Thursday, July 23, 2020 at 6:48 AM
To: "linzang(臧琳)" , Stefan Karlsson ,
"serguei.spit...@oracle.com" , David Holmes , serviceability-dev
, "hotspot-gc-...@openjdk.java.net"
Subject: RE: RFR(L): 8215624: add parallel heap inspection
support for jmap histo(G1)(Internet mail)
Just small things.
heapInspection.cpp:
In ParHeapInspectTask::work, remove the final return statement
and fix the following ‘}’ indent. I.e., replace
+Atomic::store(&_success, false);
+return;
+ }
with
+Atomic::store(&_success, false);
+ }
In HeapInspection::heap_inspection, missed_count should be a
uint to match other missed_count declarations, and should be initialized to the
result of populate_table() rather than separately to 0.
attachListener.cpp:
In heap_inspection, initial_processor_count returns an int, so
cast its result to a uint.
Similarly, parse_uintx returns a uintx, so cast its result
(num) to uint when assigning to parallel_thread_num.
BasicJMapTest.java:
I took the liberty of refactoring
testHisto*/histoToFile/testDump*/dump to remove redundant interposition methods
and make histoToFile and dump look as similar as possible.
Webrev with the above changes in
http://cr.openjdk.java.net/~phh/8214535/webrev.01/
Thanks,
Paul
On 7/15/20, 2:13 AM, "linzang(臧琳)" wrote:
Upload a new webrev at
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_07/
It fix a potential issue that unexpected number of threads maybe
calculated for "parallel" option of jmap -histo in container.
As shown at
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_07-delta/src/hotspot/share/services/attachListener.cpp.udiff.html
### attachListener.cpp
@@ -252,11 +252,11 @@
static jint heap_inspection(AttachOperation* op,
outputStream* out) {
bool live_objects_only = true; // default is true to
retain the behavior before this change is made
outputStream* os = out; // if path not specified or
path is NULL, use out
fileStream* fs = NULL;
const char* arg0 = op->arg(0);
- uint parallel_thread_num = MAX(1, os::processor_count()
* 3 / 8); // default is less than half of processors.
+ uint parallel_thread_num = MAX(1,
os::initial_active_processor_count() * 3 / 8); // default is less than half of
processors.
if (arg0 != NULL && (strlen(arg0) > 0)) {
if (strcmp(arg0, "-all") != 0 && strcmp(arg0,
"-live") != 0) {
out->print_cr("Invalid argument to inspectheap
operation: %s", arg0);
return JNI_ERR;
}
###
Thanks.
BRs,
Lin
On 2020/7/9, 3:22 PM, "linzang(臧琳)"
wrote:
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
Dear Stefan,
May I ask your help to review again? I have made a delta based on the
last changeset you have reviewed(webrev04),hope it could ease your reviewing
work.
webrev:
https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/
delta (vs webrev04):
https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/delta_10vs04/webrev/
bug: https://bugs.openjdk.java.net/browse/JDK-8215624
CSR(approved): https://bugs.openjdk.java.net/browse/JDK-8239290
BRs,
Lin
On 2020/7/30, 5:21 AM, "Hohensee, Paul" wrote:
A submit repo run with this succeeded, so afaic you're good to go. Stefan,
you reviewed the GC part before, it'd be great if you could ok the final
version.
Thanks,
Paul
On 7/29/20, 5:02 AM, "linzang(臧琳)" wrote:
Upload a new change at
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/
It fix an issue of windows fail :
In heapInspect.cpp
- size_t HeapInspection::populate_table(KlassInfoTable* cit,
BoolObjectClosure *filter, uint parallel_thread_num) {
+ uint HeapInspection::populate_table(KlassInfoTable* cit,
BoolObjectClosure *filter, uint parallel_thread_num) {
In heapInspect.hpp
- size_t populate_table(KlassInfoTable* cit, BoolObjectClosure* filter
= NULL, uint parallel_thread_num = 1) NOT_SERVICES_RETURN_(0);
+ uint populate_table(KlassInfoTable* cit, BoolObjectClosure* filter =
NULL, uint parallel_thread_num = 1) NOT_SERVICES_RETURN_(0);
BRs,
Lin
On 2020/7/27, 11:26 AM, "linzang(臧琳)" wrote:
I update a new change at
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_09
It includes a tiny fix of build failure on windows:
In attachListener.cpp:
- uint parallel_thread_num = MAX(1,
(uint)os::initial_active_processor_count() * 3 / 8);
+ uint parallel_thread_num = MAX2(1,
(uint)os::initial_active_processor_count() * 3 / 8);
BRs,
Lin
On 2020/7/23, 11:56 AM, "linzang(臧琳)" wrote:
Hi Paul,
Thanks for your help, that all looks good to me.
Just 2 minor changes:
• delete the final return in ParHeapInspectTask::work,
you mentioned it but seems not include in the webrev :-)
• delete a unnecessary blank line in heapInspect.cpp at
merge_entry()
#
--- old/src/hotspot/share/memory/heapInspection.cpp
2020-07-23 11:23:29.281666456 +0800
+++ new/src/hotspot/share/memory/heapInspection.cpp
2020-07-23 11:23:29.017666447 +0800
@@ -251,7 +251,6 @@
_size_of_instances_in_words += cie->words();
return true;
}
-
return false;
}
@@ -568,7 +567,6 @@
Atomic::add(&_missed_count, missed_count);
} else {
Atomic::store(&_success, false);
- return;
}
}
#
Here is the webrev
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_08/
BRs,
Lin
-
From: "Hohensee, Paul"
Date: Thursday, July 23, 2020 at 6:48 AM
To: "linzang(臧琳)" , Stefan Karlsson
, "serguei.spit...@oracle.com"
, David Holmes ,
serviceability-dev ,
"hotspot-gc-...@openjdk.java.net"
Subject: RE: RFR(L): 8215624: add parallel heap inspection
support for jmap histo(G1)(Internet mail)
Just small things.
heapInspection.cpp:
In ParHeapInspectTask::work, remove the final return statement
and fix the following ‘}’ indent. I.e., replace
+Atomic::store(&_success, false);
+return;
+ }
with
+Atomic::store(&_success, false);
+ }
In HeapInspection::heap_inspection, missed_count should be a
uint to match other missed_count declarations, and should be initialized to the
result of populate_table() rather than separately to 0.
attachListener.cpp:
In heap_inspection, initial_processor_c
RE: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
A submit repo run with this succeeded, so afaic you're good to go. Stefan, you
reviewed the GC part before, it'd be great if you could ok the final version.
Thanks,
Paul
On 7/29/20, 5:02 AM, "linzang(臧琳)" wrote:
Upload a new change at
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/
It fix an issue of windows fail :
In heapInspect.cpp
- size_t HeapInspection::populate_table(KlassInfoTable* cit,
BoolObjectClosure *filter, uint parallel_thread_num) {
+ uint HeapInspection::populate_table(KlassInfoTable* cit,
BoolObjectClosure *filter, uint parallel_thread_num) {
In heapInspect.hpp
- size_t populate_table(KlassInfoTable* cit, BoolObjectClosure* filter =
NULL, uint parallel_thread_num = 1) NOT_SERVICES_RETURN_(0);
+ uint populate_table(KlassInfoTable* cit, BoolObjectClosure* filter =
NULL, uint parallel_thread_num = 1) NOT_SERVICES_RETURN_(0);
BRs,
Lin
On 2020/7/27, 11:26 AM, "linzang(臧琳)" wrote:
I update a new change at
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_09
It includes a tiny fix of build failure on windows:
In attachListener.cpp:
- uint parallel_thread_num = MAX(1,
(uint)os::initial_active_processor_count() * 3 / 8);
+ uint parallel_thread_num = MAX2(1,
(uint)os::initial_active_processor_count() * 3 / 8);
BRs,
Lin
On 2020/7/23, 11:56 AM, "linzang(臧琳)" wrote:
Hi Paul,
Thanks for your help, that all looks good to me.
Just 2 minor changes:
• delete the final return in ParHeapInspectTask::work, you
mentioned it but seems not include in the webrev :-)
• delete a unnecessary blank line in heapInspect.cpp at
merge_entry()
#
--- old/src/hotspot/share/memory/heapInspection.cpp 2020-07-23
11:23:29.281666456 +0800
+++ new/src/hotspot/share/memory/heapInspection.cpp 2020-07-23
11:23:29.017666447 +0800
@@ -251,7 +251,6 @@
_size_of_instances_in_words += cie->words();
return true;
}
-
return false;
}
@@ -568,7 +567,6 @@
Atomic::add(&_missed_count, missed_count);
} else {
Atomic::store(&_success, false);
- return;
}
}
#
Here is the webrev
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_08/
BRs,
Lin
-
From: "Hohensee, Paul"
Date: Thursday, July 23, 2020 at 6:48 AM
To: "linzang(臧琳)" , Stefan Karlsson
, "serguei.spit...@oracle.com"
, David Holmes ,
serviceability-dev ,
"hotspot-gc-...@openjdk.java.net"
Subject: RE: RFR(L): 8215624: add parallel heap inspection support
for jmap histo(G1)(Internet mail)
Just small things.
heapInspection.cpp:
In ParHeapInspectTask::work, remove the final return statement and
fix the following ‘}’ indent. I.e., replace
+Atomic::store(&_success, false);
+return;
+ }
with
+Atomic::store(&_success, false);
+ }
In HeapInspection::heap_inspection, missed_count should be a uint
to match other missed_count declarations, and should be initialized to the
result of populate_table() rather than separately to 0.
attachListener.cpp:
In heap_inspection, initial_processor_count returns an int, so cast
its result to a uint.
Similarly, parse_uintx returns a uintx, so cast its result (num) to
uint when assigning to parallel_thread_num.
BasicJMapTest.java:
I took the liberty of refactoring
testHisto*/histoToFile/testDump*/dump to remove redundant interposition methods
and make histoToFile and dump look as similar as possible.
Webrev with the above changes in
http://cr.openjdk.java.net/~phh/8214535/webrev.01/
Thanks,
Paul
On 7/15/20, 2:13 AM, "linzang(臧琳)" wrote:
Upload a new webrev at
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_07/
It fix a potential issue that unexpected number of threads
maybe calculated for "parallel" option of jmap -hist
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
Upload a new change at
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/
It fix an issue of windows fail :
In heapInspect.cpp
- size_t HeapInspection::populate_table(KlassInfoTable* cit, BoolObjectClosure
*filter, uint parallel_thread_num) {
+ uint HeapInspection::populate_table(KlassInfoTable* cit, BoolObjectClosure
*filter, uint parallel_thread_num) {
In heapInspect.hpp
- size_t populate_table(KlassInfoTable* cit, BoolObjectClosure* filter = NULL,
uint parallel_thread_num = 1) NOT_SERVICES_RETURN_(0);
+ uint populate_table(KlassInfoTable* cit, BoolObjectClosure* filter = NULL,
uint parallel_thread_num = 1) NOT_SERVICES_RETURN_(0);
BRs,
Lin
On 2020/7/27, 11:26 AM, "linzang(臧琳)" wrote:
I update a new change at
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_09
It includes a tiny fix of build failure on windows:
In attachListener.cpp:
- uint parallel_thread_num = MAX(1,
(uint)os::initial_active_processor_count() * 3 / 8);
+ uint parallel_thread_num = MAX2(1,
(uint)os::initial_active_processor_count() * 3 / 8);
BRs,
Lin
On 2020/7/23, 11:56 AM, "linzang(臧琳)" wrote:
Hi Paul,
Thanks for your help, that all looks good to me.
Just 2 minor changes:
• delete the final return in ParHeapInspectTask::work, you
mentioned it but seems not include in the webrev :-)
• delete a unnecessary blank line in heapInspect.cpp at
merge_entry()
#
--- old/src/hotspot/share/memory/heapInspection.cpp 2020-07-23
11:23:29.281666456 +0800
+++ new/src/hotspot/share/memory/heapInspection.cpp 2020-07-23
11:23:29.017666447 +0800
@@ -251,7 +251,6 @@
_size_of_instances_in_words += cie->words();
return true;
}
-
return false;
}
@@ -568,7 +567,6 @@
Atomic::add(&_missed_count, missed_count);
} else {
Atomic::store(&_success, false);
- return;
}
}
#
Here is the webrev
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_08/
BRs,
Lin
-
From: "Hohensee, Paul"
Date: Thursday, July 23, 2020 at 6:48 AM
To: "linzang(臧琳)" , Stefan Karlsson
, "serguei.spit...@oracle.com"
, David Holmes ,
serviceability-dev ,
"hotspot-gc-...@openjdk.java.net"
Subject: RE: RFR(L): 8215624: add parallel heap inspection support for
jmap histo(G1)(Internet mail)
Just small things.
heapInspection.cpp:
In ParHeapInspectTask::work, remove the final return statement and fix
the following ‘}’ indent. I.e., replace
+Atomic::store(&_success, false);
+return;
+ }
with
+Atomic::store(&_success, false);
+ }
In HeapInspection::heap_inspection, missed_count should be a uint to
match other missed_count declarations, and should be initialized to the result
of populate_table() rather than separately to 0.
attachListener.cpp:
In heap_inspection, initial_processor_count returns an int, so cast its
result to a uint.
Similarly, parse_uintx returns a uintx, so cast its result (num) to
uint when assigning to parallel_thread_num.
BasicJMapTest.java:
I took the liberty of refactoring testHisto*/histoToFile/testDump*/dump
to remove redundant interposition methods and make histoToFile and dump look as
similar as possible.
Webrev with the above changes in
http://cr.openjdk.java.net/~phh/8214535/webrev.01/
Thanks,
Paul
On 7/15/20, 2:13 AM, "linzang(臧琳)" wrote:
Upload a new webrev at
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_07/
It fix a potential issue that unexpected number of threads maybe
calculated for "parallel" option of jmap -histo in container.
As shown at
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_07-delta/src/hotspot/share/services/attachListener.cpp.udiff.html
### attachListener.cpp
@@ -252,11 +252,11 @@
static jint heap_inspection(AttachOperation* op, outputStream*
out) {
bool live_objects_only = true; // default is true to retain
the behavior before this change is made
outputStream* os = out; // if path not
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
I update a new change at
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_09
It includes a tiny fix of build failure on windows:
In attachListener.cpp:
- uint parallel_thread_num = MAX(1, (uint)os::initial_active_processor_count()
* 3 / 8);
+ uint parallel_thread_num = MAX2(1,
(uint)os::initial_active_processor_count() * 3 / 8);
BRs,
Lin
On 2020/7/23, 11:56 AM, "linzang(臧琳)" wrote:
Hi Paul,
Thanks for your help, that all looks good to me.
Just 2 minor changes:
• delete the final return in ParHeapInspectTask::work, you mentioned it
but seems not include in the webrev :-)
• delete a unnecessary blank line in heapInspect.cpp at merge_entry()
#
--- old/src/hotspot/share/memory/heapInspection.cpp 2020-07-23
11:23:29.281666456 +0800
+++ new/src/hotspot/share/memory/heapInspection.cpp 2020-07-23
11:23:29.017666447 +0800
@@ -251,7 +251,6 @@
_size_of_instances_in_words += cie->words();
return true;
}
-
return false;
}
@@ -568,7 +567,6 @@
Atomic::add(&_missed_count, missed_count);
} else {
Atomic::store(&_success, false);
- return;
}
}
#
Here is the webrev
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_08/
BRs,
Lin
-
From: "Hohensee, Paul"
Date: Thursday, July 23, 2020 at 6:48 AM
To: "linzang(臧琳)" , Stefan Karlsson
, "serguei.spit...@oracle.com"
, David Holmes ,
serviceability-dev ,
"hotspot-gc-...@openjdk.java.net"
Subject: RE: RFR(L): 8215624: add parallel heap inspection support for jmap
histo(G1)(Internet mail)
Just small things.
heapInspection.cpp:
In ParHeapInspectTask::work, remove the final return statement and fix the
following ‘}’ indent. I.e., replace
+Atomic::store(&_success, false);
+return;
+ }
with
+Atomic::store(&_success, false);
+ }
In HeapInspection::heap_inspection, missed_count should be a uint to match
other missed_count declarations, and should be initialized to the result of
populate_table() rather than separately to 0.
attachListener.cpp:
In heap_inspection, initial_processor_count returns an int, so cast its
result to a uint.
Similarly, parse_uintx returns a uintx, so cast its result (num) to uint
when assigning to parallel_thread_num.
BasicJMapTest.java:
I took the liberty of refactoring testHisto*/histoToFile/testDump*/dump to
remove redundant interposition methods and make histoToFile and dump look as
similar as possible.
Webrev with the above changes in
http://cr.openjdk.java.net/~phh/8214535/webrev.01/
Thanks,
Paul
On 7/15/20, 2:13 AM, "linzang(臧琳)" wrote:
Upload a new webrev at
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_07/
It fix a potential issue that unexpected number of threads maybe
calculated for "parallel" option of jmap -histo in container.
As shown at
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_07-delta/src/hotspot/share/services/attachListener.cpp.udiff.html
### attachListener.cpp
@@ -252,11 +252,11 @@
static jint heap_inspection(AttachOperation* op, outputStream* out) {
bool live_objects_only = true; // default is true to retain the
behavior before this change is made
outputStream* os = out; // if path not specified or path is NULL,
use out
fileStream* fs = NULL;
const char* arg0 = op->arg(0);
- uint parallel_thread_num = MAX(1, os::processor_count() * 3 / 8); //
default is less than half of processors.
+ uint parallel_thread_num = MAX(1,
os::initial_active_processor_count() * 3 / 8); // default is less than half of
processors.
if (arg0 != NULL && (strlen(arg0) > 0)) {
if (strcmp(arg0, "-all") != 0 && strcmp(arg0, "-live") != 0) {
out->print_cr("Invalid argument to inspectheap operation: %s",
arg0);
return JNI_ERR;
}
###
Thanks.
BRs,
Lin
On 2020/7/9, 3:22 PM, "linzang(臧琳)" wrote:
Hi Paul,
Thanks for reviewing!
>>
>> I'd move all the argument parsing code to JMap.java and
just pass the results to Hotspot. Both histo() in JMap.java and code in
attachListener.* parse the com
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
Hi Paul,
Thanks for your help, that all looks good to me.
Just 2 minor changes:
• delete the final return in ParHeapInspectTask::work, you mentioned it
but seems not include in the webrev :-)
• delete a unnecessary blank line in heapInspect.cpp at merge_entry()
#
--- old/src/hotspot/share/memory/heapInspection.cpp 2020-07-23
11:23:29.281666456 +0800
+++ new/src/hotspot/share/memory/heapInspection.cpp 2020-07-23
11:23:29.017666447 +0800
@@ -251,7 +251,6 @@
_size_of_instances_in_words += cie->words();
return true;
}
-
return false;
}
@@ -568,7 +567,6 @@
Atomic::add(&_missed_count, missed_count);
} else {
Atomic::store(&_success, false);
- return;
}
}
#
Here is the webrev
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_08/
BRs,
Lin
-
From: "Hohensee, Paul"
Date: Thursday, July 23, 2020 at 6:48 AM
To: "linzang(臧琳)" , Stefan Karlsson
, "serguei.spit...@oracle.com"
, David Holmes ,
serviceability-dev ,
"hotspot-gc-...@openjdk.java.net"
Subject: RE: RFR(L): 8215624: add parallel heap inspection support for jmap
histo(G1)(Internet mail)
Just small things.
heapInspection.cpp:
In ParHeapInspectTask::work, remove the final return statement and fix the
following ‘}’ indent. I.e., replace
+ Atomic::store(&_success, false);
+ return;
+ }
with
+ Atomic::store(&_success, false);
+ }
In HeapInspection::heap_inspection, missed_count should be a uint to match
other missed_count declarations, and should be initialized to the result of
populate_table() rather than separately to 0.
attachListener.cpp:
In heap_inspection, initial_processor_count returns an int, so cast its result
to a uint.
Similarly, parse_uintx returns a uintx, so cast its result (num) to uint when
assigning to parallel_thread_num.
BasicJMapTest.java:
I took the liberty of refactoring testHisto*/histoToFile/testDump*/dump to
remove redundant interposition methods and make histoToFile and dump look as
similar as possible.
Webrev with the above changes in
http://cr.openjdk.java.net/~phh/8214535/webrev.01/
Thanks,
Paul
On 7/15/20, 2:13 AM, "linzang(臧琳)" wrote:
Upload a new webrev at
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_07/
It fix a potential issue that unexpected number of threads maybe
calculated for "parallel" option of jmap -histo in container.
As shown at
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_07-delta/src/hotspot/share/services/attachListener.cpp.udiff.html
### attachListener.cpp
@@ -252,11 +252,11 @@
static jint heap_inspection(AttachOperation* op, outputStream* out) {
bool live_objects_only = true; // default is true to retain the
behavior before this change is made
outputStream* os = out; // if path not specified or path is NULL, use
out
fileStream* fs = NULL;
const char* arg0 = op->arg(0);
- uint parallel_thread_num = MAX(1, os::processor_count() * 3 / 8); //
default is less than half of processors.
+ uint parallel_thread_num = MAX(1, os::initial_active_processor_count() *
3 / 8); // default is less than half of processors.
if (arg0 != NULL && (strlen(arg0) > 0)) {
if (strcmp(arg0, "-all") != 0 && strcmp(arg0, "-live") != 0) {
out->print_cr("Invalid argument to inspectheap operation: %s", arg0);
return JNI_ERR;
}
###
Thanks.
BRs,
Lin
On 2020/7/9, 3:22 PM, "linzang(臧琳)" wrote:
Hi Paul,
Thanks for reviewing!
>>
>> I'd move all the argument parsing code to JMap.java and just
pass the results to Hotspot. Both histo() in JMap.java and code in
attachListener.* parse the command line arguments, though the code in histo()
doesn't parse the argument to "parallel". I'd upgrade the code in histo() to do
a complete parse and pass the option values to executeCommandForPid as before:
there would just be more of them now. That would allow you to eliminate all the
parsing code in attachListener.cpp as well as the change to arguments.hpp.
>>
The reason I made the change in Jmap.java that compose all
arguments as 1 string , instead of passing 3 argments, is to avoid the
compatibility issue, as we discussed in
http://mail.openjdk.java.net/pipermail/serviceability-dev/2019-February/thread.html#27240.
The root cause of the compatibility issue is because max argument count in
HotspotVirtualMachineImp
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
Upload a new webrev at
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_07/
It fix a potential issue that unexpected number of threads maybe calculated
for "parallel" option of jmap -histo in container.
As shown at
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_07-delta/src/hotspot/share/services/attachListener.cpp.udiff.html
### attachListener.cpp
@@ -252,11 +252,11 @@
static jint heap_inspection(AttachOperation* op, outputStream* out) {
bool live_objects_only = true; // default is true to retain the behavior
before this change is made
outputStream* os = out; // if path not specified or path is NULL, use out
fileStream* fs = NULL;
const char* arg0 = op->arg(0);
- uint parallel_thread_num = MAX(1, os::processor_count() * 3 / 8); // default
is less than half of processors.
+ uint parallel_thread_num = MAX(1, os::initial_active_processor_count() * 3 /
8); // default is less than half of processors.
if (arg0 != NULL && (strlen(arg0) > 0)) {
if (strcmp(arg0, "-all") != 0 && strcmp(arg0, "-live") != 0) {
out->print_cr("Invalid argument to inspectheap operation: %s", arg0);
return JNI_ERR;
}
###
Thanks.
BRs,
Lin
On 2020/7/9, 3:22 PM, "linzang(臧琳)" wrote:
Hi Paul,
Thanks for reviewing!
>>
>> I'd move all the argument parsing code to JMap.java and just
pass the results to Hotspot. Both histo() in JMap.java and code in
attachListener.* parse the command line arguments, though the code in histo()
doesn't parse the argument to "parallel". I'd upgrade the code in histo() to do
a complete parse and pass the option values to executeCommandForPid as before:
there would just be more of them now. That would allow you to eliminate all the
parsing code in attachListener.cpp as well as the change to arguments.hpp.
>>
The reason I made the change in Jmap.java that compose all arguments as
1 string , instead of passing 3 argments, is to avoid the compatibility issue,
as we discussed in
http://mail.openjdk.java.net/pipermail/serviceability-dev/2019-February/thread.html#27240.
The root cause of the compatibility issue is because max argument count in
HotspotVirtualMachineImpl.java and attachlistener.cpp need to be enlarged
(changes like http://hg.openjdk.java.net/jdk/jdk/rev/e7cf035682e3#l2.1) when
jmap has more than 3 arguments. But if user use an old jcmd/jmap tool, it may
stuck at socket read(), because the "max argument count" don't match.
I re-checked this change, the argument count of jmap histo is equal to
3 (live, file, parallel), so it can work normally even without the change of
passing argument. But I think we have to face the problem if more arguments is
added in jcmd alike tools later, not sure whether it should be sloved (or a
workaround) in this changeset.
And here are the lastest webrev and delta:
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_06/
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_06-delta/
Cheers,
Lin
On 2020/7/7, 5:57 AM, "Hohensee, Paul" wrote:
I'd like to see this feature added. :)
The CSR looks good, as does the basic parallel inspection algorithm.
Stefan's done the GC part, so I'll stick to the non-GC part (fwiw, the GC part
lgtm).
I'd move all the argument parsing code to JMap.java and just pass the
results to Hotspot. Both histo() in JMap.java and code in attachListener.*
parse the command line arguments, though the code in histo() doesn't parse the
argument to "parallel". I'd upgrade the code in histo() to do a complete parse
and pass the option values to executeCommandForPid as before: there would just
be more of them now. That would allow you to eliminate all the parsing code in
attachListener.cpp as well as the change to arguments.hpp.
heapInspection.hpp:
_shared_miss_count (s/b _missed_count, see below) isn't a size, so it
should be a uint instead of a size_t. Same with the new parallel_thread_num
argument to heap_inspection() and populate_table().
Comment copy-edit:
+// Parallel heap inspection task. Parallel inspection can fail due to
+// a native OOM when allocating memory for TL-KlassInfoTable.
+// _success will be set false on an OOM, and serial inspection tried.
_shared_miss_count should be _missed_count to match the missed_count()
getter, or rename missed_count() to be shared_miss_count(). Whichever way you
go, the field type should match the getter result type: uint is reasonable.
heapInspection.cpp:
You might use ResourceMark twice in populate_table, separately for the
parallel attempt and the serial code. If the parallel attempt fails and
available memory is low, it would be good to clean up the memory used by the
parallel attempt before doing
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
Hi Paul,
Thanks for reviewing!
>>
>> I'd move all the argument parsing code to JMap.java and just pass
the results to Hotspot. Both histo() in JMap.java and code in attachListener.*
parse the command line arguments, though the code in histo() doesn't parse the
argument to "parallel". I'd upgrade the code in histo() to do a complete parse
and pass the option values to executeCommandForPid as before: there would just
be more of them now. That would allow you to eliminate all the parsing code in
attachListener.cpp as well as the change to arguments.hpp.
>>
The reason I made the change in Jmap.java that compose all arguments as 1
string , instead of passing 3 argments, is to avoid the compatibility issue, as
we discussed in
http://mail.openjdk.java.net/pipermail/serviceability-dev/2019-February/thread.html#27240.
The root cause of the compatibility issue is because max argument count in
HotspotVirtualMachineImpl.java and attachlistener.cpp need to be enlarged
(changes like http://hg.openjdk.java.net/jdk/jdk/rev/e7cf035682e3#l2.1) when
jmap has more than 3 arguments. But if user use an old jcmd/jmap tool, it may
stuck at socket read(), because the "max argument count" don't match.
I re-checked this change, the argument count of jmap histo is equal to 3
(live, file, parallel), so it can work normally even without the change of
passing argument. But I think we have to face the problem if more arguments is
added in jcmd alike tools later, not sure whether it should be sloved (or a
workaround) in this changeset.
And here are the lastest webrev and delta:
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_06/
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_06-delta/
Cheers,
Lin
On 2020/7/7, 5:57 AM, "Hohensee, Paul" wrote:
I'd like to see this feature added. :)
The CSR looks good, as does the basic parallel inspection algorithm.
Stefan's done the GC part, so I'll stick to the non-GC part (fwiw, the GC part
lgtm).
I'd move all the argument parsing code to JMap.java and just pass the
results to Hotspot. Both histo() in JMap.java and code in attachListener.*
parse the command line arguments, though the code in histo() doesn't parse the
argument to "parallel". I'd upgrade the code in histo() to do a complete parse
and pass the option values to executeCommandForPid as before: there would just
be more of them now. That would allow you to eliminate all the parsing code in
attachListener.cpp as well as the change to arguments.hpp.
heapInspection.hpp:
_shared_miss_count (s/b _missed_count, see below) isn't a size, so it
should be a uint instead of a size_t. Same with the new parallel_thread_num
argument to heap_inspection() and populate_table().
Comment copy-edit:
+// Parallel heap inspection task. Parallel inspection can fail due to
+// a native OOM when allocating memory for TL-KlassInfoTable.
+// _success will be set false on an OOM, and serial inspection tried.
_shared_miss_count should be _missed_count to match the missed_count()
getter, or rename missed_count() to be shared_miss_count(). Whichever way you
go, the field type should match the getter result type: uint is reasonable.
heapInspection.cpp:
You might use ResourceMark twice in populate_table, separately for the
parallel attempt and the serial code. If the parallel attempt fails and
available memory is low, it would be good to clean up the memory used by the
parallel attempt before doing the serial code.
Style nit in KlassInfoTable::merge_entry(). I'd line up the definitions of
k and elt, so "k" is even with "elt". And, because it's two lines shorter, I'd
replace
+ } else {
+return false;
+ }
with
+ return false;
KlassInfoTableMergeClosure.is_success() should be just success() (i.e., no
"is_" prefix) because it's a getter.
I'd reorganize the code in populate_table() to make it more clear, vis (I
changed _shared_missed_count to _missed_count)
+ if (cit.allocation_failed()) {
+// fail to allocate memory, stop parallel mode
+Atomic::store(&_success, false);
+return;
+ }
+ RecordInstanceClosure ric(, _filter);
+ _poi->object_iterate(, worker_id);
+ missed_count = ric.missed_count();
+ {
+MutexLocker x(&_mutex);
+merge_success = _shared_cit->merge();
+ }
+ if (merge_success) {
+Atomic::add(&_missed_count, missed_count);
+ else {
+Atomic::store(&_success, false);
+ }
Thanks,
Paul
On 6/29/20, 7:20 PM, "linzang(臧琳)" wrote:
Dear All,
Sorry to bother again, I just want to make sure that is this
change worth to be continue to work on? If decision is made to not. I think I
can drop this work and stop asking for help reviewing...
Thanks for all your help about reviewing this previously.
BRs,
RE: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
I'd like to see this feature added. :)
The CSR looks good, as does the basic parallel inspection algorithm. Stefan's
done the GC part, so I'll stick to the non-GC part (fwiw, the GC part lgtm).
I'd move all the argument parsing code to JMap.java and just pass the results
to Hotspot. Both histo() in JMap.java and code in attachListener.* parse the
command line arguments, though the code in histo() doesn't parse the argument
to "parallel". I'd upgrade the code in histo() to do a complete parse and pass
the option values to executeCommandForPid as before: there would just be more
of them now. That would allow you to eliminate all the parsing code in
attachListener.cpp as well as the change to arguments.hpp.
heapInspection.hpp:
_shared_miss_count (s/b _missed_count, see below) isn't a size, so it should be
a uint instead of a size_t. Same with the new parallel_thread_num argument to
heap_inspection() and populate_table().
Comment copy-edit:
+// Parallel heap inspection task. Parallel inspection can fail due to
+// a native OOM when allocating memory for TL-KlassInfoTable.
+// _success will be set false on an OOM, and serial inspection tried.
_shared_miss_count should be _missed_count to match the missed_count() getter,
or rename missed_count() to be shared_miss_count(). Whichever way you go, the
field type should match the getter result type: uint is reasonable.
heapInspection.cpp:
You might use ResourceMark twice in populate_table, separately for the parallel
attempt and the serial code. If the parallel attempt fails and available memory
is low, it would be good to clean up the memory used by the parallel attempt
before doing the serial code.
Style nit in KlassInfoTable::merge_entry(). I'd line up the definitions of k
and elt, so "k" is even with "elt". And, because it's two lines shorter, I'd
replace
+ } else {
+return false;
+ }
with
+ return false;
KlassInfoTableMergeClosure.is_success() should be just success() (i.e., no
"is_" prefix) because it's a getter.
I'd reorganize the code in populate_table() to make it more clear, vis (I
changed _shared_missed_count to _missed_count)
+ if (cit.allocation_failed()) {
+// fail to allocate memory, stop parallel mode
+Atomic::store(&_success, false);
+return;
+ }
+ RecordInstanceClosure ric(, _filter);
+ _poi->object_iterate(, worker_id);
+ missed_count = ric.missed_count();
+ {
+MutexLocker x(&_mutex);
+merge_success = _shared_cit->merge();
+ }
+ if (merge_success) {
+Atomic::add(&_missed_count, missed_count);
+ else {
+Atomic::store(&_success, false);
+ }
Thanks,
Paul
On 6/29/20, 7:20 PM, "linzang(臧琳)" wrote:
Dear All,
Sorry to bother again, I just want to make sure that is this change
worth to be continue to work on? If decision is made to not. I think I can drop
this work and stop asking for help reviewing...
Thanks for all your help about reviewing this previously.
BRs,
Lin
On 2020/5/9, 3:47 PM, "linzang(臧琳)" wrote:
Dear All,
May I ask your help again for review the latest change? Thanks!
BRs,
Lin
On 2020/4/28, 1:54 PM, "linzang(臧琳)" wrote:
Hi Stefan,
>> - Adding Atomic::load/store.
>> - Removing the time measurement in the run_task. I renamed
G1's function
>> to run_task_timed. If we need this outside of G1, we can
rethink the API
>> at that point.
>> - ZGC style cleanups
Thanks for revising the patch, they are all good to me, and I
have made a tiny change based on it:
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_04/
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_04-delta/
it reduce the scope of mutex in ParHeapInspectTask, and delete
unnecessary comments.
BRs,
Lin
On 2020/4/27, 4:34 PM, "Stefan Karlsson"
wrote:
Hi Lin,
On 2020-04-26 05:10, linzang(臧琳) wrote:
> Hi Stefan and Paul，
> I have made a new patch based on your comments and
Stefan's Poc code:
> Webrev:
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_03/
> Delta(based on Stefan's change:) :
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_03-delta/webrev_03-delta/
Thanks for providing a delta patch. It makes it much easier to
look at,
and more likely for reviewers to continue reviewing.
I'm going to continue focusing on the GC parts, and leave the
rest to
others to review.
>
> And Here are main changed I made and want to discuss
with you:
> 1. changed"parallelThreadNum=" to "parallel=" for jmap
-histo options.
>
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
Dear All, Sorry to bother again, I just want to make sure that is this change worth to be continue to work on? If decision is made to not. I think I can drop this work and stop asking for help reviewing... Thanks for all your help about reviewing this previously. BRs, Lin On 2020/5/9, 3:47 PM, "linzang(臧琳)" wrote: Dear All, May I ask your help again for review the latest change? Thanks! BRs, Lin On 2020/4/28, 1:54 PM, "linzang(臧琳)" wrote: Hi Stefan, >> - Adding Atomic::load/store. >> - Removing the time measurement in the run_task. I renamed G1's function >> to run_task_timed. If we need this outside of G1, we can rethink the API >> at that point. >> - ZGC style cleanups Thanks for revising the patch, they are all good to me, and I have made a tiny change based on it: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_04/ http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_04-delta/ it reduce the scope of mutex in ParHeapInspectTask, and delete unnecessary comments. BRs, Lin On 2020/4/27, 4:34 PM, "Stefan Karlsson" wrote: Hi Lin, On 2020-04-26 05:10, linzang(臧琳) wrote: > Hi Stefan and Paul， > I have made a new patch based on your comments and Stefan's Poc code: > Webrev: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_03/ > Delta(based on Stefan's change:) : http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_03-delta/webrev_03-delta/ Thanks for providing a delta patch. It makes it much easier to look at, and more likely for reviewers to continue reviewing. I'm going to continue focusing on the GC parts, and leave the rest to others to review. > > And Here are main changed I made and want to discuss with you: > 1. changed"parallelThreadNum=" to "parallel=" for jmap -histo options. > 2. Add logic to test where parallelHeapInspection is fail, in heapInspection.cpp >This is because the parHeapInspectTask create thread local KlassInfoTable in it's work() method, and this may fail because of native OOM, in this case, the parallel should fail and serial heap inspection can be tried. >One more thing I want discuss with you is about the member "_success" of parHeapInspectTask, when native OOM happenes, it is set to false. And since this "set" operation can be conducted in multiple threads, should it be atomic ops? IMO, this is not necessary because "_success" can only be set to false, and there is no way to change it from back to true after the ParHeapInspectTask instance is created, so it is save to be non-atomic, do you agree with that? In these situations you should be using the Atomic::load/store primitives. We're moving toward a later C++ standard were data races are considered undefined behavior. > 3. make CollectedHeap::run_task() be an abstract virtual func, so that every subclass of collectedHeap should support it, so later implementation of new collectedHeap will not miss the "parallel" features. > The problem I want to discuss with you is about epsilonHeap and SerialHeap, as they may not need parallel heap iteration, so I only make task->work(0), in case the run_task() is invoked someway in future. Another way is to left run_task() unimplemented, which one do you think is better? I don't have a strong opinion about this. And also please help take a look at the zHeap, as there is a class zTask that wrap the abstractGangTask, and the collectedHeap::run_task() only accept AbstraceGangTask* as argument, so I made a delegate class to adapt it , please see src/hotspot/share/gc/z/zHeap.cpp. > >There maybe other better ways to sovle the above problems, welcome for any comments, Thanks! I've created a few cleanups and changes on top of your latest patch: https://cr.openjdk.java.net/~stefank/8215624/webrev.02.delta https://cr.openjdk.java.net/~stefank/8215624/webrev.02 - Adding Atomic::load/store. - Removing the time measurement in the run_task. I renamed G1's function to run_task_timed. If we need this outside of G1, we can rethink the API at that point. - ZGC style cleanups Thanks, StefanK > > BRs, > Lin > > On 2020/4/23, 11:08 AM, "linzang(臧琳)" wrote: > > Thanks Paul! I agree with
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
Dear All, May I ask your help again for review the latest change? Thanks! BRs, Lin On 2020/4/28, 1:54 PM, "linzang(臧琳)" wrote: Hi Stefan, >> - Adding Atomic::load/store. >> - Removing the time measurement in the run_task. I renamed G1's function >> to run_task_timed. If we need this outside of G1, we can rethink the API >> at that point. >> - ZGC style cleanups Thanks for revising the patch, they are all good to me, and I have made a tiny change based on it: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_04/ http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_04-delta/ it reduce the scope of mutex in ParHeapInspectTask, and delete unnecessary comments. BRs, Lin On 2020/4/27, 4:34 PM, "Stefan Karlsson" wrote: Hi Lin, On 2020-04-26 05:10, linzang(臧琳) wrote: > Hi Stefan and Paul， > I have made a new patch based on your comments and Stefan's Poc code: > Webrev: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_03/ > Delta(based on Stefan's change:) : http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_03-delta/webrev_03-delta/ Thanks for providing a delta patch. It makes it much easier to look at, and more likely for reviewers to continue reviewing. I'm going to continue focusing on the GC parts, and leave the rest to others to review. > > And Here are main changed I made and want to discuss with you: > 1. changed"parallelThreadNum=" to "parallel=" for jmap -histo options. > 2. Add logic to test where parallelHeapInspection is fail, in heapInspection.cpp >This is because the parHeapInspectTask create thread local KlassInfoTable in it's work() method, and this may fail because of native OOM, in this case, the parallel should fail and serial heap inspection can be tried. >One more thing I want discuss with you is about the member "_success" of parHeapInspectTask, when native OOM happenes, it is set to false. And since this "set" operation can be conducted in multiple threads, should it be atomic ops? IMO, this is not necessary because "_success" can only be set to false, and there is no way to change it from back to true after the ParHeapInspectTask instance is created, so it is save to be non-atomic, do you agree with that? In these situations you should be using the Atomic::load/store primitives. We're moving toward a later C++ standard were data races are considered undefined behavior. > 3. make CollectedHeap::run_task() be an abstract virtual func, so that every subclass of collectedHeap should support it, so later implementation of new collectedHeap will not miss the "parallel" features. > The problem I want to discuss with you is about epsilonHeap and SerialHeap, as they may not need parallel heap iteration, so I only make task->work(0), in case the run_task() is invoked someway in future. Another way is to left run_task() unimplemented, which one do you think is better? I don't have a strong opinion about this. And also please help take a look at the zHeap, as there is a class zTask that wrap the abstractGangTask, and the collectedHeap::run_task() only accept AbstraceGangTask* as argument, so I made a delegate class to adapt it , please see src/hotspot/share/gc/z/zHeap.cpp. > >There maybe other better ways to sovle the above problems, welcome for any comments, Thanks! I've created a few cleanups and changes on top of your latest patch: https://cr.openjdk.java.net/~stefank/8215624/webrev.02.delta https://cr.openjdk.java.net/~stefank/8215624/webrev.02 - Adding Atomic::load/store. - Removing the time measurement in the run_task. I renamed G1's function to run_task_timed. If we need this outside of G1, we can rethink the API at that point. - ZGC style cleanups Thanks, StefanK > > BRs, > Lin > > On 2020/4/23, 11:08 AM, "linzang(臧琳)" wrote: > > Thanks Paul! I agree with using "parallel", will make the update in next patch, Thanks for help update the CSR. > > BRs, > Lin > > On 2020/4/23, 4:42 AM, "Hohensee, Paul" wrote: > > For the interface, I'd use "parallel" instead of "parallelThreadNum". All the other options are lower case, and it's a lot easier to type "parallel". I took the liberty of updating the CSR. If you're ok with it, you might want to change variable names and such, plus of course JMap.usage. > > Thanks, > Paul
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
Hi Stefan, >> - Adding Atomic::load/store. >> - Removing the time measurement in the run_task. I renamed G1's function >> to run_task_timed. If we need this outside of G1, we can rethink the API >> at that point. >> - ZGC style cleanups Thanks for revising the patch, they are all good to me, and I have made a tiny change based on it: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_04/ http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_04-delta/ it reduce the scope of mutex in ParHeapInspectTask, and delete unnecessary comments. BRs, Lin On 2020/4/27, 4:34 PM, "Stefan Karlsson" wrote: Hi Lin, On 2020-04-26 05:10, linzang(臧琳) wrote: > Hi Stefan and Paul， > I have made a new patch based on your comments and Stefan's Poc code: > Webrev: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_03/ > Delta(based on Stefan's change:) : http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_03-delta/webrev_03-delta/ Thanks for providing a delta patch. It makes it much easier to look at, and more likely for reviewers to continue reviewing. I'm going to continue focusing on the GC parts, and leave the rest to others to review. > > And Here are main changed I made and want to discuss with you: > 1. changed"parallelThreadNum=" to "parallel=" for jmap -histo options. > 2. Add logic to test where parallelHeapInspection is fail, in heapInspection.cpp >This is because the parHeapInspectTask create thread local KlassInfoTable in it's work() method, and this may fail because of native OOM, in this case, the parallel should fail and serial heap inspection can be tried. >One more thing I want discuss with you is about the member "_success" of parHeapInspectTask, when native OOM happenes, it is set to false. And since this "set" operation can be conducted in multiple threads, should it be atomic ops? IMO, this is not necessary because "_success" can only be set to false, and there is no way to change it from back to true after the ParHeapInspectTask instance is created, so it is save to be non-atomic, do you agree with that? In these situations you should be using the Atomic::load/store primitives. We're moving toward a later C++ standard were data races are considered undefined behavior. > 3. make CollectedHeap::run_task() be an abstract virtual func, so that every subclass of collectedHeap should support it, so later implementation of new collectedHeap will not miss the "parallel" features. > The problem I want to discuss with you is about epsilonHeap and SerialHeap, as they may not need parallel heap iteration, so I only make task->work(0), in case the run_task() is invoked someway in future. Another way is to left run_task() unimplemented, which one do you think is better? I don't have a strong opinion about this. And also please help take a look at the zHeap, as there is a class zTask that wrap the abstractGangTask, and the collectedHeap::run_task() only accept AbstraceGangTask* as argument, so I made a delegate class to adapt it , please see src/hotspot/share/gc/z/zHeap.cpp. > >There maybe other better ways to sovle the above problems, welcome for any comments, Thanks! I've created a few cleanups and changes on top of your latest patch: https://cr.openjdk.java.net/~stefank/8215624/webrev.02.delta https://cr.openjdk.java.net/~stefank/8215624/webrev.02 - Adding Atomic::load/store. - Removing the time measurement in the run_task. I renamed G1's function to run_task_timed. If we need this outside of G1, we can rethink the API at that point. - ZGC style cleanups Thanks, StefanK > > BRs, > Lin > > On 2020/4/23, 11:08 AM, "linzang(臧琳)" wrote: > > Thanks Paul! I agree with using "parallel", will make the update in next patch, Thanks for help update the CSR. > > BRs, > Lin > > On 2020/4/23, 4:42 AM, "Hohensee, Paul" wrote: > > For the interface, I'd use "parallel" instead of "parallelThreadNum". All the other options are lower case, and it's a lot easier to type "parallel". I took the liberty of updating the CSR. If you're ok with it, you might want to change variable names and such, plus of course JMap.usage. > > Thanks, > Paul > > On 4/22/20, 2:29 AM, "serviceability-dev on behalf of linzang(臧琳)" wrote: > > Dear Stefan, > > Thanks a lot! I agree with you to decouple the heap inspection code with GC's. > I will start from your POC code, may discuss with you later. > > > BRs, > Lin > >
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
Hi Lin, On 2020-04-26 05:10, linzang(臧琳) wrote: Hi Stefan and Paul， I have made a new patch based on your comments and Stefan's Poc code: Webrev: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_03/ Delta(based on Stefan's change:) : http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_03-delta/webrev_03-delta/ Thanks for providing a delta patch. It makes it much easier to look at, and more likely for reviewers to continue reviewing. I'm going to continue focusing on the GC parts, and leave the rest to others to review. And Here are main changed I made and want to discuss with you: 1. changed"parallelThreadNum=" to "parallel=" for jmap -histo options. 2. Add logic to test where parallelHeapInspection is fail, in heapInspection.cpp This is because the parHeapInspectTask create thread local KlassInfoTable in it's work() method, and this may fail because of native OOM, in this case, the parallel should fail and serial heap inspection can be tried. One more thing I want discuss with you is about the member "_success" of parHeapInspectTask, when native OOM happenes, it is set to false. And since this "set" operation can be conducted in multiple threads, should it be atomic ops? IMO, this is not necessary because "_success" can only be set to false, and there is no way to change it from back to true after the ParHeapInspectTask instance is created, so it is save to be non-atomic, do you agree with that? In these situations you should be using the Atomic::load/store primitives. We're moving toward a later C++ standard were data races are considered undefined behavior. 3. make CollectedHeap::run_task() be an abstract virtual func, so that every subclass of collectedHeap should support it, so later implementation of new collectedHeap will not miss the "parallel" features. The problem I want to discuss with you is about epsilonHeap and SerialHeap, as they may not need parallel heap iteration, so I only make task->work(0), in case the run_task() is invoked someway in future. Another way is to left run_task() unimplemented, which one do you think is better? I don't have a strong opinion about this. And also please help take a look at the zHeap, as there is a class zTask that wrap the abstractGangTask, and the collectedHeap::run_task() only accept AbstraceGangTask* as argument, so I made a delegate class to adapt it , please see src/hotspot/share/gc/z/zHeap.cpp. There maybe other better ways to sovle the above problems, welcome for any comments, Thanks! I've created a few cleanups and changes on top of your latest patch: https://cr.openjdk.java.net/~stefank/8215624/webrev.02.delta https://cr.openjdk.java.net/~stefank/8215624/webrev.02 - Adding Atomic::load/store. - Removing the time measurement in the run_task. I renamed G1's function to run_task_timed. If we need this outside of G1, we can rethink the API at that point. - ZGC style cleanups Thanks, StefanK BRs, Lin On 2020/4/23, 11:08 AM, "linzang(臧琳)" wrote: Thanks Paul! I agree with using "parallel", will make the update in next patch, Thanks for help update the CSR. BRs, Lin On 2020/4/23, 4:42 AM, "Hohensee, Paul" wrote: For the interface, I'd use "parallel" instead of "parallelThreadNum". All the other options are lower case, and it's a lot easier to type "parallel". I took the liberty of updating the CSR. If you're ok with it, you might want to change variable names and such, plus of course JMap.usage. Thanks, Paul On 4/22/20, 2:29 AM, "serviceability-dev on behalf of linzang(臧琳)" wrote: Dear Stefan, Thanks a lot! I agree with you to decouple the heap inspection code with GC's. I will start from your POC code, may discuss with you later. BRs, Lin On 2020/4/22, 5:14 PM, "Stefan Karlsson" wrote: Hi Lin, I took a look at this earlier and saw that the heap inspection code is strongly coupled with the CollectedHeap and G1CollectedHeap. I'd prefer if we'd abstract this away, so that the GCs only provide a "parallel object iteration" interface, and the heap inspection code is kept elsewhere. I started experimenting with doing that, but other higher-priority (to me) tasks have had to take precedence. I've uploaded my work-in-progress / proof-of-concept: https://cr.openjdk.java.net/~stefank/8215624/webrev.01.delta/ https://cr.openjdk.java.net/~stefank/8215624/webrev.01/ The current code doesn't handle the lifecycle (deletion) of the ParallelObjectIterators. There's also code left unimplemented in around
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
Hi Stefan and Paul， I have made a new patch based on your comments and Stefan's Poc code: Webrev: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_03/ Delta(based on Stefan's change:) : http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_03-delta/webrev_03-delta/ And Here are main changed I made and want to discuss with you: 1. changed"parallelThreadNum=" to "parallel=" for jmap -histo options. 2. Add logic to test where parallelHeapInspection is fail, in heapInspection.cpp This is because the parHeapInspectTask create thread local KlassInfoTable in it's work() method, and this may fail because of native OOM, in this case, the parallel should fail and serial heap inspection can be tried. One more thing I want discuss with you is about the member "_success" of parHeapInspectTask, when native OOM happenes, it is set to false. And since this "set" operation can be conducted in multiple threads, should it be atomic ops? IMO, this is not necessary because "_success" can only be set to false, and there is no way to change it from back to true after the ParHeapInspectTask instance is created, so it is save to be non-atomic, do you agree with that? 3. make CollectedHeap::run_task() be an abstract virtual func, so that every subclass of collectedHeap should support it, so later implementation of new collectedHeap will not miss the "parallel" features. The problem I want to discuss with you is about epsilonHeap and SerialHeap, as they may not need parallel heap iteration, so I only make task->work(0), in case the run_task() is invoked someway in future. Another way is to left run_task() unimplemented, which one do you think is better? And also please help take a look at the zHeap, as there is a class zTask that wrap the abstractGangTask, and the collectedHeap::run_task() only accept AbstraceGangTask* as argument, so I made a delegate class to adapt it , please see src/hotspot/share/gc/z/zHeap.cpp. There maybe other better ways to sovle the above problems, welcome for any comments, Thanks! BRs, Lin On 2020/4/23, 11:08 AM, "linzang(臧琳)" wrote: Thanks Paul! I agree with using "parallel", will make the update in next patch, Thanks for help update the CSR. BRs, Lin On 2020/4/23, 4:42 AM, "Hohensee, Paul" wrote: For the interface, I'd use "parallel" instead of "parallelThreadNum". All the other options are lower case, and it's a lot easier to type "parallel". I took the liberty of updating the CSR. If you're ok with it, you might want to change variable names and such, plus of course JMap.usage. Thanks, Paul On 4/22/20, 2:29 AM, "serviceability-dev on behalf of linzang(臧琳)" wrote: Dear Stefan, Thanks a lot! I agree with you to decouple the heap inspection code with GC's. I will start from your POC code, may discuss with you later. BRs, Lin On 2020/4/22, 5:14 PM, "Stefan Karlsson" wrote: Hi Lin, I took a look at this earlier and saw that the heap inspection code is strongly coupled with the CollectedHeap and G1CollectedHeap. I'd prefer if we'd abstract this away, so that the GCs only provide a "parallel object iteration" interface, and the heap inspection code is kept elsewhere. I started experimenting with doing that, but other higher-priority (to me) tasks have had to take precedence. I've uploaded my work-in-progress / proof-of-concept: https://cr.openjdk.java.net/~stefank/8215624/webrev.01.delta/ https://cr.openjdk.java.net/~stefank/8215624/webrev.01/ The current code doesn't handle the lifecycle (deletion) of the ParallelObjectIterators. There's also code left unimplemented in around CollectedHeap::run_task. However, I think this could work as a basis to pull out the heap inspection code out of the GCs. Thanks, StefanK On 2020-04-22 02:21, linzang(臧琳) wrote: > Dear all, > May I ask you help to review? This RFR has been there for quite a while. > Thanks! > > BRs, > Lin > > > On 2020/3/16, 5:18 PM, "linzang(臧琳)" wrote:> > >>Just update a new path, my preliminary measure show about 3.5x speedup of jmap histo on a nearly full 4GB G1 heap (8-core platform with parallel thread number set to 4). >> webrev: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_02/ >> bug: https://bugs.openjdk.java.net/browse/JDK-8215624 >>
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
Thanks Paul! I agree with using "parallel", will make the update in next patch, Thanks for help update the CSR. BRs, Lin On 2020/4/23, 4:42 AM, "Hohensee, Paul" wrote: For the interface, I'd use "parallel" instead of "parallelThreadNum". All the other options are lower case, and it's a lot easier to type "parallel". I took the liberty of updating the CSR. If you're ok with it, you might want to change variable names and such, plus of course JMap.usage. Thanks, Paul On 4/22/20, 2:29 AM, "serviceability-dev on behalf of linzang(臧琳)" wrote: Dear Stefan, Thanks a lot! I agree with you to decouple the heap inspection code with GC's. I will start from your POC code, may discuss with you later. BRs, Lin On 2020/4/22, 5:14 PM, "Stefan Karlsson" wrote: Hi Lin, I took a look at this earlier and saw that the heap inspection code is strongly coupled with the CollectedHeap and G1CollectedHeap. I'd prefer if we'd abstract this away, so that the GCs only provide a "parallel object iteration" interface, and the heap inspection code is kept elsewhere. I started experimenting with doing that, but other higher-priority (to me) tasks have had to take precedence. I've uploaded my work-in-progress / proof-of-concept: https://cr.openjdk.java.net/~stefank/8215624/webrev.01.delta/ https://cr.openjdk.java.net/~stefank/8215624/webrev.01/ The current code doesn't handle the lifecycle (deletion) of the ParallelObjectIterators. There's also code left unimplemented in around CollectedHeap::run_task. However, I think this could work as a basis to pull out the heap inspection code out of the GCs. Thanks, StefanK On 2020-04-22 02:21, linzang(臧琳) wrote: > Dear all, > May I ask you help to review? This RFR has been there for quite a while. > Thanks! > > BRs, > Lin > > > On 2020/3/16, 5:18 PM, "linzang(臧琳)" wrote:> > >>Just update a new path, my preliminary measure show about 3.5x speedup of jmap histo on a nearly full 4GB G1 heap (8-core platform with parallel thread number set to 4). >> webrev: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_02/ >> bug: https://bugs.openjdk.java.net/browse/JDK-8215624 >> CSR: https://bugs.openjdk.java.net/browse/JDK-8239290 >> BRs, >> Lin >> > On 2020/3/2, 9:56 PM, "linzang(臧琳)" wrote: >> > >> >Dear all, >> > Let me try to ease the reviewing work by some explanation :P >> > The patch's target is to speed up jmap -histo for heap iteration, from my experience it is necessary for large heap investigation. E.g in bigData scenario I have tried to conduct jmap -histo against 180GB heap, it does take quite a while. >> > And if my understanding is corrent, even the jmap -histo without "live" option does heap inspection with heap lock acquired. so it is very likely to block mutator thread in allocation-sensitive scenario. I would say the faster the heap inspection does, the shorter the mutator be blocked. This is parallel iteration for jmap is necessary. >> > I think the parallel heap inspection should be applied to all kind of heap. However, consider the heap layout are different for GCs, much time is required to understand all kinds of the heap layout to make the whole change. IMO, It is not wise to have a huge patch for the whole solution at once, and it is even harder to review it. So I plan to implement it incrementally, the first patch (this one) is going to confirm the implemention detail of how jmap accept the new option, passes it to attachListener of the jvm process and then how to make the parallel inspection closure be generic enough to make it easy to extend to different heap layout. And also how to implement the heap inspection in specific gc's heap. This patch use G1's heap as the begining. >> > This patch actually do several things: >> > 1. Add an option "parallelThreadNum=" to jmap -histo, the default behavior is to set N to 0, means let's JVM decide how many threads to use for heap inspection. Set this option to 1 will disable parallel heap inspection. (more details in CSR: https://bugs.openjdk.java.net/browse/JDK-8239290) >> > 2. Make a change in how Jmap passing arguments, changes in
RE: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
For the interface, I'd use "parallel" instead of "parallelThreadNum". All the other options are lower case, and it's a lot easier to type "parallel". I took the liberty of updating the CSR. If you're ok with it, you might want to change variable names and such, plus of course JMap.usage. Thanks, Paul On 4/22/20, 2:29 AM, "serviceability-dev on behalf of linzang(臧琳)" wrote: Dear Stefan, Thanks a lot! I agree with you to decouple the heap inspection code with GC's. I will start from your POC code, may discuss with you later. BRs, Lin On 2020/4/22, 5:14 PM, "Stefan Karlsson" wrote: Hi Lin, I took a look at this earlier and saw that the heap inspection code is strongly coupled with the CollectedHeap and G1CollectedHeap. I'd prefer if we'd abstract this away, so that the GCs only provide a "parallel object iteration" interface, and the heap inspection code is kept elsewhere. I started experimenting with doing that, but other higher-priority (to me) tasks have had to take precedence. I've uploaded my work-in-progress / proof-of-concept: https://cr.openjdk.java.net/~stefank/8215624/webrev.01.delta/ https://cr.openjdk.java.net/~stefank/8215624/webrev.01/ The current code doesn't handle the lifecycle (deletion) of the ParallelObjectIterators. There's also code left unimplemented in around CollectedHeap::run_task. However, I think this could work as a basis to pull out the heap inspection code out of the GCs. Thanks, StefanK On 2020-04-22 02:21, linzang(臧琳) wrote: > Dear all, > May I ask you help to review? This RFR has been there for quite a while. > Thanks! > > BRs, > Lin > > > On 2020/3/16, 5:18 PM, "linzang(臧琳)" wrote:> > >>Just update a new path, my preliminary measure show about 3.5x speedup of jmap histo on a nearly full 4GB G1 heap (8-core platform with parallel thread number set to 4). >> webrev: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_02/ >> bug: https://bugs.openjdk.java.net/browse/JDK-8215624 >> CSR: https://bugs.openjdk.java.net/browse/JDK-8239290 >> BRs, >> Lin >> > On 2020/3/2, 9:56 PM, "linzang(臧琳)" wrote: >> > >> >Dear all, >> > Let me try to ease the reviewing work by some explanation :P >> > The patch's target is to speed up jmap -histo for heap iteration, from my experience it is necessary for large heap investigation. E.g in bigData scenario I have tried to conduct jmap -histo against 180GB heap, it does take quite a while. >> > And if my understanding is corrent, even the jmap -histo without "live" option does heap inspection with heap lock acquired. so it is very likely to block mutator thread in allocation-sensitive scenario. I would say the faster the heap inspection does, the shorter the mutator be blocked. This is parallel iteration for jmap is necessary. >> > I think the parallel heap inspection should be applied to all kind of heap. However, consider the heap layout are different for GCs, much time is required to understand all kinds of the heap layout to make the whole change. IMO, It is not wise to have a huge patch for the whole solution at once, and it is even harder to review it. So I plan to implement it incrementally, the first patch (this one) is going to confirm the implemention detail of how jmap accept the new option, passes it to attachListener of the jvm process and then how to make the parallel inspection closure be generic enough to make it easy to extend to different heap layout. And also how to implement the heap inspection in specific gc's heap. This patch use G1's heap as the begining. >> > This patch actually do several things: >> > 1. Add an option "parallelThreadNum=" to jmap -histo, the default behavior is to set N to 0, means let's JVM decide how many threads to use for heap inspection. Set this option to 1 will disable parallel heap inspection. (more details in CSR: https://bugs.openjdk.java.net/browse/JDK-8239290) >> > 2. Make a change in how Jmap passing arguments, changes in http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_01/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java.udiff.html, originally it pass options as separate arguments to attachListener, this patch change to that all options be compose to a single string. So the arg_count_max in attachListener.hpp do not need to be changed, and hence avoid the compatibility issue, as disscussed at https://mail.openjdk.java.net/pipermail/serviceability-dev/2019-March/027334.html
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
Dear Stefan, Thanks a lot! I agree with you to decouple the heap inspection code with GC's. I will start from your POC code, may discuss with you later. BRs, Lin On 2020/4/22, 5:14 PM, "Stefan Karlsson" wrote: Hi Lin, I took a look at this earlier and saw that the heap inspection code is strongly coupled with the CollectedHeap and G1CollectedHeap. I'd prefer if we'd abstract this away, so that the GCs only provide a "parallel object iteration" interface, and the heap inspection code is kept elsewhere. I started experimenting with doing that, but other higher-priority (to me) tasks have had to take precedence. I've uploaded my work-in-progress / proof-of-concept: https://cr.openjdk.java.net/~stefank/8215624/webrev.01.delta/ https://cr.openjdk.java.net/~stefank/8215624/webrev.01/ The current code doesn't handle the lifecycle (deletion) of the ParallelObjectIterators. There's also code left unimplemented in around CollectedHeap::run_task. However, I think this could work as a basis to pull out the heap inspection code out of the GCs. Thanks, StefanK On 2020-04-22 02:21, linzang(臧琳) wrote: > Dear all, > May I ask you help to review? This RFR has been there for quite a while. > Thanks! > > BRs, > Lin > > > On 2020/3/16, 5:18 PM, "linzang(臧琳)" wrote:> > >>Just update a new path, my preliminary measure show about 3.5x speedup of jmap histo on a nearly full 4GB G1 heap (8-core platform with parallel thread number set to 4). >> webrev: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_02/ >> bug: https://bugs.openjdk.java.net/browse/JDK-8215624 >> CSR: https://bugs.openjdk.java.net/browse/JDK-8239290 >> BRs, >> Lin >> > On 2020/3/2, 9:56 PM, "linzang(臧琳)" wrote: >> > >> >Dear all, >> > Let me try to ease the reviewing work by some explanation :P >> > The patch's target is to speed up jmap -histo for heap iteration, from my experience it is necessary for large heap investigation. E.g in bigData scenario I have tried to conduct jmap -histo against 180GB heap, it does take quite a while. >> > And if my understanding is corrent, even the jmap -histo without "live" option does heap inspection with heap lock acquired. so it is very likely to block mutator thread in allocation-sensitive scenario. I would say the faster the heap inspection does, the shorter the mutator be blocked. This is parallel iteration for jmap is necessary. >> > I think the parallel heap inspection should be applied to all kind of heap. However, consider the heap layout are different for GCs, much time is required to understand all kinds of the heap layout to make the whole change. IMO, It is not wise to have a huge patch for the whole solution at once, and it is even harder to review it. So I plan to implement it incrementally, the first patch (this one) is going to confirm the implemention detail of how jmap accept the new option, passes it to attachListener of the jvm process and then how to make the parallel inspection closure be generic enough to make it easy to extend to different heap layout. And also how to implement the heap inspection in specific gc's heap. This patch use G1's heap as the begining. >> > This patch actually do several things: >> > 1. Add an option "parallelThreadNum=" to jmap -histo, the default behavior is to set N to 0, means let's JVM decide how many threads to use for heap inspection. Set this option to 1 will disable parallel heap inspection. (more details in CSR: https://bugs.openjdk.java.net/browse/JDK-8239290) >> > 2. Make a change in how Jmap passing arguments, changes in http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_01/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java.udiff.html, originally it pass options as separate arguments to attachListener, this patch change to that all options be compose to a single string. So the arg_count_max in attachListener.hpp do not need to be changed, and hence avoid the compatibility issue, as disscussed at https://mail.openjdk.java.net/pipermail/serviceability-dev/2019-March/027334.html >> > 3. Add an abstract class ParHeapInspectTask in heapInspection.hpp / heapInspection.cpp, It's work(uint worker_id) method prepares the data structure (KlassInfoTable) need for every parallel worker thread, and then call do_object_iterate_parallel() which is heap specific implementation. I also added some machenism in KlassInfoTable to support parallel iteration, such as merge(). >> >4. In specific heap (G1 in this patch), create a subclass of ParHeapInspectTask, implement the do_object_iterate_parallel()
