RFR: 8144505: Change G1ParCopyHelper to inherit OopClosure

[Stefan Johansson]

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/

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
>