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

[Bengt Rutisson]

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
>>> the super class, but is now actually not initialized until after the
>>> call to the super constructor. I would have preferred the order as it
>>> was before. I guess the new order works fine as long as the super
>>> constructor never tries to use the _scanner.
>> You're right. I wanted the constructor to be called first but I was
>> clearly a bit careless and I missed the dependency. Thanks for looking
>> at this carefully! Even though the dependency is benign here (the ref to
>> _scanner is only stored locally in the super class) I think we should
>> avoid and future surprises. So I'll undo the change.
> I learned this the hard way:  the order you write it in the source
> doesn't affect the initialization order.  The initialization order is
> defined by the declaration order (i.e., superclasses are initialized
> first in the order they appear in the class declaration, followed by
> data members in the order they're declared in the class).
>
> 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 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?

Bengt


>
> -John