Re: RFR(S): 6977426: sun/tools tests can intermittently fail to find app's Java pid
The default JTreg time out is 5 minutes...
I believe the default JTREG timeout is 120 seconds or 2 minutes.
Dan
On 12/17/14 6:11 AM, 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.
The new webrev can be found here:
http://cr.openjdk.java.net/~ykantser/6977426/webrev.02/
// 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
Re: RFR(S): 6977426: sun/tools tests can intermittently fail to find app's Java pid
Hi, Thanks for all reviews! The new webrev can be found here: http://cr.openjdk.java.net/~ykantser/6977426/webrev.01/ // 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
Re: RFR(S): 6977426: sun/tools tests can intermittently fail to find app's Java pid
Adding Erik. On 12/17/2014 10:10 AM, Yekaterina Kantserova wrote: Hi, Thanks for all reviews! The new webrev can be found here: http://cr.openjdk.java.net/~ykantser/6977426/webrev.01/ // 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
Re: RFR(S): 6977426: sun/tools tests can intermittently fail to find app's Java pid
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
Re: RFR(S): 6977426: sun/tools tests can intermittently fail to find app's Java pid
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 ='.
(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.
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?
// 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
Re: RFR(S): 6977426: sun/tools tests can intermittently fail to find app's Java pid
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
Re: RFR(S): 6977426: sun/tools tests can intermittently fail to find app's Java pid
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.
The new webrev can be found here:
http://cr.openjdk.java.net/~ykantser/6977426/webrev.02/
// 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
Re: RFR(S): 6977426: sun/tools tests can intermittently fail to find app's Java pid
On 12/17/2014 02:11 PM, 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.
The new webrev can be found here:
http://cr.openjdk.java.net/~ykantser/6977426/webrev.02/
So, basically, instead of relying on the test timeout we will use the
harness timeout. Sounds legit.
Thumbs up!
-JB-
// 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
Re: RFR(S): 6977426: sun/tools tests can intermittently fail to find app's Java pid
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
RFR(S): 6977426: sun/tools tests can intermittently fail to find app's Java pid
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
Re: RFR(S): 6977426: sun/tools tests can intermittently fail to find app's Java pid
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
Re: RFR(S): 6977426: sun/tools tests can intermittently fail to find app's Java pid
You could use a lambda instead of a boolean and the duplicated
functions, i.e.
public static void main(String[] args) throws Exception {
if (args[0].equals(Logger)) {
testIfLeaking(TestLoggerWeakRefLeak::getLogger);
} else {
testIfLeaking(TestLoggerWeakRefLeak::getAnonymousLogger);
}
}
private static void testIfLeaking(Runnable getLogger) {
...
getLogger.run();
...
}
Erik
Yekaterina Kantserova skrev 2014-12-16 16:57:
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
Re: RFR(S): 6977426: sun/tools tests can intermittently fail to find app's Java pid
Hi Katja, Nice too see some more shell scripts go away! I wonder - shouldn't the test force a System.gc() at some point before taking the histogram? If not what guarantee do we have that the various weak references will be purged? best regards, -- daniel On 16/12/14 16:57, 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
Re: RFR(S): 6977426: sun/tools tests can intermittently fail to find app's Java pid
Hi Daniel, Thank you for reviewing the fix! That's what 'vm.heapHisto(-live)' does - it will request a full GC. Best regards, Katja On 12/16/2014 06:37 PM, Daniel Fuchs wrote: Hi Katja, Nice too see some more shell scripts go away! I wonder - shouldn't the test force a System.gc() at some point before taking the histogram? If not what guarantee do we have that the various weak references will be purged? best regards, -- daniel On 16/12/14 16:57, 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
