CRR (L / updated): 6888336: G1: avoid explicitly marking and pushing objects in survivor spaces

[Bengt Rutisson]

John,

With some good help from Mikael Gerdin I was able to work out how to fix 
the templates for G1ParCopyClosure to remove the need for G1ParCopyHelper.

Here is a webrev where I have removed the G1ParCopyHelper completely:
http://cr.openjdk.java.net/~brutisso/template-fix/webrev.01/

Should I file a bug and fix this, or should we leave it as it is? 
Personally I think that it is always nice to be able to reduce the 
inheritance graph.

Thanks,
Bengt

On 2012-01-12 21:55, John Coomes wrote:
> Bengt Rutisson (bengt.rutisson at oracle.com) wrote:
>> John,
>>
>> Inline...
>>
>> On 2012-01-12 02:16, John Coomes wrote:
>>> Tony Printezis (tony.printezis at oracle.com) wrote:
>>>> Bengt,
>>>>
>>>> Hi, thanks for looking at it! See inline.
>>>>
>>>> On 01/10/2012 04:41 PM, Bengt Rutisson wrote:
>>>>> ...
>>>>> In  g1OopClosures.hpp you swapped the lines 151 and 152, which makes
>>>>> it look like this:
>>>>>
>>>>>    149   G1ParCopyClosure(G1CollectedHeap* g1, G1ParScanThreadState*
>>>>> par_scan_state,
>>>>>    150                    ReferenceProcessor* rp) :
>>>>>    151       G1ParCopyHelper(g1, par_scan_state,&_scanner),
>>>>>    152       _scanner(g1, par_scan_state, rp) {
>>>>>
>>>>> I guess you want the call to the super class constructor before other
>>>>> initialization. To me it looks strange that the _scanner is passed to
>>>>> ...
>>> So the c++ compiler will call the G1ParCopyHelper ctor first, no
>>> matter how you write it :-).
>> That's good to know! Thanks for pointing this out.
>>
>> This makes the dependency between G1ParCopyHelper and G1ParCopyClosure
>> kind of scary to me. I agree with Tony that it is not an issue as it is
>> right now, but if anybody in the future will try to access the _scanner
>> field in the constructor of G1ParCopyHelper we are in trouble.
> I agree, it's a (minor) accident waiting to happen.
>
>> I don't think we need to fix it right away, but maybe we should think
>> about a fix for it. As far as I can tell the G1ParCopyHelper is only
>> needed to get around some template issues with G1ParCopyClosure. Maybe
>> there is a cleaner way to solve that?
> I think _scanner and its accessor can move to G1ParCopyHelper, and the
> ReferenceProcessor needed to initialize _scanner can be passed to the
> G1ParCopyHelper ctor.  I don't see any other types being used for
> _scanner, so making it a data member of G1ParCopyHelper instead of a
> pointer (the latter allows virtual dispatch) should be fine.
>
> -John