RFR: 8065749: [TESTBUG]: gc/arguments/TestG1HeapRegionSize.java fails at nightly

[Bengt Rutisson]

On 2014-11-25 12:11, Evgeniya Stepanova wrote:
> Hi Bengt,
> Ok, file is reverted.
>
> http://cr.openjdk.java.net/~eistepan/8065749/webrev.01

Thanks, looks good.

>
> But please note that  test will run only if UseG1GC set. It will be 
> skipped otherwise (with no GC option set for example).

Agreed.

Bengt

>
> Thanks,
> Evgeniya Stepanova
>
>
> On 25.11.2014 13:12, Bengt Rutisson wrote:
>>
>> Hi Dima,
>>
>> On 2014-11-24 15:03, Dmitry Fazunenko wrote:
>>> Hi Bengt,
>>>
>>>
>>> On 24.11.2014 18:33, Bengt Rutisson wrote:
>>>>
>>>> Hi Evgeniya,
>>>>
>>>> On 2014-11-24 14:13, Evgeniya Stepanova wrote:
>>>>> Hi,
>>>>>
>>>>> Could you please review changes for 8065749 ?
>>>>> Test TestG1HeapRegionSize.java is G1-specific and could not work 
>>>>> without -XX:+UseG1GC passed directly.
>>>>> Option added to the run.
>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8065749
>>>>> fix: http://cr.openjdk.java.net/~eistepan/8065749/webrev.00/
>>>>
>>>> I think your suggested fix will work, but I have a question 
>>>> regarding the original change.
>>>>
>>>> Before the original change the test explicitly checked that it was 
>>>> run with G1 and just skipped otherwise:
>>>>
>>>> http://hg.openjdk.java.net/jdk9/hs-gc/hotspot/comparison/6464714dd742/test/gc/arguments/TestG1HeapRegionSize.java
>>>>
>>>> Was this test really failing at all? I thought we were only going 
>>>> to add @requires to tests that were failing due to conflicting GC 
>>>> combinations. If it was working before I think a different approach 
>>>> to fix the current problem would be to revert back to the old code 
>>>> without @requires.
>>>
>>> You are right, it was our mistake. This test wasn't reverted when we 
>>> decided to update only failing tests.
>>> Yes, a possible solution would be just to roll back the previous 
>>> change, but I don't think it makes sense just to roll back.
>>>
>>> What I suggest:
>>> 1) keep this test G1 specific (@requires g1 and +UseG1 explicitly) - 
>>> what is proposed by this change
>>> 2) make tests GC independent - remove @requires and check in tests 
>>> that RegionSize == <expected> in case of G1, and RegionSize == 0 in 
>>> case of another GC.
>>>
>>> I don't think that original variant (silently pass if not g1) is the 
>>> right solution.
>>
>> I don't think 2) makes sense. G1HeapRegionSize is not set to 0 for 
>> other collectors and there is really no reason to do that either. So, 
>> testing the value of that with other GCs is just a waste of time.
>>
>> Considering that, I don't really see why 1) is better than reverting 
>> to the original code. After all, we are not sure how we want 
>> @requires to work and in my mind it is better to use it as little as 
>> possible until we have figured out how it should work.
>>
>> Thanks,
>> Bengt
>>
>>>
>>> What do you think?
>>>
>>> -- Dima
>>>
>>>
>>>
>>>
>>>
>>>>
>>>> Thanks,
>>>> Bengt
>>>>
>>>>>
>>>>> Thanks,
>>>>> Evgeniya Stepanova
>>>>>
>>>>> -- 
>>>>> /Evgeniya Stepanova/
>>>>>
>>>>>
>>>>
>>>
>>
>
> -- 
> /Evgeniya Stepanova/

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20141125/0fff4632/attachment.htm>