RFR: 8144505: Change G1ParCopyHelper to inherit OopClosure

[Stefan Karlsson]

Hi Stefan,

On 2015-12-02 17:33, Stefan Johansson wrote:
>
>
> On 2015-12-02 17:28, Mikael Gerdin wrote:
>> Hi Stefan,
>>
>> On 2015-12-02 16:55, Stefan Johansson wrote:
>>> Hi,
>>>
>>> Please review this change for:
>>> https://bugs.openjdk.java.net/browse/JDK-8144505
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~sjohanss/8138888/8144505/hotspot.00/
>>
>>   88 // Add back base class for metadata
>>   89 class G1ParCopyHelper : public OopClosure {
>>   90 protected:
>>   91   uint _worker_id;              // Cache value from par_scan_state.
>>   92   Klass* _scanned_klass;
>>   93   G1CollectedHeap* _g1;
>>   94   ConcurrentMark* _cm;
>>   95   G1ParScanThreadState* _par_scan_state;
>>   96
>>
>> That's a weird ordering of the fields, why not just move the heap an 
>> par scan state pointers to before the other fields, just as if they 
>> were still in the parent class?
>>
> I agree, fixed.
>
> New webrev:
> http://cr.openjdk.java.net/~sjohanss/8138888/8144505/hotspot.01/

Looks good.

StefanK

>
> Thanks for reviewing,
> Stefan
>
>> Otherwise it looks good.
>> /Mikael
>>
>>>
>>> Background:
>>> When iterating objects with oop_iterate() there is a special case for
>>> InstanceRefKlass, if the closure overrides
>>> apply_to_weak_ref_discovered_field() to return true, the closure 
>>> will be
>>> applied to the discovered field, even if the normal checks prevents it.
>>> This special treatment is needed in certain situations, but it is not
>>> obvious if it should be done when iterating the object. Currently only
>>> G1 closures override apply_to_weak_ref_discovered_field() and many of
>>> them seem to do it without any reason. My initial plan [1] was to 
>>> remove
>>> all usage of this special casing but after discussions I realized it
>>> would be easier to see the effects if doing this one closure at a time.
>>>
>>> Summary:
>>> G1ParCopyClosure is only used as a normal OopClosure. It inherits from
>>> G1ParCopyHelper which in turn inherits G1ParClosureSuper.
>>> G1ParClosureSuper overrides apply_to_weak_ref_discovered_field() to
>>> return true but since G1ParCopyClosure is never used with oop_iterate()
>>> it doesn't need to inherit this functionality. G1ParCopyHelper can
>>> instead be changed to inherit directly from OopClosure, and this way we
>>> have one closure less depending on 
>>> apply_to_weak_ref_discovered_field().
>>>
>>> Testing:
>>> * JPRT
>>> * RBT
>>>
>>> Thanks,
>>> Stefan
>>>
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8138888
>>
>