uintx is fine with me.
Thanks,
Paul
On 8/5/20, 1:14 AM, "linzang(臧琳)" <linz...@tencent.com> 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" <stefan.karls...@oracle.com> 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" <serguei.spit...@oracle.com>
> *Date: *Wednesday, August 5, 2020 at 1:02 PM
> *To: *"linzang(臧琳)" <linz...@tencent.com>, "Hohensee, Paul"
> <hohen...@amazon.com>, Stefan Karlsson <stefan.karls...@oracle.com>,
> David Holmes <david.hol...@oracle.com>, serviceability-dev
> <serviceability-dev@openjdk.java.net>,
"hotspot-gc-...@openjdk.java.net"
> <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"<hohen...@amazon.com>
<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(臧琳)"<linz...@tencent.com>
<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(臧琳)"<linz...@tencent.com> <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_processor_count() * 3 / 8);
>
> + uint parallel_thread_num = MAX2<uint>(1,
(uint)os::initial_active_processor_count() * 3 / 8);
>
> ####################################
>
> BRs,
>
> Lin
>
> On 2020/7/23, 11:56 AM,
"linzang(臧琳)"<linz...@tencent.com> <mailto:linz...@tencent.com> 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
webrevhttp://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_08/
>
> BRs,
>
> Lin
>
> ---------------------------------------------
>
> From: "Hohensee, Paul"<hohen...@amazon.com>
<mailto:hohen...@amazon.com>
>
> Date: Thursday, July 23, 2020 at 6:48 AM
>
> To: "linzang(臧琳)"<linz...@tencent.com>
<mailto:linz...@tencent.com>, Stefan Karlsson<stefan.karls...@oracle.com>
<mailto:stefan.karls...@oracle.com>,"serguei.spit...@oracle.com"
<mailto:serguei.spit...@oracle.com> <serguei.spit...@oracle.com>
<mailto:serguei.spit...@oracle.com>, David Holmes<david.hol...@oracle.com>
<mailto:david.hol...@oracle.com>,
serviceability-dev<serviceability-dev@openjdk.java.net>
<mailto:serviceability-dev@openjdk.java.net>,"hotspot-gc-...@openjdk.java.net"
<mailto:hotspot-gc-...@openjdk.java.net> <hotspot-gc-...@openjdk.java.net>
<mailto: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(臧琳)"<linz...@tencent.com> <mailto:linz...@tencent.com> wrote:
>
> Upload a new webrev
athttp://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
athttp://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(臧琳)"<linz...@tencent.com> <mailto:linz...@tencent.com> 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
inhttp://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 likehttp://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"<hohen...@amazon.com> <mailto:hohen...@amazon.com> 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(&cit, _filter);
>
> + _poi->object_iterate(&ric,
worker_id);
>
> + missed_count =
ric.missed_count();
>
> + {
>
> + MutexLocker x(&_mutex);
>
> + merge_success =
_shared_cit->merge(&cit);
>
> + }
>
> + if (merge_success) {
>
> + Atomic::add(&_missed_count,
missed_count);
>
> + else {
>
> + Atomic::store(&_success,
false);
>
> + }
>
> Thanks,
>
> Paul
>
> On 6/29/20, 7:20 PM,
"linzang(臧琳)"<linz...@tencent.com> <mailto:linz...@tencent.com> 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(臧琳)"<linz...@tencent.com> <mailto:linz...@tencent.com> 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(臧琳)"<linz...@tencent.com> <mailto:linz...@tencent.com> 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"<stefan.karls...@oracle.com>
<mailto:stefan.karls...@oracle.com> 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(臧琳)"<linz...@tencent.com> <mailto:linz...@tencent.com>
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"<hohen...@amazon.com>
<mailto:hohen...@amazon.com> 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(臧琳)"<serviceability-dev-boun...@openjdk.java.net on behalf of
> linz...@tencent.com>
<mailto:serviceability-dev-boun...@openjdk.java.netonbehalfoflinz...@tencent.com>
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"<stefan.karls...@oracle.com>
<mailto:stefan.karls...@oracle.com> 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(臧琳)"<linz...@tencent.com>
<mailto:linz...@tencent.com> 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(臧琳)"<linz...@tencent.com>
<mailto:linz...@tencent.com> 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=<N>" 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
inhttp://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
athttps://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() for parallel
heap inspection. For G1, it simply invoke g1CollectedHeap's
object_iterate_parallel().
>
> >
>> > 5. Add related test.
>
> >
>> > 6. it may be easy to extend this patch for other kinds of
heap by creating subclass of ParHeapInspectTask and implement the
do_object_iterate_parallel().
>
> >
>> >
>
> >
>> > Hope these info could help on code review and initate the
discussion :-)
>
> >
>> > Thanks!
>
> >
>> >
>
> >
>> > BRs,
>
> >
>> > Lin
>
> >
>> > >On 2020/2/19, 9:40 AM, "linzang(臧琳)"<linz...@tencent.com>
<mailto:linz...@tencent.com> wrote:.
>
> >
>> > >
>
> >
>> > > Re-post this RFR with correct enhancement number to make it
trackable.
>
> >
>> > > please ignore the previous wrong post. sorry for troubles.
>
> >
>> > >
>
> >
>> > >
webrev:http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_01/
>
> >
>> > > Hi bug:https://bugs.openjdk.java.net/browse/JDK-8215624
>
> >
>> > > CSR:https://bugs.openjdk.java.net/browse/JDK-8239290
>
> >
>> > > --------------
>
> >
>> > > Lin
>
> >
>> > > >Hi Lin,
>
> >
> > > > >
>
> >
>> > > >Could you, please, re-post your RFR with the right
enhancement number in
>
> >
>> > > >the message subject?
>
> >
>> > > >It will be more trackable this way.
>
> >
>> > > >
>
> >
>> > > >Thanks,
>
> >
>> > > >Serguei
>
> >
>> > > >
>
> >
>> > > >
>
> >
>> > > >On 2/17/20 10:29 PM, linzang(臧琳) wrote:
>
> >
>> > > >> Dear David,
>
> >
>> > > >> Thanks a lot!
>
> >
>> > > >> I have updated the refined code
tohttp://cr.openjdk.java.net/~lzang/jmap-8214535/8215264/webrev_01/.
>
> >
>> > > >> IMHO the parallel heap inspection can be
extended to all kinds of heap as long as the heap layout can support parallel
iteration.
>
> >
>> > > >> Maybe we can firstly use this webrev to discuss
how to implement it, because I am not sure my current implementation is an
appropriate way to communicate with collectedHeap, then we can extend the
solution to other kinds of heap.
>
> >
>> > > >>
>
> >
>> > > >> Thanks,
>
> >
>> > > >> --------------
>
> >
>> > > >> Lin
>
> >
>> > > >>> Hi Lin,
>
> >
>> > > >>>
>
> >
>> > > >>> Adding in hotspot-gc-dev as they need to see how this
interacts with GC
>
> >
>> > > >>> worker threads, and whether it needs to be extended
beyond G1.
>
> >
>> > > >>>
>
> >
>> > > >>> I happened to spot one nit when browsing:
>
> >
>> > > >>>
>
> >
>> > > >>> src/hotspot/share/gc/shared/collectedHeap.hpp
>
> >
>> > > >>>
>
> >
>> > > >>> + virtual bool
run_par_heap_inspect_task(KlassInfoTable* cit,
>
> >
>> > > >>> +
BoolObjectClosure* filter,
>
> >
>> > > >>> + size_t*
missed_count,
>
> >
>> > > >>> + size_t
thread_num) {
>
> >
>> > > >>> + return NULL;
>
> >
>> > > >>>
>
> >
>> > > >>> s/NULL/false/
>
> >
>> > > >>>
>
> >
>> > > >>> Cheers,
>
> >
>> > > >>> David
>
> >
> > > > >>>
>
> >
>> > > >>> On 18/02/2020 2:15 pm, linzang(臧琳) wrote:
>
> >
>> > > >>>> Dear All,
>
> >
>> > > >>>> May I ask your help to review the follow
changes:
>
> >
>> > > >>>> webrev:
>
> >
>> > >
>>>>http://cr.openjdk.java.net/~lzang/jmap-8214535/8215264/webrev_00/
>
> >
>> > > >>>>
bug:https://bugs.openjdk.java.net/browse/JDK-8215624
>
> >
>> > > >>>> related
CSR:https://bugs.openjdk.java.net/browse/JDK-8239290
>
> >
>> > > >>>> This patch enable parallel heap inspection of
G1 for jmap histo.
>
> >
>> > > >>>> my simple test shown it can speed up 2x of
jmap -histo with
>
> >
>> > > >>>> parallelThreadNum set to 2 for heap at ~500M on
4-core platform.
>
> >
>> > > >>>>
>
> >
>> > > >>>>
------------------------------------------------------------------------
>
> >
>> > > >>>> BRs,
>
> >
>> > > >>>> Lin
>
> >
>> > > >> >
>
> >
>> > > >
>
> >
>
>
> >
>
>
> >
>
>
> >
>
> >
>
> >
>
> >
>
> >
>
> >
>
>
>
