RFR: 8183124: Remove OopsInHeapRegionClosure

[Erik Helin]

On 06/28/2017 04:53 PM, Thomas Schatzl wrote:
> Hi,
>
> On Wed, 2017-06-28 at 15:30 +0200, Erik Helin wrote:
>> ...and now with subject as well :)
>>
>> Erik
>>
>> On 06/28/2017 02:59 PM, Erik Helin wrote:
>>>
>>> Hi all,
>>>
>>> this small patch removes the class OopsInHeapRegionClosure.
>>> OopsInHeapRegionClosure only contains a protected _from field and
>>> the
>>> public method set_from, and there are only two other classes
>>> inheriting
>>> from OopsInHeapRegionClosure (G1ScanClosureBase and
>>> UpdareRsetDeferred).
>>>
>>> This patch gets rid of the class OopsInHeapRegionClosure and adds
>>> the
>>> corresponding field and method to the classes inheriting from
>>> OopsInHeapRegionClosure.
>>>
>>> Patch: http://cr.openjdk.java.net/~ehelin/8183124/00/
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8183124
>>>
>>> Testing: make jprt
>
> from my POV there are two reasons here:
>
> - the additional class only for that field adds more overhead in
> various aspects than just duplicating the member.

Yeah, thanks for clarifying. My motivation for this patch was mainly to 
get rid of the awkward inheritance. Having OopsInHeapRegionClosure is 
kind of like we would have a ClosureWithG1FieldClosure with a 
G1CollectedHeap* _g1h field, because many G1 closures has a _g1h field. 
This sort of code de-duplication is IMO worse than just having the field 
in multiple closures.

> - using _from in UpdateRSDeferred is due to current way of checking for
> cross-region pointers, using the _from value. It kind of saves us from
> recreating it. However there are better options here that will fix both
> JDK-8183127 and remove the need for the _from pointer completely.

This is a great idea, we should use HeapRegion::is_in_same_region (this 
will also make the code more similar to G1ParScanThreadState::update_rs).

Thanks,
Erik

> So overall, this change seems good to me.
>
> Thanks,
>   Thomas
>