RFR 8067655: Clean up G1 remembered set oop iteration

[Mikael Gerdin]

Hi Jon,

On 2014-12-17 01:09, Jon Masamitsu wrote:
> http://cr.openjdk.java.net/~mgerdin/8067655/webrev.0/src/share/vm/gc_implementation/g1/heapRegion.hpp.frames.html
>
>
> Are we trying to move toward all parameters on 1 line or
> 1 line per parameter?
>
>    84   HeapRegionDCTOC(G1CollectedHeap* g1,
>    85                   HeapRegion* hr, G1ParPushHeapRSClosure* cl,
>    86                   CardTableModRefBS::PrecisionStyle precision);
>
>
> Same here.
>
> http://cr.openjdk.java.net/~mgerdin/8067655/webrev.0/src/share/vm/gc_implementation/g1/heapRegion.cpp.frames.html
>
>
>    50 HeapRegionDCTOC::HeapRegionDCTOC(G1CollectedHeap* g1,
>    51                                  HeapRegion* hr,
> G1ParPushHeapRSClosure* cl,
>    52                                  CardTableModRefBS::PrecisionStyle
> precision) :
>
>
> Otherwise, looks good.

I don't mind fixing these two occurences.

Thanks for the review.

/Mikael

>
> Jon
>
> On 12/16/2014 06:33 AM, Mikael Gerdin wrote:
>> Hi all,
>>
>> while reading through the code which iterates over oops pointing into
>> the collection set I noticed that the closure used to implement the
>> iteration, G1ParPushHeapRSClosure, is hooked into the
>> de-virtualization macros with oop_oop_iterate_nv but the static type
>> is not preserved all the way down to the call site where oop_iterate
>> is called, so the de-virtualization cannot work.
>>
>> I fixed the code to pass down the correct type and while doing that I
>> had to clean up some dead code, such as the FilterKind parameter to
>> the HeapRegionDCTOC which was only ever used with the
>> IntoCSFilterKind. The filtering is not needed at all since
>> G1ParPushHeapRSClosure already does a collection set check so I
>> removed the filtering altogether.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8067655
>> Webrev: http://cr.openjdk.java.net/~mgerdin/8067655/webrev.0/
>>
>> Testing:
>> JPRT, jbb2005 on x86_64 and sparc.
>> The jbb runs showed no significant changes and the ScanRS time changes
>> are inconclusive but I still think this is a nice cleanup change. At
>> least this change makes it clear that it's actually only one closure
>> type which is used by the HeapRegionDCTOC.
>>
>> /Mikael
>