On Fri, 9 Oct 2020 14:16:51 GMT, Lin Zang <lz...@openjdk.org> wrote:
>> Hi Lin,
>>
>> Sorry for not getting to this sooner. One of the reasons is that I haven't
>> had time to explore a better solution, but
>> the above notification triggered me to at least share my thoughts.
>> I would like us to avoid having the logic that check how many workers are
>> doing the work and instead have some
>> mechanism that claim different chunks of work. You can compare it a bit to
>> G1 where we have the `HeapRegionClaimer`
>> that make sure only one thread handles a given region. This claimer needs to
>> be a bit different and allow claiming of
>> eden, to-space, from-space and then multiple chunks of old. But I believe
>> such solution would both be more efficient
>> (since all threads can help out on old in the end) and easier to follow (no
>> special cases). So basically the
>> `PSScavengeParallelObjectIterator` need to set up this claimer and pass it
>> down to the workers and all workers than try
>> to do all work, but only the one getting the claim will do the actual work.
>> What do you think about this approach? Do
>> you understand what I'm after?
>
> Hi @kstefanj, Thanks a lot for your comments.
> I think there can be a reclaimer that treat eden space, from space, to space
> and "blocks" in old space as parallel
> iteration candidates, then every workers try to claim their ownership of the
> candidate that it is going to scan. So
> there is no need to check worker numbers and Id for processing different
> spaces. what do you think?
> BTW, Paul (hohen...@amazon.com) also helped review this patch and send the
> comment in mail list, I will paste his
> comments here and then try to polish a patch with all of your comments
> considered.
Here is the review comments from Paul, copied from hotspot-gc-dev digest:
> Message: 3
> Date: Mon, 28 Sep 2020 22:39:38 +0000
> From: "Hohensee, Paul" <hohen...@amazon.com>
> To: Lin Zang <lz...@openjdk.java.net>,
> "hotspot-gc-...@openjdk.java.net"
> <hotspot-gc-...@openjdk.java.net>,
> "serviceability-dev@openjdk.java.net"
> <serviceability-dev@openjdk.java.net>
> Subject: RE: RFR: 8252103: Parallel heap inspection for
> ParallelScavengeHeap
> Message-ID: <25a4dc80-74a8-4c99-aef4-7e989317b...@amazon.com>
> Content-Type: text/plain; charset="utf-8"
>
> I'm not a GC specialist, but your approach looks reasonable to me.
>
> parallelScavengeHeap.cpp:
>
> "less that 1 workers" -> "less that 1 worker"
>
> psOldGen.cpp:
>
> ">=2" -> ">= 2"
>
> "thread_num-2 worker" -> "(thread_num -2) workers"
>
> "cross blocks" -> "crosses blocks"
>
> "that the object start address locates in the related block" -> "that owns
> the block within which the object starts"
>
> blocks_iterate_parallel() relies on BLOCK_SIZE being an integral multiple of
> a start_array() block (i.e.,
> ObjectStartArray::block_size). I'd add an assert to that effect.
> The comment on the definition of BLOCK_SIZE is "64 KB", but you're doing
> pointer arithmetic in terms of HeapWords,
> which > means the actual byte value is either 256KB or 512KB for 32- and
> 64-bit respectively. You could use
> const int BLOCK_SIZE = (64 * 1024 / HeapWordSize);
>
> "// There is objects" -> "There are objects"
>
> "reture" -> "return"
>
> ", here only focus objects" -> ". Here only focus on objects"
>
> It's a matter of taste, but imo the loop in blocks_iterate_parallel() would
> be more clear as
>
> for (HeapWord* p = start; p < end; p += oop(p)->size()) {
> cl->do_object(oop(p));
> }
>
> psOldGen.hpp:
>
> blocks_iterate_parallel() is pure implementation and I don't see a reference
> outside psOldGen.cpp. So, it should be
> private, > not public.
>
> psYoungGen.cpp:
>
> "<=1" -> "<= 1"
>
> Thanks,
> Paul
Thanks for Paul's help and effort for reviewing, I will make a refined patch
based on your comments.
-------------
PR: https://git.openjdk.java.net/jdk/pull/25
