RFR (XL): 8218668: Clean up evacuation of optional collection set

[sangheon.kim at oracle.com]

Hi Thomas,

On 4/5/19 2:41 AM, Thomas Schatzl wrote:
> Hi Sangheon,
>
>    thanks for your review.
>
> On Thu, 2019-04-04 at 14:41 -0700, sangheon.kim at oracle.com wrote:
>> Hi Thomas,
>>
>> On 4/4/19 4:18 AM, Thomas Schatzl wrote:
>>> Hi Kim,
>>>
>>> On Tue, 2019-04-02 at 21:02 -0400, Kim Barrett wrote:
>>>>> On Mar 13, 2019, at 9:12 AM, Thomas Schatzl <
>>>>> thomas.schatzl at oracle.com> wrote:
>>>>>
> [...]
>>> New webrevs:
>>> http://cr.openjdk.java.net/~tschatzl/8218668/webrev.1_to_2/
>>> http://cr.openjdk.java.net/~tschatzl/8218668/webrev.2/
>> Nice clean up!
>>
>> The updated comment at G1CollectionSet is much better to me.
>>
>> I have minor nits:
>> --------------------------------------------------
>> share/gc/g1/g1CollectedHeap.cpp
>>    157 Tickspan G1CollectedHeap::run_task(AbstractGangTask* task,
>> uint num_workers) {
>> - Couldn't always use workers()->run_task(task) instead of
>> num_workers treatment? The difference between
>> WorkGang::run_task(AbstractGangTask* task) and
>> G1ColectedHeap::run_task() looks like WorkGang doesn't
>> allow active workers to be 0. I don't think we have such case.
> I removed the optional parameter.
>
>> - Just an idea and not expect to fix here. It would nice if this api
>> exists on WorkGang something like run_task() and run_task_ms().
> Yeah, that might be interesting to consider as well as other automatic
> time-taking improvements for WorkGang.
>
>> --------------------------------------------------
>> share/gc/g1/g1CollectionSet.hpp
>>     52 // This set is built incrementally at mutator time as regions
>> are retired, and,
>> - two commas?
>>
>>    174   size_t _inc_part_start;
>> - Not initialized at ctor.
>>
>> --------------------------------------------------
>> share/gc/g1/g1RemSet.cpp
>>    342 void
>> G1ScanRSForRegionClosure::scan_opt_rem_set_roots(HeapRegion* r){
>> - Space between '){'
>>
>> --------------------------------------------------
>> share/gc/g1/g1OopStarChunkedList.cpp
>> - copyright year update
>>
>> --------------------------------------------------
>> share/gc/g1/g1OopStarChunkedList.hpp
>> - copyright year update
> All fixed. Thanks,
>
> New webrevs:
> http://cr.openjdk.java.net/~tschatzl/8218668/webrev.2_to_3/
> http://cr.openjdk.java.net/~tschatzl/8218668/webrev.3/
Looks good.

Thanks,
Sangheon


>
>
> Thanks,
>    Thomas
>
>
>