[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    RE: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
From:       "Hohensee, Paul" <hohensee () amazon ! com>
Date:       2020-07-29 21:21:19
Message-ID: 01181077-0DD7-4AEE-945B-5D6E21224A31 () amazon ! com
[Download RAW message or body]

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(臧琳)" <linzang@tencent.com> 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(臧琳)" <linzang@tencent.com> 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<uint>(1, \
(uint)os::initial_active_processor_count() * 3 / 8);  \
####################################

        BRs,
        Lin

        On 2020/7/23, 11:56 AM, "linzang(臧琳)" <linzang@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 webrev  \
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_08/

            BRs,
            Lin
            ---------------------------------------------
            From: "Hohensee, Paul" <hohensee@amazon.com>
            Date: Thursday, July 23, 2020 at 6:48 AM
            To: "linzang(臧琳)" <linzang@tencent.com>, Stefan Karlsson \
<stefan.karlsson@oracle.com>, "serguei.spitsyn@oracle.com" \
<serguei.spitsyn@oracle.com>, David Holmes <david.holmes@oracle.com>, \
serviceability-dev <serviceability-dev@openjdk.java.net>, \
                "hotspot-gc-dev@openjdk.java.net" <hotspot-gc-dev@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(臧琳)" <linzang@tencent.com> 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(臧琳)" <linzang@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 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" <hohensee@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(臧琳)" <linzang@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(臧琳)" \
<linzang@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(臧琳)" \
<linzang@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.karlsson@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(臧琳)" \
<linzang@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" <hohensee@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-bounces@openjdk.java.net on behalf of linzang@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.karlsson@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(臧琳)" <linzang@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(臧琳)" <linzang@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(臧琳)" <linzang@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
                                        >                  >>       >    >    >> >
                                        >                  >>       >    >    >
                                        >                  >
                                        >                  >
                                        >                  >
                                        >
                                        >
                                        >
                                        >
                                        >
                                        >


[prev in list] [next in list] [prev in thread] [next in thread] 

Configure | About | News | Add a list | Sponsored by KoreLogic