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

Evgeniya Stepanova evgeniya.stepanova at oracle.com
Tue Nov 25 11:11:59 UTC 2014


Hi Bengt,
Ok, file is reverted.

http://cr.openjdk.java.net/~eistepan/8065749/webrev.01

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

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: <http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20141125/56d2a858/attachment.html>


More information about the hotspot-gc-dev mailing list