RFR [S][5/7]: 8197573: Remove concurrent cleanup and secondary free list handling

[sangheon.kim]

Hi Thomas,

On 03/08/2018 05:31 AM, Thomas Schatzl wrote:
> Hi,
>
> On Thu, 2018-03-08 at 13:28 +0100, Stefan Johansson wrote:
>> On 2018-03-08 12:59, Thomas Schatzl wrote:
>>> Hi,
>>>
>>> On Mon, 2018-03-05 at 16:16 +0100, Thomas Schatzl wrote:
>>>> Hi all,
>>>>
>>>>     can I have reviews for this change that removes the concurrent
>>>> cleanup phase and the secondary free list handling entirely. It
>>>> moves leftover work into the Cleanup pause.
>>>>
>>>> In the previous change the remembered set scrubbing, which took
>>>> in cases where it mattered like 95%+ of time, has been removed
>>>> because it is not required any more.
>>>>
>>>> The reasons for removal are:
>>>> - due to JDK-8180415 *freeing* remembered sets (as opposed to
>>>> calculating occupancy) is really fast, so this phase has never
>>>> been very long for a long time.
>>>> - it adds a lock in the LAB allocation path
>>>>
>>>> It also allows us to remove the "mixed_gc_pending" flag in the
>>>> CollectorState, as there is no further time to wait until G1 can
>>>> schedule the "last young gc".
>>>>
>>>> CR:
>>>> https://bugs.openjdk.java.net/browse/JDK-8197573
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~tschatzl/8197573/webrev/index.html
>>>> Testing:
>>>> hs-tier 1-3, ....
>>>>
>>>> (I consider this an "S" changeset because it is mostly removal of
>>>> code).
>>>>
>>> Some ripple-through after latest changes to JDK-8180415 requires an
>>> update of the webrevs:
>>> http://cr.openjdk.java.net/~tschatzl/8197573/webrev.0_to_1 (diff)
>>> http://cr.openjdk.java.net/~tschatzl/8197573/webrev.1 (full)
>> Looks lovely, just some minor things:
>> src/hotspot/share/gc/g1/concurrentMarkThread.cpp
>> 436           log_info(gc, marking)("Concurrent Mark Abort");
>>
>> This line was removed, and it looks like will never log a concurrent
>> mark abort now. Looks like this could be added to
>> G1ConcurrentMark::concurrent_cycle_end() where we trace a potential
>> cm  failure.
> Nice catch!
>
>> ---
>> src/hotspot/share/gc/g1/g1FullGCPrepareTask.cpp
>> 106   _g1h->free_humongous_region(hr, &dummy_free_list);
>>
>> Should be safe to call free_humongous_region with locked = true
>> here, since this will be the only thread working on that region.
> Fixed.
>
>> 115   _g1h->card_table()->clear(MemRegion(hr->bottom(), hr->end()));
>>
>> Pre-existing, but since you have added the clear_cardtable() function
>> we could use it here as well, seems to be the only place where the
>> old "scheme" is used.
> Done.
>
>> ---
>>
>>> I also found some more opportunity to do minor cleanup after
>>> Stefan's
>>> last remark about log messages...
>> Nice, and you also restructured the code exactly like I planned to
>> comment :)
> :)
>
> New webrevs:
> http://cr.openjdk.java.net/~tschatzl/8197573/webrev.1_to_2/ (diff)
> http://cr.openjdk.java.net/~tschatzl/8197573/webrev.2/ (full)
webrev.2 looks good. I only have minor nits.

===============================
src/hotspot/share/gc/g1/concurrentMarkThread.cpp
  362       if (!cm()->has_aborted()) {
...
  364       }
  365
  366       if (!cm()->has_aborted()) {
- Any reason for having 2 if clause at line:362 and 366?

===============================
src/hotspot/share/gc/g1/g1CollectedHeap.cpp (from original file)
1030   // following two methods.
- Do you know which method is referring originally? I guess 
wait_while_free_regions_coming() and _cm->root_regions()->abort()? And 
this is why you didn't remove the comment above 
wait_while_free_regions_coming(), right?

===============================
src/hotspot/share/gc/g1/heapRegionSet.cpp
- Copyright year update

Thanks,
Sangheon


>
> Thanks,
>    Thomas
>