RFR (7xS): 8175554: Improve G1UpdateRSOrPushRefClosure

[Erik Helin]

On 06/28/2017 10:25 AM, Thomas Schatzl wrote:
> Hi all,
>
>   Erik suggested a few more refactorings:
>
> - rename G1ParClosureSuper -> G1ScanClosureBase
> - rename a few "oops_in_heap_closure" parameter -> "update_rs_cl"
> - move instantiation of closures from oops_into_collection_set_do()
> into scan_rem_set()/update_rem_set() methods.
>
> I assume these are the final ones :)
>
> Webrevs:
> http://cr.openjdk.java.net/~tschatzl/8175554/webrev.3/ (full)
> http://cr.openjdk.java.net/~tschatzl/8175554/webrev.2_to_3/ (diff)

Thank you Thomas, this looks really nice now! Reviewed and ready to go :)

Thanks,
Erik

> Testing:
> jprt
>
> Thanks,
>   Thomas
>
> On Thu, 2017-06-22 at 12:44 +0200, Thomas Schatzl wrote:
>> Hi all,
>>
>>   after discussion with Erik, I removed one comment, and renamed the
>> closures to something that resembles their use. Also I had to
>> reintroduce the G1ParPushRefClosure removed in the initial patch due
>> to
>> performance regressions.
>>
>> G1UpdateOrScanRSClosure -> G1ScanObjsDuringUpdateRSClosure
>> G1ParPushRefClosure -> G1ScanObjsDuringScanRSClosure
>> G1ParScanClosure -> G1ScanEvacuatedObjClosure
>>
>> We also found that the mechanism to collect cards that contain
>> references into the collection set to not lose any remembered set
>> entries during update RS if there is an evacuation failure is
>> basically
>> superfluous. Other, existing mechanism make sure that all required
>> remembered sets are (re-)created in other stages of the GC.
>>
>> Removal of this code has been decided to be out of scope here.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~tschatzl/8175554/webrev.1_to_2/ (diff)
>> http://cr.openjdk.java.net/~tschatzl/8175554/webrev.2/ (full)
>> Testing:
>> jprt, local testing
>>
>> Thanks,
>>   Thomas
>>
>>
>> On Tue, 2017-06-20 at 10:05 +0200, Thomas Schatzl wrote:
>>>
>>> Hi Sangheon, others,
>>>
>>> On Tue, 2017-05-30 at 15:15 -0700, sangheon wrote:
>>>>
>>>>
>>>> Hi Thomas,
>>>>
>>>> On 05/05/2017 05:13 AM, Thomas Schatzl wrote:
>>>>>
>>>>>
>>>>>
>>>>> Hi all,
>>>>>
>>>>>    recent reviews have made changes necessary to parts of the
>>>>> changeset chain.
>>>>>
>>>>> Here is a list of links to updated webrevs. Since they have
>>>>> apparently not been reviewed yet, I simply overwrote the old
>>>>> webrevs.
>>>>>
>>>>> JDK-8177044: Remove _scan_top from HeapRegion
>>>>> http://cr.openjdk.java.net/~tschatzl/8177044/webrev/
>>>>>
>>>>> JDK-8178148: Log more detailed information about scan rs phase
>>>>> http://cr.openjdk.java.net/~tschatzl/8178148/webrev/
>>>>>
>>>>> JDK-8175554: Improve G1UpdateRSOrPushRefClosure
>>>>> http://cr.openjdk.java.net/~tschatzl/8175554/webrev/
>>>> Looks good to me.
>>>> I only have minor nits.
>>>>
>>>> ------------------------------------------------------
>>>> src/share/vm/gc/g1/g1OopClosures.hpp
>>>>    78   virtual void do_oop(oop* p) { do_oop_nv(p); }
>>>> Misaligned with above line.
>>>>
>>>> ------------------------------------------------------
>>>> src/share/vm/gc/g1/g1RemSet.hpp
>>>>   204                   G1UpdateOrScanRSClosure* push_heap_cl,
>>>> Rename to reflect new closure name?
>>>>
>>>> ------------------------------------------------------
>>>> src/share/vm/gc/g1/g1RootProcessor.hpp
>>>> Copyright update.
>>>>
>>>> ------------------------------------------------------
>>>> src/share/vm/gc/g1/g1_specialized_oop_closures.hpp
>>>>    45       f(G1UpdateOrScanRSClosure,_nv)         \
>>>> Misaligned '\'.
>>>>
>>>   I fixed all this in addition to incorporating ErikD's comments
>>> that
>>> asked for factoring out two parts of the G1ParScanClosure and
>>> G1UpdateOrScanRSClosure that were equal now.
>>>
>>> I did some performance testing again due to that, and also found
>>> that
>>> the check to filter out non-cross-region references
>>> in G1UpdateOrScanRSClosure::do_oop_nv() seemed faster, so I also
>>> reverted it to the old code.
>>>
>>> Also in this change G1UpdateOrScanRSClosure::do_oop_nv() did not
>>> update
>>> _has_refs_into_cset as before. Fixed that as well.
>>>
>>> Thanks,
>>>   Thomas
>>>