RFR: 8252103: Parallel heap inspection for ParallelScavengeHeap

[Lin Zang]

On Fri, 9 Oct 2020 14:16:51 GMT, Lin Zang <lzang at 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 (hohensee at 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" <hohensee at amazon.com>
> To: Lin Zang <lzang at openjdk.java.net>,
>                "hotspot-gc-dev at openjdk.java.net" <hotspot-gc-dev at openjdk.java.net>,
>                "serviceability-dev at openjdk.java.net"
>                 <serviceability-dev at openjdk.java.net>
> Subject: RE: RFR: 8252103: Parallel heap inspection for
>                 ParallelScavengeHeap
> Message-ID: <25A4DC80-74A8-4C99-AEF4-7E989317B18A at 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