On Fri, 9 Oct 2020 14:23:31 GMT, Lin Zang <lz...@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 (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.
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
