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

[Evgeniya Stepanova]

Bengt, Jesper

Thank you for the review!

Evgeniya
On 25.11.2014 17:01, Bengt Rutisson wrote:
>
> 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/
>

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