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> 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> 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 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() 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> 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 to
http://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
> > >> >
> > >
