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

[Konstantin Shefov]

Gently reminder

On 17.10.2014 13:38, Konstantin Shefov wrote:
> 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
>