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

[John Coomes]

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