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
RFR 8067447: Factor out the shared implementation of the VM flags manipulation code
Please, review the following change. Issue : https://bugs.openjdk.java.net/browse/JDK-8067447 Webrev: http://cr.openjdk.java.net/~jbachorik/8067447/webrev.00 This patch is a precursor for implementing https://bugs.openjdk.java.net/browse/JDK-8054890 which itself is a part of JEP-228 (https://bugs.openjdk.java.net/browse/JDK-8043764). Here, the code related to manipulating JVM flags is extracted to a separate ManagedFlags class and the codebease is adjusted to use this class. Thanks, -JB-
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(XS): 8066814: Reduce accessibility in TraceEvent
Greetings, Kindly asking for reviews for the following changeset: Bug: https://bugs.openjdk.java.net/browse/JDK-8066814 Webrev: http://cr.openjdk.java.net/~mgronlun/8066814/webrev01/ Description: TraceEvent currently exposes internals unnecessarily. Therefore: Remove unnecessarily exposed methods. Add assert for not committing a cancelled event. Add method stubs for !INCLUDE_TRACE Thanks in advance Markus
Re: RFR(XS): 8066814: Reduce accessibility in TraceEvent
Looks good Erik Markus Grönlund skrev 2014-12-17 15:45: Greetings, Kindly asking for reviews for the following changeset: Bug: https://bugs.openjdk.java.net/browse/JDK-8066814 Webrev: http://cr.openjdk.java.net/~mgronlun/8066814/webrev01/ Description: TraceEvent currently exposes internals unnecessarily. Therefore: Remove unnecessarily exposed methods. Add assert for not committing a cancelled event. Add method stubs for !INCLUDE_TRACE Thanks in advance Markus
RFR(S): backport 8039995 and 8061785 changes to SA testcase
Hi, I'd like a review of a backport of these test changes, into 8u. webrev: http://cr.openjdk.java.net/~kevinw/8039995_8061785/webrev.00/ Changes simply hg imported from: 8039995 http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/raw-rev/da92e4c42b24 8061785 http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/raw-rev/3cdb9f480a8c Thanks! Kevin
2-nd round RFR (S) 8008678: JSR 292: constant pool reconstitution must support pseudo strings
Please, review the second round fix for: https://bugs.openjdk.java.net/browse/JDK-8008678 Open webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo.2/ Summary: This fix implements a footprint saving approach suggested by Coleen. To be able to reconstitute a class constant pool, an intermediate pseudo-string map is used. Now, this field is accounted optionally, only if the 'cp_patches' is provided in the ClassFileParser::parseClassFile() before ConstantPool is allocated. This fix is not elegant, even a little bit ugly, but it is the only way I see so far. Unfortunately, this approach did not help much to make some other fields (eg., 'operands') optional. The problem is that we have to account optional fields before parsing, at the CP allocation time. It is possible to re-allocate the ConstantPool when any InvokeDynamic bytecode is discovered, but it looks too complicated. Testing: - the unit test from bug report - nsk.jvmti,testlist, nsk.jdi.testlist, JTREG java/lang/instrument - vm.mlvm.testlist, vm.quick.testlist, vm.parallel_class_loading.testlist (in progress) Thanks, Serguei On 11/26/14 11:53 AM, serguei.spit...@oracle.com wrote: Coleen, Thank you for looking at this! I'll check how this can be improved. It is my concern too. Thanks, Serguei On 11/26/14 9:17 AM, Coleen Phillimore wrote: Serguei, I had a quick look at this. I was wondering if we could make the pseudo_string_map conditional in ConstantPool and not make all classes pay in footprint for this field? The same thing probably could be done for operands too. There are flags that you can set to conditionally add a pointer to base() in this function. Typical C++ would subclass ConstantPool to add InvokeDynamicConstantPool fields, but this is not typical C++ so the trick we use is like the one in ConstMethod. I think it's worth doing in this case. Thanks, Coleen On 11/26/14, 3:59 AM, serguei.spit...@oracle.com wrote: Please, review the fix for: https://bugs.openjdk.java.net/browse/JDK-8008678 Open webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2014/hotspot/8008678-JVMTI-pseudo.1/ Summary: The pseudo-strings are currently not supported in reconstitution of constant pool. This is an explanation from John Rose about what the pseudo-strings are: We still need live oop constants pre-linked into the constant pool of bytecodes which implement some method handles. We use the anonymous class pseudo-string feature for that. The relevant code is here: http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/tip/src/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java These oops are what pseudo-strings are. The odd name refers to the fact that, even though they are random oops, they appear in the constant pool where one would expect (because of class file syntax) to find a string. ... If you really wanted to reconstitute a class file for an anonymous class, and if that class has oop patching (pseudo-strings), you would need either to (a) reconstitute the patches array handed to Unsafe.defineAnonymousClass, or (b) accept whatever odd strings were there first, as an approximation. The odd strings are totally insignificant, and are typically something like CONSTANT_PLACEHOLDER_42 (see java/lang/invoke/InvokerBytecodeGenerator.java). Reconstitution of the ConstantPool is needed for both the JVMTI GetConstantPool() and RetransformClasses(). Finally, it goes to the ConstantPool::copy_cpool_bytes(). The problem is that a pseudo-string is a patched string that does not have a reference to the string symbol anymore: unresolved_string_at(idx) == NULL The fix is to create and fill in a map from JVM_CONSTANT_String cp index to the JVM_CONSTANT_Utf8 cp index to be able to restore this assotiation in the JvmtiConstantPoolReconstituter. Testing: Run: - java/lang/instrument tests - new jtreg test (see webrev) that was written by Filipp Zhinkin Thanks, Serguei
Re: RFR(L): 8049716: PPC64: Implement SA on Linux/PPC64
Hi Dmitry,
once again, thanks for your detailed review. You can find the new
version of the webrev under:
http://cr.openjdk.java.net/~simonis/webrevs/8049716.v2/
I've rebased it to the latest jdk9/hs-rt repository today.
I hope I adressed all your concerns. Please find my additional
comments inline below:
And I still need a sponsor for pushing this reviewed change. Any volunteers:)
Thank you and best regards,
Volker
Below is fist part of review - shared files.
* agent/make/Makefile - no comments
* agent/src/os/linux/LinuxDebuggerLocal.c - no comments
* agent/src/os/linux/symtab.c:
438:
- What is the magic of symbols that starts with '.' ?
- As far as I understand you are getting indirect value from opd section.
There's already a very detailed descrition of the whole story in
hotspot/src/share/vm/utilities/elfFuncDescTable.hpp and I've added a
reference to that file in the comment now.
Could you reformat it a bit to have it better readable?
Something like:
uintptr_t sym_value;
...
symvalue = syms-st_value
#ifdef ppc64
// Some comments here
// ppc specific code here
sym_value =
#endif
symtab-symbols[j].offset = sym_value - baseaddr;
Good point. Done as suggested by you.
454:
I appreciate detailed comments here.
if (false) can cause unreachable code warning, and unused variable
warning so it might be better to add #ifdef ppc64 on caller
site at ll. 516 and leave here only a comment.
But if you decide to guard against try_debuginfo please replace
if (false)
to
goto quit
I think there's already a comment in place. The problem is that the
debuginfo file only has an empty .opd section. So if we would use the
debuginfo file for symbol resolution we would still need the .opd
section from the regular library. This doesn't fit in the current
function work flow which only uses ELF data from a single file and I
didn’t wanted to change that.
But your suggestion of using 'goto quit' is good and I've used that version.
*
agent/src/share/classes/sun/jvm/hotspot/debugger/MachineDescriptionPPC64.java
38: return (endian.equals(big));
is enough
I've used the even shorter version proposed by Alexander Smundak:
return big.equals(System.getProperty(sun.cpu.endian))
*
agent/src/share/classes/sun/jvm/hotspot/debugger/linux/LinuxCDebugger.java
- no comments
*
agent/src/share/classes/sun/jvm/hotspot/debugger/linux/LinuxThreadContextFactory.java
- no comments
*
agent/src/share/classes/sun/jvm/hotspot/debugger/proc/ProcDebuggerLocal.java
- no comments
*
agent/src/share/classes/sun/jvm/hotspot/debugger/remote/RemoteDebuggerClient.java
- no comments
* agent/src/share/classes/sun/jvm/hotspot/runtime/Threads.java - no comments
* agent/src/share/classes/sun/jvm/hotspot/runtime/VFrame.java - no comments
* make/linux/makefiles/sa.make - no comments
* make/sa.files - no comments
* src/share/vm/runtime/vmStructs.cpp
- no comments
This is part two of review. Code looks good for me, only minor nits.
*
agent/src/share/classes/sun/jvm/hotspot/debugger/linux/ppc64/LinuxPPC64CFrame.java:
41 missed space around =
51 extra space between pc
Done.
*
agent/src/share/classes/sun/jvm/hotspot/debugger/linux/ppc64/LinuxPPC64ThreadContext.java
- no comments
*
agent/src/share/classes/sun/jvm/hotspot/debugger/ppc64/PPC64ThreadContext.java
47, 48 extra space before =
Done.
*
agent/src/share/classes/sun/jvm/hotspot/debugger/proc/ppc64/ProcPPC64Thread.java
34 extra space before id
42 extra space before = ,
Done.
it might be better to create a constant for size of integer.
I agree, but this code was just copied from the corresponding AMD64
version and as far as I can see all the other architectures do it the
same way so I didn't change it.
*
agent/src/share/classes/sun/jvm/hotspot/debugger/proc/ppc64/ProcPPC64ThreadContext.java
- no comments
*
agent/src/share/classes/sun/jvm/hotspot/debugger/proc/ppc64/ProcPPC64ThreadFactory.java
-no comments
*
agent/src/share/classes/sun/jvm/hotspot/debugger/remote/ppc64/RemotePPC64Thread.java
43 missed space before ?
it might be be better to reformat this line to usual if and add comments
The same here - this is copied code. But I've adjusted it to at least
match with the other platforms.
*
agent/src/share/classes/sun/jvm/hotspot/debugger/remote/ppc64/RemotePPC64ThreadContext.java
- no comments
*
agent/src/share/classes/sun/jvm/hotspot/debugger/remote/ppc64/RemotePPC64ThreadFactory.java
- no comments
*
agent/src/share/classes/sun/jvm/hotspot/runtime/linux_ppc64/LinuxPPC64JavaThreadPDAccess.java
57, 58 extra space before =
63 and below extra space after public
108 unaligned comments
*
agent/src/share/classes/sun/jvm/hotspot/runtime/ppc64/PPC64CurrentFrameGuess.java
49 Formatting in PPC64Frame.java looks much better for me.
40 private static final boolean DEBUG;
41 static {
42 DEBUG = ...
43 }
60, 61 extra
Re: RFR(L): 8049716: PPC64: Implement SA on Linux/PPC64
Volker, The changes looks good for me and I'll sponsor the push. But please check, whether you need one more reviewer or not. -Dmitry On 2014-12-17 20:37, Volker Simonis wrote: Hi Dmitry, once again, thanks for your detailed review. You can find the new version of the webrev under: http://cr.openjdk.java.net/~simonis/webrevs/8049716.v2/ I've rebased it to the latest jdk9/hs-rt repository today. I hope I adressed all your concerns. Please find my additional comments inline below: And I still need a sponsor for pushing this reviewed change. Any volunteers:) Thank you and best regards, Volker Below is fist part of review - shared files. * agent/make/Makefile - no comments * agent/src/os/linux/LinuxDebuggerLocal.c - no comments * agent/src/os/linux/symtab.c: 438: - What is the magic of symbols that starts with '.' ? - As far as I understand you are getting indirect value from opd section. There's already a very detailed descrition of the whole story in hotspot/src/share/vm/utilities/elfFuncDescTable.hpp and I've added a reference to that file in the comment now. Could you reformat it a bit to have it better readable? Something like: uintptr_t sym_value; ... symvalue = syms-st_value #ifdef ppc64 // Some comments here // ppc specific code here sym_value = #endif symtab-symbols[j].offset = sym_value - baseaddr; Good point. Done as suggested by you. 454: I appreciate detailed comments here. if (false) can cause unreachable code warning, and unused variable warning so it might be better to add #ifdef ppc64 on caller site at ll. 516 and leave here only a comment. But if you decide to guard against try_debuginfo please replace if (false) to goto quit I think there's already a comment in place. The problem is that the debuginfo file only has an empty .opd section. So if we would use the debuginfo file for symbol resolution we would still need the .opd section from the regular library. This doesn't fit in the current function work flow which only uses ELF data from a single file and I didn’t wanted to change that. But your suggestion of using 'goto quit' is good and I've used that version. * agent/src/share/classes/sun/jvm/hotspot/debugger/MachineDescriptionPPC64.java 38: return (endian.equals(big)); is enough I've used the even shorter version proposed by Alexander Smundak: return big.equals(System.getProperty(sun.cpu.endian)) * agent/src/share/classes/sun/jvm/hotspot/debugger/linux/LinuxCDebugger.java - no comments * agent/src/share/classes/sun/jvm/hotspot/debugger/linux/LinuxThreadContextFactory.java - no comments * agent/src/share/classes/sun/jvm/hotspot/debugger/proc/ProcDebuggerLocal.java - no comments * agent/src/share/classes/sun/jvm/hotspot/debugger/remote/RemoteDebuggerClient.java - no comments * agent/src/share/classes/sun/jvm/hotspot/runtime/Threads.java - no comments * agent/src/share/classes/sun/jvm/hotspot/runtime/VFrame.java - no comments * make/linux/makefiles/sa.make - no comments * make/sa.files - no comments * src/share/vm/runtime/vmStructs.cpp - no comments This is part two of review. Code looks good for me, only minor nits. * agent/src/share/classes/sun/jvm/hotspot/debugger/linux/ppc64/LinuxPPC64CFrame.java: 41 missed space around = 51 extra space between pc Done. * agent/src/share/classes/sun/jvm/hotspot/debugger/linux/ppc64/LinuxPPC64ThreadContext.java - no comments * agent/src/share/classes/sun/jvm/hotspot/debugger/ppc64/PPC64ThreadContext.java 47, 48 extra space before = Done. * agent/src/share/classes/sun/jvm/hotspot/debugger/proc/ppc64/ProcPPC64Thread.java 34 extra space before id 42 extra space before = , Done. it might be better to create a constant for size of integer. I agree, but this code was just copied from the corresponding AMD64 version and as far as I can see all the other architectures do it the same way so I didn't change it. * agent/src/share/classes/sun/jvm/hotspot/debugger/proc/ppc64/ProcPPC64ThreadContext.java - no comments * agent/src/share/classes/sun/jvm/hotspot/debugger/proc/ppc64/ProcPPC64ThreadFactory.java -no comments * agent/src/share/classes/sun/jvm/hotspot/debugger/remote/ppc64/RemotePPC64Thread.java 43 missed space before ? it might be be better to reformat this line to usual if and add comments The same here - this is copied code. But I've adjusted it to at least match with the other platforms. * agent/src/share/classes/sun/jvm/hotspot/debugger/remote/ppc64/RemotePPC64ThreadContext.java - no comments * agent/src/share/classes/sun/jvm/hotspot/debugger/remote/ppc64/RemotePPC64ThreadFactory.java - no comments * agent/src/share/classes/sun/jvm/hotspot/runtime/linux_ppc64/LinuxPPC64JavaThreadPDAccess.java 57, 58 extra space before = 63 and below extra space after public 108 unaligned comments *
Re: RFR 8066708: JMXStartStopTest fails to connect to port 38112
Stuart, 1. Ever if you set SO_LINGER to zero, socket will not be closed immediately. see TCP shutdown sequence. 2. In a native world it's quite easy to find the port your rmi server uses - it could be achieved by parsing /proc/pid/net/tcp on Linux or using special API on windows and solaris. 3. For rmid and port provided from outside I think the only reliable way to get what you need is: Write a driver that: 1. starts rmid -port some_random_value 2. connect to this port to make sure rmid is actually started (no rmi, just plain tcp connect) 3. if (2) fail return to (1) 4. run client with this port number. 4. For rmid you can also emulate inetd behaviour - i.e. driver open a server port, communicate it to client than redirect everything that come to this port to stdin of rmid. -Dmitry On 2014-12-17 01:55, Stuart Marks wrote: Hi Dmitry, Strictly speaking you are correct. As soon as you close a socket, there is a possibility -- perhaps vanishingly small but nonzero -- that you might not be able to open it again. The first scenario, where the user of the socket itself opens the socket using an ephemeral port (e.g. new ServerSocket(0)) is of course preferred. This avoids race conditions entirely. It's the second case that I'm still wrestling with, and maybe Jaroslav too. It's fairly difficult to get such black box systems to open an ephemeral port and report it back, as opposed to opening up their service on some port number handed in from the outside. (For RMI, rmid is the culprit here. I don't know about JMX.) What makes this difficult is that the rmid service is running in a separate VM, so getting reliable information back from it can be difficult. It's also fairly difficult to establish the retry logic in such cases. If the service fails with a BindException, maybe -- maybe -- it was because there was a conflict over the port, and a retry is warranted. But this needs to be distinguished from other failure modes that might occur, that should be reported as failures instead of causing a retry. In principle, this is possible to do, of course, it's just that it involves more restructuring of the tests, and possibly adding debug/test code to rmid. (It may yet come to that.) I'm still pondering the reasons that, in the open/close/reopen scenario, why the reopen might fail. The obvious reason is that some other process on the system has opened that port between the close and the reopen. I admit that this is a possibility. However, with the open/close/reopen scenario in place, we see tests that fail up to 15% of the time with BindExceptions. This is an extraordinarily high failure rate to be caused by some random other process happening to open the same port in the few microseconds between the close and reopen. It's simply not believable to me. My thinking is still that the port isn't ready for reuse until a small amount of time after it's closed. I have some test programs that exercise sockets in a particular way (e.g., from multiple threads, or opening and closing batches of sockets) that can reproduce the problem on some systems, and these test programs seem to behave better if a time delay is added between the close and the reopen. The exact circumstances under which the problem occurs is difficult to pin down and seems OS specific, and so choosing the right delay time is very difficult. But it does strengthen this conjecture in my mind. Naturally it would be better if there were a way to determine when a port is available for reuse without actually opening it. I'm not aware of any such way, but I'm holding onto a little hope that one can be found. s'marks On 12/11/14 10:18 AM, Dmitry Samersoff wrote: Stuart, As soon as you close socket, you open a door for the race. So you need another communication channel to pass a port number (or bind result) between a client and a server without closing a socket on the server side. Typical scenario used by network related code is: 1. Server opens the socket 2. Server binds to port(0) 3. Server gets port number assigned by OS 4. Server informs client (e.g. write the port down to known file, broadcast it etc) 5. Client establishes connection. If the server is a blackbox and have to get a port number from outside, scenario looks like: WHILE(!success and !timeout) 1. Driver chooses random port number 2. Driver runs a server with this number 3. Driver checks that server is actually listening on this port (e.g. try to connect by it self) WEND 4. Driver runs a client with this port number or bails out with descriptive error message. -Dmitry On 2014-12-11 20:53, Stuart Marks wrote: On 12/11/14 7:09 AM, olivier.lagn...@oracle.com wrote: On 11/12/2014 15:43, Dmitry Samersoff wrote: You can set SO_LINGER to zero, in this case socket will be closed immediately without waiting in TIME_WAIT SO-LINGER did not help either in my case (see my previous mail to
