[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR(S): 6977426: sun/tools tests can intermittently fail to find app's Java pid
From:       Daniel Fuchs <daniel.fuchs () oracle ! com>
Date:       2014-12-17 13:36:51
Message-ID: 549186F3.5070608 () oracle ! com
[Download RAW message or body]

Hi Katja,

On 17/12/14 14:11, Yekaterina Kantserova wrote:
>
> Hopefully the last version :)
>
> Erik has recommended to skip the whole timeout concept. Instead the test
> will loop until a decreasing count is found. Otherwise the test will
> timeout. The default JTreg time out is 5 minutes so it would be enough
> even for slower configurations.

Good suggestion. I should have thought of that :-)

> The new webrev can be found here:
> http://cr.openjdk.java.net/~ykantser/6977426/webrev.02/

Thumbs up from me!

cheers,

-- daniel

>
> // Katja
>
>
>
> On 12/17/2014 12:33 PM, Daniel Fuchs wrote:
>> On 17/12/14 12:05, Yekaterina Kantserova wrote:
>>> Daniel,
>>>
>>> It's really funny to get such a feedback!
>>>
>>> (1) The output from test right now looks like:
>>>
>>> ----------System.out:(7/183)----------
>>> call count = 1000
>>> instance count: 1001
>>> call count = 2000
>>> instance count: 1001
>>> Finishing early due to non-increasing instance count
>>> increasing count: 1
>>> decreasing or the same count: 1
>>>
>>> so printing the value of 'count' is already in there: 'call count ='.
>>
>> I meant to print the final value for count - but OK - having the
>> 'call count' printed every 1000 count should be enough :-)
>>
>>
>>> (2) In the case we want to have an extra condition I think it should be
>>> 'count > 2000'. Otherwise if the end time is passed the 'count > 1000'
>>> will result in having just one iteration (we will left the loop on 1100)
>>> and test will fail on assert.
>>
>> right. my mistake. you need at least two histograms.
>>
>>> The test timeout value should be increased as well. Right now it's 120 +
>>> 30 sec.
>>>
>>>   * @run main/timeout=150 TestLoggerWeakRefLeak Logger
>>>
>>> If the test will not be completed in 120 s the extra 30 s will not make
>>> the trick and JTreg will interrupt it. What do you think will be enough?
>>
>> I am not sure - but I guess what you have now should be good enough
>> for starter. In the problematic configurations there often is
>> an additional timeout factor - so in such case the real jtreg timeout
>> would I believe be something like 150*4 anyway - which should hopefully
>> leave time enough for the while loop to take two histograms. Simply
>> adding the extra condition to keep looping until we have at least two
>> should do the trick.
>>
>> best regards, and thanks again for taking care of 6977426!
>>
>> -- daniel
>>
>>>
>>> // Katja
>>>
>>>
>>>
>>> On 12/17/2014 10:55 AM, Daniel Fuchs wrote:
>>>> Hi Katja,
>>>>
>>>> Sorry for not thinking of that when I replied earlier.
>>>> Your new test is so much more readable than the old shell
>>>> script :-)
>>>>
>>>> These are minor  suggestions but they might help analysis
>>>> if the test fails:
>>>>
>>>> On 17/12/14 10:10, Yekaterina Kantserova wrote:
>>>>> Hi,
>>>>>
>>>>> Thanks for all reviews!
>>>>>
>>>>> The new webrev can be found here:
>>>>> http://cr.openjdk.java.net/~ykantser/6977426/webrev.01/
>>>>
>>>> Not that I believe it's very likely to happen, but I wonder
>>>> if the condition to exit the while loop:
>>>>
>>>>   79         while (now < (startTime + (LOOP_TIME_SEC * 1000))) {
>>>>
>>>> should also make sure that count > 1000 - and continue if not.
>>>> (Thinking of passed timeout related issues when tests where run
>>>>  with -Xcomp on fastdebug builds during integration nightlies)
>>>>
>>>> Possibly printing the value of 'count' before the assert
>>>> at line 103 could help with debugging too, if the test actually
>>>> fails (even though the combination of increasingCount
>>>> + decreasingCount would give us a hint)
>>>>
>>>> best regards,
>>>>
>>>> -- daniel
>>>>
>>>>>
>>>>> // Katja
>>>>>
>>>>>
>>>>>
>>>>> On 12/16/2014 05:41 PM, Jaroslav Bachorik wrote:
>>>>>> Hi Katja,
>>>>>>
>>>>>> This request should probably go to core-libs as well.
>>>>>>
>>>>>> Other than that I have just a few nits:
>>>>>>
>>>>>> test/java/util/logging/TestLoggerWeakRefLeak.java
>>>>>>
>>>>>> L57: if ("Logger".contains(args[0])) -> "Logger".equals(args[0]) ?
>>>>>> L127: // in LogManager.loggers that *arebeing* properly managed ->
>>>>>> *are being*
>>>>>>
>>>>>> callLoggerAndCount() and callAnonymousLoggerAndCount() are using the
>>>>>> 'count' variable to always return 100. Could they be made to return a
>>>>>> constant instead?
>>>>>>
>>>>>>
>>>>>> Cheers,
>>>>>>
>>>>>> -JB-
>>>>>>
>>>>>>
>>>>>> On 12/16/2014 04:57 PM, Yekaterina Kantserova wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Could I please have a review of this fix.
>>>>>>>
>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-6977426
>>>>>>> webrev: http://cr.openjdk.java.net/~ykantser/6977426/webrev.00/
>>>>>>>
>>>>>>> java/util/logging/LoggerWeakRefLeak.sh and
>>>>>>> java/util/logging/AnonLoggerWeakRefLeak.sh are merged and
>>>>>>> rewritten in
>>>>>>> java. sun/tools/common/CommonTests.sh is removed because it doesn't
>>>>>>> test
>>>>>>> the product but sun/tool/common library functionality.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Katja
>>>>>>
>>>>>
>>>>
>>>
>>
>

[prev in list] [next in list] [prev in thread] [next in thread]