RFR(S): 8068739: G1CollectorPolicy uses uninitialized field '_sigma' in the constructor

[Volker Simonis]

On Mon, Jan 12, 2015 at 9:46 AM, Mikael Gerdin <mikael.gerdin at oracle.com> wrote:
> Hi Volker, Johannes
>
> On 2015-01-09 19:23, Volker Simonis wrote:
>>
>> Hi,
>>
>> could somebody please review and sponsor the following fix which was
>> contributed by my colleague Johannes Scheerer:
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2015/8068739/
>
>
> Good catch!
>
>> https://bugs.openjdk.java.net/browse/JDK-8068739
>>
>> I've already review the change and think it's good.
>
>
> +1, Reviewed.
>
>>
>> The G1CollectorPolicy constructor uses a huge initializer list. In
>> this list it calls "new SurvRateGroup(this, ..)" passing it a
>> reference to the not yet fully initialized G1CollectorPolicy instance.
>>
>> Via the following call chain, the SurvRateGroup constructor calls back
>> to G1CollectorPolicy::get_new_prediction() which uses the uninitialzed
>> value of G1CollectorPolicy::_sigma:
>>
>> G1CollectorPolicy::G1CollectorPolicy
>> SurvRateGroup::SurvRateGroup
>> SurvRateGroup::reset
>> SurvRateGroup::all_surviving_words_recorded
>> G1CollectorPolicy::get_new_prediction
>>
>> Depending on the indefinite value of '_sigma' this can lead to
>> situations, where a GC is triggered before the whole system is
>> initialized resulting in the following crash:
>>
>> Error occurred during initialization of VM
>> GC triggered before VM initialization completed. Try increasing
>> NewSize, current value 5324K.
>>
>> This is a "day 1" problem in G1. The funny thing is that the Visual
>> Studio compiler specially warns about this dangarous usage of the
>> 'this' pointer in the initializer list ("Compiler Warning C4355:
>> 'this' : used in base member initializer list") but unfortunately the
>> warning was disable in the file.
>
>
> Indeed. Unfortunately I'm not surprised, a lot of G1 code is still very
> messy and full of strange hacks which have no clear explanation.
>
>>
>> I have re-enabled this warning for g1CollectorPolicy.cpp because after
>> the fix it isn't necessary any more. The call to "new
>> SurvRateGroup(this, ..)" has been moved from the initializer list into
>> the constructor body. Notice that it wouldn't have helped to
>> initialize '_sigma' in the initializer list before the call to "new
>> SurvRateGroup(this, ..)" because according to the C++ standard,
>> members are initialized in the order how they are defined in a class
>> (and not in the initializer list order).
>>
>> I also can not understand why we have these huge initializer list for
>> many gc related class constructors. As far as I understand, this only
>> makes sense for class members because that saves a call to the default
>> constructor and one assignment. For basic and pointer types this makes
>> no sense however. Taking into account the maybe unexpected
>> initialization order of initialization lists, I would strongly
>> recommend to move all the member initializations into the constructor
>> bodies.
>
>
> I sort of disagree here, I'd prefer to keep initializations in initializer
> lists where it's possible since it implies that there are no special
> dependencies between the initializations. If I find initializations in the
> constructor body I expect to find some sort of conditionals or error
> handling code.
>

The problem is that the initialization order of the members is not the
one imposed by the initializer list, but rather the one imposed by the
declaration order of the members in the class definition which is in
another file. This is unintuitive and I'm not sure that everybody is
aware of this. I'd prefer to only initialize members with constant
values in the initializer list and not members which require other
members for initialization. But that's a different topic - for now
I've created the issue 8068883: Remove disabling of warning "C4355:
'this' : used in base member initializer list" and will remove the
initializations which require a 'this' pointer from the initializer
list.

>>
>> I also noticed that there are several other files which have disabled
>> warning 4355 and which use a not fully initialized 'this' pointer in
>> the initializer list:
>>
>> src/share/vm/gc_implementation/g1/concurrentMark.cpp:#pragma warning(
>> disable:4355 )
>> src/share/vm/gc_implementation/g1/dirtyCardQueue.cpp:#pragma warning(
>> disable:4355 )
>> src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp:#pragma warning(
>> disable:4355 )
>> src/share/vm/gc_implementation/g1/satbQueue.cpp:#pragma warning(
>> disable:4355 )
>> src/share/vm/gc_implementation/parNew/parNewGeneration.cpp:#pragma
>> warning( disable:4355 )
>> src/share/vm/gc_implementation/parNew/parNewGeneration.cpp:#pragma
>> warning( disable:4355 )
>>
>> I think it would be good idea to review all these locations, refactor
>> them as suggested above and re-enable the warnings afterwards.
>
>
> Indeed. Can you please file an issue on JDK 9 for that and assign it to
> yourself (or the appropriate SAP engineer) if you guys plan to work on it?
>
> /Mikael
>
>
>>
>> Regards,
>> Volker
>>
>