[9] Review request : JDK-8059070: [TESTBUG] java/lang/invoke/LFCaching/LFMultiThreadCachingTest.java failed - timeout

[Konstantin Shefov]

Hi,

I have updated the webrev: 
http://cr.openjdk.java.net/~kshefov/8059070/webrev.01/

-Konstantin

16.10.2014 17:24, Igor Ignatyev пишет:
> Konstantin,
>
> I haven't looked at code religiously, so I wouldn't say that I have 
> reviewed it. however I have some comments, please see them inline.
>
> On 10/16/2014 02:03 PM, Konstantin Shefov wrote:
>> Paul,
>>
>> Thanks for reviewing
>>
>> In the jtreg scripts of the three existing LFCaching tests timeout is
>> set explicitly to 300 seconds. The file currently being changed is not a
>> test itself, it is parent class of tests.
>> In fact we can unset this explicit timeout and use the current fix
>> together with default JTREG timeout and jtreg timeout factor option.
>> These test cases are randomly generated, so the more iterations, the
>> better, but in fact we need at least one iteration.
>>
>> As for "if (avgIterTime > 2 * remainTime)", I think 2 is enough.
>>
>> -Konstantin
>>
>> On 16.10.2014 13:30, Paul Sandoz wrote:
>>> On Oct 16, 2014, at 10:43 AM, Konstantin Shefov
>>> <konstantin.shefov at oracle.com> wrote:
>>>
>>>> Gently reminder
>>>>
>>>> On 14.10.2014 16:58, Konstantin Shefov wrote:
>>>>> Hello,
>>>>>
>>>>> Please review the test bug fix
>>>>> https://bugs.openjdk.java.net/browse/JDK-8059070
>>>>> Webrev is http://cr.openjdk.java.net/~kshefov/8059070/webrev.00/
>>>>>
>>>    45     private static final long TIMEOUT = 300000;
>>>
>>> Does jtreg define a system property for this value? and is 300s the
>>> same as what jtreg defines as the default?
> unfortunately, jtreg doesn't define a property for timeout, but I 
> think it should do. we need to file an RFE against jtreg to add such a 
> property and an RFE against these tests/testlibrary to use this 
> property. could you please file them?
>>>
>>> The online documentation (jtreg -onlineHelp) says the default is 2
>>> minutes, but that might be out of date.
>>>
>>> Might be more readable to use:
>>>
>>>    TimeUnit.SECONDS.toMillis(300);
>>>
>>>
>>>   143             long passedTime = new Date().getTime() - startTime;
>>>
>>> Why don't you use System.currentTimeMillis() instead of "new
>>> Date().getTime()" ?
>>>
>>>
>>>   145             double timeoutFactor = new
>>> Double(System.getProperty("test.timeout.factor", "1.0"));
>>>
>>> You can that pull out into a static and perhaps merge with TIMEOUT.
> + you should use jdk.testlibrary.Utils::adjustTimeout instead of 
> writing this code again.
>>>
>>>
>>>   147             if (avgIterTime > 2 * remainTime) {
>>>
>>> That seems sufficient but it will be interesting to see if
>>> intermittent failures still occur due to high variance.
>>>
>>> Paul.
>>
>
>
> Igor