RFR: 8040935: -XX:+AggressiveOpts broken: GC triggered before VM initialization completed on several tests

[Bengt Rutisson]

Hi Stefan,

On 2015-01-15 16:10, Stefan Johansson wrote:
> Hi again,
>
> On 2015-01-15 15:12, Stefan Johansson wrote:
>> On 2015-01-15 00:49, Stefan Johansson wrote:
>>> Hi Kim,
>>>
>>> On 2015-01-14 19:13, Kim Barrett wrote:
>>>> On Jan 12, 2015, at 5:01 AM, Stefan Johansson 
>>>> <stefan.johansson at oracle.com> wrote:
>>>>> Please review this testfix to avoid the following issue:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8040935
>>>>>
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~sjohanss/8040935/hotspot.00
>>>>>
>>>>> Summary:
>>>>> Making the test a driver test to avoid dealing with externally 
>>>>> specified flags. One problematic flag is AggressiveOpts, it causes 
>>>>> the VM to allocate more stuff during startup and that together 
>>>>> with UseNUMA the eden space gets to little to handle all startup 
>>>>> allocations. This will in turn lead to a GC being triggered before 
>>>>> initialization is finished, and because of that failing the test.
>>>>>
>>>>> Testing:
>>>>> * Tested via JPRT
>>>> test/gc/TestNUMAPageSize.java
>>>> Summary: Changed test to not be affected by external options.
>>>>
>>>> I'm not sure I like this approach to resolving the bug.  I think it
>>>> results in many potentially interesting configurations never being
>>>> tested, e.g. this tests with one and only one GC, the default GC.  It
>>>> ignores the nightly build cycling through various configurations to
>>>> increase test coverage.
>>>>
>>> Thanks for taking a look at this. I see your point and think we 
>>> should do something better. The test was originally written to test 
>>> a specific regression we had when starting with -Xmx8m and 
>>> -XX:+UseNUMA with the default collector, but that doesn't mean we 
>>> shouldn't test it for the others as well. I'm not sure if we have 
>>> any NUMA support for the other collectors today, but no harm in 
>>> testing that the option doesn't cause any problems.
>>>
>>> I see two ways of improving the coverage, one would be to use 
>>> @requires to say that AggressiveOpts can't be true, but that will 
>>> still leave the possibility to set other options that might push us 
>>> over the limit of allocations during startup. The other way would be 
>>> to extend the test to run with all collectors explicitly, but ignore 
>>> any options passed in. This would of course still have less coverage 
>>> than today, but at least all collectors would be covered.
>>>
>>> I would prefer the second one, it would be robust and the coverage 
>>> would be ok.
>>>
>>
>> Updated webrev that tests all GCs:
>> http://cr.openjdk.java.net/~sjohanss/8040935/hotspot.01/
>>
>> Verified that the test still passes when run through JPRT.
>>
> I think I've changed my mind. After some discussions here in 
> Stockholm, and after seeing that gc/TestSmallHeap.java fails for the 
> same reason, I'm starting to think that @requires is the way to go. We 
> don't want to re-write all tests using small heaps just because it 
> doesn't work with AggressiveOpts, that will loose too much test 
> coverage and I think we should solve similar issues the same way.
>
> New proposal:
> http://cr.openjdk.java.net/~sjohanss/8040935/hotspot.02

I agree with this approach.

The change looks good to me.

Bengt

>
> Stefan
>
>> Thanks,
>> Stefan
>>
>>> Thanks,
>>> Stefan
>>>
>>
>