RFR: 8252103: Parallel heap inspection for ParallelScavengeHeap

[Lin Zang]

On Fri, 9 Oct 2020 14:23:31 GMT, Lin Zang <lzang at openjdk.org> wrote:

>> 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.

Dear @stefank,
I have update this PR that use a claimer to help worker thread do parallel iteration. would you like to help review
again? Thanks,
Lin

-------------

PR: https://git.openjdk.java.net/jdk/pull/25