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
