Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again
Serguei,
http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.04/
I refactored the debugInit_exit function.
Now we have separate exit code for transport error and better readable
code.
-Dmitry
On 2014-08-27 00:22, serguei.spit...@oracle.com wrote:
Dmitry,
It makes sense to consider a special exit code for transport init error.
Other than that it looks good.
No need to re-review.
Thanks,
Serguei
On 8/26/14 10:46 AM, Dmitry Samersoff wrote:
Serguei,
exit_code set to 1 at ll. 1288
I thought of refactoring this code for better readability but finally
decide to keep changes minimal.
-Dmitry
On 2014-08-26 21:35, serguei.spit...@oracle.com wrote:
Dmitry,
This looks good in general.
But what error code will be returned in the case of
AGENT_ERROR_TRANSPORT_INIT ?
Is it Ok to return whatever exit_code we already have or we better
return some special error code?
Probably, it is Ok to keep the comment at the line 1315 as it is.
Thanks,
Serguei
On 8/26/14 7:47 AM, Dmitry Samersoff wrote:
Staffan,
http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.03/
After couple of experiments I end up with simple fix - don't
coredump on
AGENT_ERROR_TRANSPORT_INIT
Current code don't propagate transport error to upper level, so if we
need fine grained control I'll rewrite transport initialization.
-Dmitry
On 2014-08-25 15:53, Staffan Larsen wrote:
On 25 aug 2014, at 13:33, Dmitry Samersoff
dmitry.samers...@oracle.com
mailto:dmitry.samers...@oracle.com wrote:
Staffan,
On 2014-08-25 15:26, Staffan Larsen wrote:
Dmitry,
One problem with this fix is that debugInit_exit() is used from many
places in the JDWP code. For some of these I think it still does
make
sense to receive a core dump for analysis. I agree that when parsing
options, we do not need a core dump.
jdwp has explicit option to request a coredump at debugInit_exit().
Is it enough or I should create a more sophisticated fix ?
I don’t think that is enough. Often a problem will happen and will not
be reproducible.
Maybe this code:
if ((arg.error != JDWP_ERROR(NONE))
(arg.startCount == 0)
initOnStartup) {
EXIT_ERROR(map2jvmtiError(arg.error), No transports
initialized);
}
can use some variant of EXIT_ERROR that does not call fatalError() ?
/Staffan
Actually, I don't think that coredump on every call to
jni_FatalError()
(see jni.cpp:726) is necessary but this hotspot code is here for 6
years
and changing it probably out of scope of this fix.
-Dmitry
/Staffan
On 20 aug 2014, at 17:55, Dmitry Samersoff
dmitry.samers...@oracle.com
mailto:dmitry.samers...@oracle.com wrote:
Serguei,
After some additional testing I changed the fix a bit. Please take
a look at new version.
http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/
New version reports JVMTI error to stderr.
Also jniFatalError is not referenced any more so I remove it.
-Dmitry
On 2014-08-20 12:08, serguei.spit...@oracle.com wrote:
Ok.
Thank you for the explanation! Serguei
On 8/20/14 1:01 AM, Dmitry Samersoff wrote:
Serguei,
1. Historically JDI test-suite had no tests for failed
transport initialization behavior and invalid parameters
handling.
2. As a part of JDWP hardening work I added couple of such
tests to OptionTest.java - these tests pass invalid parameters
to dt_socket transport to make sure that transport doesn't
crash (one such crash was discovered and fixed) but just return
non-zero exit code to upper level.
3. After fix for JDK-6694099 any non-zero exit code from
transport cause VM to coredump. Dumping multiple cores on busy
machine takes a time so harness kills the test by timeout.
We can just increase timeout for this test but I don't think
it's a good idea to dump core when invalid parameters passed to
transport.
So there is the fix.
4. After the fix tests for negative parameters will return
non-zero exit code as expected but will not dump the core.
-Dmitry
On 2014-08-20 00:54, serguei.spit...@oracle.com wrote:
Hi Dmitry,
The fix seems to be Ok. Just want to make it clear... This
fix just changes the bug pattern. It a case of incorrect
transport parameters the test is still going to fail but
without crash, right?
Thanks, Serguei
On 8/19/14 12:09 PM, Dmitry Samersoff wrote:
Hi Everybody,
Please review the fix:
http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.01/
JDWP call jniFatalError if transport can't be initialized (e.g. wrong
parameters) and jniFatalError call os::abort(). Therefor
all transport initialization errors cause vm to coredump.
I see no reason for debugInit_exit to call jniFatalError so
remove this code.
-Dmitry
-- Dmitry Samersoff Oracle Java development team, Saint Petersburg,
Russia * I would love to change the world, but they won't give me
the sources.
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would
Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again
This looks good.
Could you, please, remove extra spaces in the following conditions ?:
1291 if (error != JVMTI_ERROR_NONE docoredump ) {
1302 if ( gdata-jvmti != NULL ) {
1309 if ( error == JVMTI_ERROR_NONE ) {
Thanks,
Serguei
On 8/27/14 12:36 AM, Dmitry Samersoff wrote:
Serguei,
http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.04/
I refactored the debugInit_exit function.
Now we have separate exit code for transport error and better readable
code.
-Dmitry
On 2014-08-27 00:22, serguei.spit...@oracle.com wrote:
Dmitry,
It makes sense to consider a special exit code for transport init error.
Other than that it looks good.
No need to re-review.
Thanks,
Serguei
On 8/26/14 10:46 AM, Dmitry Samersoff wrote:
Serguei,
exit_code set to 1 at ll. 1288
I thought of refactoring this code for better readability but finally
decide to keep changes minimal.
-Dmitry
On 2014-08-26 21:35, serguei.spit...@oracle.com wrote:
Dmitry,
This looks good in general.
But what error code will be returned in the case of
AGENT_ERROR_TRANSPORT_INIT ?
Is it Ok to return whatever exit_code we already have or we better
return some special error code?
Probably, it is Ok to keep the comment at the line 1315 as it is.
Thanks,
Serguei
On 8/26/14 7:47 AM, Dmitry Samersoff wrote:
Staffan,
http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.03/
After couple of experiments I end up with simple fix - don't
coredump on
AGENT_ERROR_TRANSPORT_INIT
Current code don't propagate transport error to upper level, so if we
need fine grained control I'll rewrite transport initialization.
-Dmitry
On 2014-08-25 15:53, Staffan Larsen wrote:
On 25 aug 2014, at 13:33, Dmitry Samersoff
dmitry.samers...@oracle.com
mailto:dmitry.samers...@oracle.com wrote:
Staffan,
On 2014-08-25 15:26, Staffan Larsen wrote:
Dmitry,
One problem with this fix is that debugInit_exit() is used from many
places in the JDWP code. For some of these I think it still does
make
sense to receive a core dump for analysis. I agree that when parsing
options, we do not need a core dump.
jdwp has explicit option to request a coredump at debugInit_exit().
Is it enough or I should create a more sophisticated fix ?
I don’t think that is enough. Often a problem will happen and will not
be reproducible.
Maybe this code:
if ((arg.error != JDWP_ERROR(NONE))
(arg.startCount == 0)
initOnStartup) {
EXIT_ERROR(map2jvmtiError(arg.error), No transports
initialized);
}
can use some variant of EXIT_ERROR that does not call fatalError() ?
/Staffan
Actually, I don't think that coredump on every call to
jni_FatalError()
(see jni.cpp:726) is necessary but this hotspot code is here for 6
years
and changing it probably out of scope of this fix.
-Dmitry
/Staffan
On 20 aug 2014, at 17:55, Dmitry Samersoff
dmitry.samers...@oracle.com
mailto:dmitry.samers...@oracle.com wrote:
Serguei,
After some additional testing I changed the fix a bit. Please take
a look at new version.
http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/
New version reports JVMTI error to stderr.
Also jniFatalError is not referenced any more so I remove it.
-Dmitry
On 2014-08-20 12:08, serguei.spit...@oracle.com wrote:
Ok.
Thank you for the explanation! Serguei
On 8/20/14 1:01 AM, Dmitry Samersoff wrote:
Serguei,
1. Historically JDI test-suite had no tests for failed
transport initialization behavior and invalid parameters
handling.
2. As a part of JDWP hardening work I added couple of such
tests to OptionTest.java - these tests pass invalid parameters
to dt_socket transport to make sure that transport doesn't
crash (one such crash was discovered and fixed) but just return
non-zero exit code to upper level.
3. After fix for JDK-6694099 any non-zero exit code from
transport cause VM to coredump. Dumping multiple cores on busy
machine takes a time so harness kills the test by timeout.
We can just increase timeout for this test but I don't think
it's a good idea to dump core when invalid parameters passed to
transport.
So there is the fix.
4. After the fix tests for negative parameters will return
non-zero exit code as expected but will not dump the core.
-Dmitry
On 2014-08-20 00:54, serguei.spit...@oracle.com wrote:
Hi Dmitry,
The fix seems to be Ok. Just want to make it clear... This
fix just changes the bug pattern. It a case of incorrect
transport parameters the test is still going to fail but
without crash, right?
Thanks, Serguei
On 8/19/14 12:09 PM, Dmitry Samersoff wrote:
Hi Everybody,
Please review the fix:
http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.01/
JDWP call jniFatalError if transport can't be initialized (e.g. wrong
parameters) and jniFatalError call os::abort(). Therefor
all transport initialization errors cause vm to coredump.
I see no reason for debugInit_exit to call jniFatalError so
remove this code.
-Dmitry
-- Dmitry Samersoff Oracle Java
Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again
1284 enum exit_codes { EXIT_NO_ERRORS = 0, EXIT_TRANSPORT_ERROR,
EXIT_JVMTI_ERROR };
I'd suggest to swap the positions of EXIT_TRANSPORT_ERROR and
EXIT_JVMTI_ERROR
in the enum to keep this as compatible to previous behavior as possible.
Thanks,
Serguei
On 8/27/14 1:08 AM, serguei.spit...@oracle.com wrote:
This looks good.
Could you, please, remove extra spaces in the following conditions ?:
1291 if (error != JVMTI_ERROR_NONE docoredump ) {
1302 if ( gdata-jvmti != NULL ) {
1309 if ( error == JVMTI_ERROR_NONE ) {
Thanks,
Serguei
On 8/27/14 12:36 AM, Dmitry Samersoff wrote:
Serguei,
http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.04/
I refactored the debugInit_exit function.
Now we have separate exit code for transport error and better readable
code.
-Dmitry
On 2014-08-27 00:22,serguei.spit...@oracle.com wrote:
Dmitry,
It makes sense to consider a special exit code for transport init error.
Other than that it looks good.
No need to re-review.
Thanks,
Serguei
On 8/26/14 10:46 AM, Dmitry Samersoff wrote:
Serguei,
exit_code set to 1 at ll. 1288
I thought of refactoring this code for better readability but finally
decide to keep changes minimal.
-Dmitry
On 2014-08-26 21:35,serguei.spit...@oracle.com wrote:
Dmitry,
This looks good in general.
But what error code will be returned in the case of
AGENT_ERROR_TRANSPORT_INIT ?
Is it Ok to return whatever exit_code we already have or we better
return some special error code?
Probably, it is Ok to keep the comment at the line 1315 as it is.
Thanks,
Serguei
On 8/26/14 7:47 AM, Dmitry Samersoff wrote:
Staffan,
http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.03/
After couple of experiments I end up with simple fix - don't
coredump on
AGENT_ERROR_TRANSPORT_INIT
Current code don't propagate transport error to upper level, so if we
need fine grained control I'll rewrite transport initialization.
-Dmitry
On 2014-08-25 15:53, Staffan Larsen wrote:
On 25 aug 2014, at 13:33, Dmitry Samersoff
dmitry.samers...@oracle.com
mailto:dmitry.samers...@oracle.com wrote:
Staffan,
On 2014-08-25 15:26, Staffan Larsen wrote:
Dmitry,
One problem with this fix is that debugInit_exit() is used from many
places in the JDWP code. For some of these I think it still does
make
sense to receive a core dump for analysis. I agree that when parsing
options, we do not need a core dump.
jdwp has explicit option to request a coredump at debugInit_exit().
Is it enough or I should create a more sophisticated fix ?
I don’t think that is enough. Often a problem will happen and will not
be reproducible.
Maybe this code:
if ((arg.error != JDWP_ERROR(NONE))
(arg.startCount == 0)
initOnStartup) {
EXIT_ERROR(map2jvmtiError(arg.error), No transports
initialized);
}
can use some variant of EXIT_ERROR that does not call fatalError() ?
/Staffan
Actually, I don't think that coredump on every call to
jni_FatalError()
(see jni.cpp:726) is necessary but this hotspot code is here for 6
years
and changing it probably out of scope of this fix.
-Dmitry
/Staffan
On 20 aug 2014, at 17:55, Dmitry Samersoff
dmitry.samers...@oracle.com
mailto:dmitry.samers...@oracle.com wrote:
Serguei,
After some additional testing I changed the fix a bit. Please take
a look at new version.
http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/
New version reports JVMTI error to stderr.
Also jniFatalError is not referenced any more so I remove it.
-Dmitry
On 2014-08-20 12:08,serguei.spit...@oracle.com wrote:
Ok.
Thank you for the explanation! Serguei
On 8/20/14 1:01 AM, Dmitry Samersoff wrote:
Serguei,
1. Historically JDI test-suite had no tests for failed
transport initialization behavior and invalid parameters
handling.
2. As a part of JDWP hardening work I added couple of such
tests to OptionTest.java - these tests pass invalid parameters
to dt_socket transport to make sure that transport doesn't
crash (one such crash was discovered and fixed) but just return
non-zero exit code to upper level.
3. After fix for JDK-6694099 any non-zero exit code from
transport cause VM to coredump. Dumping multiple cores on busy
machine takes a time so harness kills the test by timeout.
We can just increase timeout for this test but I don't think
it's a good idea to dump core when invalid parameters passed to
transport.
So there is the fix.
4. After the fix tests for negative parameters will return
non-zero exit code as expected but will not dump the core.
-Dmitry
On 2014-08-20 00:54,serguei.spit...@oracle.com wrote:
Hi Dmitry,
The fix seems to be Ok. Just want to make it clear... This
fix just changes the bug pattern. It a case of incorrect
transport parameters the test is still going to fail but
without crash, right?
Thanks, Serguei
On 8/19/14 12:09 PM, Dmitry Samersoff wrote:
Hi Everybody,
Please review the fix:
http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.01/
RFR: 8056143: java/lang/management/MemoryMXBean/LowMemoryTest.java leaves running process on Windows
Hi all, Please review this patch to put the LowMemoryTest.java test in the ProblemLists.txt. It currently hangs and leaves processes running after the test run has completed. http://cr.openjdk.java.net/~stefank/8056143/webrev.00/ thanks, StefanK
Re: RFR: 8056143: java/lang/management/MemoryMXBean/LowMemoryTest.java leaves running process on Windows
Reviewed. -Chris. On 27/08/14 09:48, Stefan Karlsson wrote: Hi all, Please review this patch to put the LowMemoryTest.java test in the ProblemLists.txt. It currently hangs and leaves processes running after the test run has completed. http://cr.openjdk.java.net/~stefank/8056143/webrev.00/ thanks, StefanK
Re: RFR: 8056143: java/lang/management/MemoryMXBean/LowMemoryTest.java leaves running process on Windows
Thanks. StefanK On 27/08/14 10:48, Chris Hegarty wrote: Reviewed. -Chris. On 27/08/14 09:48, Stefan Karlsson wrote: Hi all, Please review this patch to put the LowMemoryTest.java test in the ProblemLists.txt. It currently hangs and leaves processes running after the test run has completed. http://cr.openjdk.java.net/~stefank/8056143/webrev.00/ thanks, StefanK
RFR(XS) : 8055755: Information about loaded dynamic libraries is wrong on MacOSX
Please help me review the following small patch: Review - http://cr.openjdk.java.net/~farvidsson/8055755/ Bug - https://bugs.openjdk.java.net/browse/JDK-8055755 There is information about the problem and the solution added to the bug as a comment. Cheers /Fredrik
Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again
Serguei,
Fixed (in-place, press shift reload)
-Dmitry
On 2014-08-27 12:12, serguei.spit...@oracle.com wrote:
1284 enum exit_codes { EXIT_NO_ERRORS = 0, EXIT_TRANSPORT_ERROR,
EXIT_JVMTI_ERROR };
I'd suggest to swap the positions of EXIT_TRANSPORT_ERROR and
EXIT_JVMTI_ERROR
in the enum to keep this as compatible to previous behavior as possible.
Thanks,
Serguei
On 8/27/14 1:08 AM, serguei.spit...@oracle.com wrote:
This looks good.
Could you, please, remove extra spaces in the following conditions ?:
1291 if (error != JVMTI_ERROR_NONE docoredump ) {
1302 if ( gdata-jvmti != NULL ) {
1309 if ( error == JVMTI_ERROR_NONE ) {
Thanks,
Serguei
On 8/27/14 12:36 AM, Dmitry Samersoff wrote:
Serguei,
http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.04/
I refactored the debugInit_exit function.
Now we have separate exit code for transport error and better readable
code.
-Dmitry
On 2014-08-27 00:22, serguei.spit...@oracle.com wrote:
Dmitry,
It makes sense to consider a special exit code for transport init error.
Other than that it looks good.
No need to re-review.
Thanks,
Serguei
On 8/26/14 10:46 AM, Dmitry Samersoff wrote:
Serguei,
exit_code set to 1 at ll. 1288
I thought of refactoring this code for better readability but finally
decide to keep changes minimal.
-Dmitry
On 2014-08-26 21:35, serguei.spit...@oracle.com wrote:
Dmitry,
This looks good in general.
But what error code will be returned in the case of
AGENT_ERROR_TRANSPORT_INIT ?
Is it Ok to return whatever exit_code we already have or we better
return some special error code?
Probably, it is Ok to keep the comment at the line 1315 as it is.
Thanks,
Serguei
On 8/26/14 7:47 AM, Dmitry Samersoff wrote:
Staffan,
http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.03/
After couple of experiments I end up with simple fix - don't
coredump on
AGENT_ERROR_TRANSPORT_INIT
Current code don't propagate transport error to upper level, so if we
need fine grained control I'll rewrite transport initialization.
-Dmitry
On 2014-08-25 15:53, Staffan Larsen wrote:
On 25 aug 2014, at 13:33, Dmitry Samersoff
dmitry.samers...@oracle.com
mailto:dmitry.samers...@oracle.com wrote:
Staffan,
On 2014-08-25 15:26, Staffan Larsen wrote:
Dmitry,
One problem with this fix is that debugInit_exit() is used from many
places in the JDWP code. For some of these I think it still does
make
sense to receive a core dump for analysis. I agree that when parsing
options, we do not need a core dump.
jdwp has explicit option to request a coredump at debugInit_exit().
Is it enough or I should create a more sophisticated fix ?
I don’t think that is enough. Often a problem will happen and will not
be reproducible.
Maybe this code:
if ((arg.error != JDWP_ERROR(NONE))
(arg.startCount == 0)
initOnStartup) {
EXIT_ERROR(map2jvmtiError(arg.error), No transports
initialized);
}
can use some variant of EXIT_ERROR that does not call fatalError() ?
/Staffan
Actually, I don't think that coredump on every call to
jni_FatalError()
(see jni.cpp:726) is necessary but this hotspot code is here for 6
years
and changing it probably out of scope of this fix.
-Dmitry
/Staffan
On 20 aug 2014, at 17:55, Dmitry Samersoff
dmitry.samers...@oracle.com
mailto:dmitry.samers...@oracle.com wrote:
Serguei,
After some additional testing I changed the fix a bit. Please take
a look at new version.
http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/
New version reports JVMTI error to stderr.
Also jniFatalError is not referenced any more so I remove it.
-Dmitry
On 2014-08-20 12:08, serguei.spit...@oracle.com wrote:
Ok.
Thank you for the explanation! Serguei
On 8/20/14 1:01 AM, Dmitry Samersoff wrote:
Serguei,
1. Historically JDI test-suite had no tests for failed
transport initialization behavior and invalid parameters
handling.
2. As a part of JDWP hardening work I added couple of such
tests to OptionTest.java - these tests pass invalid parameters
to dt_socket transport to make sure that transport doesn't
crash (one such crash was discovered and fixed) but just return
non-zero exit code to upper level.
3. After fix for JDK-6694099 any non-zero exit code from
transport cause VM to coredump. Dumping multiple cores on busy
machine takes a time so harness kills the test by timeout.
We can just increase timeout for this test but I don't think
it's a good idea to dump core when invalid parameters passed to
transport.
So there is the fix.
4. After the fix tests for negative parameters will return
non-zero exit code as expected but will not dump the core.
-Dmitry
On 2014-08-20 00:54, serguei.spit...@oracle.com wrote:
Hi Dmitry,
The fix seems to be Ok. Just want to make it clear... This
fix just changes the bug pattern. It a case of incorrect
transport parameters the
Re: RFR: 8056143: java/lang/management/MemoryMXBean/LowMemoryTest.java leaves running process on Windows
I revoke this RFR. Bengt pointed out that I shouldn't use 8056143 to updated the problem list. I've created a new bug JDK-8056148 and will send out a new RFR. thanks, StefanK On 27/08/14 10:48, Stefan Karlsson wrote: Hi all, Please review this patch to put the LowMemoryTest.java test in the ProblemLists.txt. It currently hangs and leaves processes running after the test run has completed. http://cr.openjdk.java.net/~stefank/8056143/webrev.00/ thanks, StefanK
8056148: Add java/lang/management/MemoryMXBean/LowMemoryTest.java to ProblemList.txt
Hi all, Please review this patch to put the LowMemoryTest.java test in the ProblemLists.txt. It currently hangs and leaves processes running after the test run has completed. http://cr.openjdk.java.net/~stefank/8056148/webrev.00/ thanks, StefanK
Re: 8056148: Add java/lang/management/MemoryMXBean/LowMemoryTest.java to ProblemList.txt
Looks ok to me. -Chris. On 27/08/14 10:11, Stefan Karlsson wrote: Hi all, Please review this patch to put the LowMemoryTest.java test in the ProblemLists.txt. It currently hangs and leaves processes running after the test run has completed. http://cr.openjdk.java.net/~stefank/8056148/webrev.00/ thanks, StefanK
Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again
One more minor comment.
The indent in this file must be 4.
Could you fix it?
Thanks,
Serguei
On 8/27/14 2:03 AM, Dmitry Samersoff wrote:
Serguei,
Fixed (in-place, press shift reload)
-Dmitry
On 2014-08-27 12:12, serguei.spit...@oracle.com wrote:
1284 enum exit_codes { EXIT_NO_ERRORS = 0, EXIT_TRANSPORT_ERROR,
EXIT_JVMTI_ERROR };
I'd suggest to swap the positions of EXIT_TRANSPORT_ERROR and
EXIT_JVMTI_ERROR
in the enum to keep this as compatible to previous behavior as possible.
Thanks,
Serguei
On 8/27/14 1:08 AM, serguei.spit...@oracle.com wrote:
This looks good.
Could you, please, remove extra spaces in the following conditions ?:
1291 if (error != JVMTI_ERROR_NONE docoredump ) {
1302 if ( gdata-jvmti != NULL ) {
1309 if ( error == JVMTI_ERROR_NONE ) {
Thanks,
Serguei
On 8/27/14 12:36 AM, Dmitry Samersoff wrote:
Serguei,
http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.04/
I refactored the debugInit_exit function.
Now we have separate exit code for transport error and better readable
code.
-Dmitry
On 2014-08-27 00:22, serguei.spit...@oracle.com wrote:
Dmitry,
It makes sense to consider a special exit code for transport init error.
Other than that it looks good.
No need to re-review.
Thanks,
Serguei
On 8/26/14 10:46 AM, Dmitry Samersoff wrote:
Serguei,
exit_code set to 1 at ll. 1288
I thought of refactoring this code for better readability but finally
decide to keep changes minimal.
-Dmitry
On 2014-08-26 21:35, serguei.spit...@oracle.com wrote:
Dmitry,
This looks good in general.
But what error code will be returned in the case of
AGENT_ERROR_TRANSPORT_INIT ?
Is it Ok to return whatever exit_code we already have or we better
return some special error code?
Probably, it is Ok to keep the comment at the line 1315 as it is.
Thanks,
Serguei
On 8/26/14 7:47 AM, Dmitry Samersoff wrote:
Staffan,
http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.03/
After couple of experiments I end up with simple fix - don't
coredump on
AGENT_ERROR_TRANSPORT_INIT
Current code don't propagate transport error to upper level, so if we
need fine grained control I'll rewrite transport initialization.
-Dmitry
On 2014-08-25 15:53, Staffan Larsen wrote:
On 25 aug 2014, at 13:33, Dmitry Samersoff
dmitry.samers...@oracle.com
mailto:dmitry.samers...@oracle.com wrote:
Staffan,
On 2014-08-25 15:26, Staffan Larsen wrote:
Dmitry,
One problem with this fix is that debugInit_exit() is used from many
places in the JDWP code. For some of these I think it still does
make
sense to receive a core dump for analysis. I agree that when parsing
options, we do not need a core dump.
jdwp has explicit option to request a coredump at debugInit_exit().
Is it enough or I should create a more sophisticated fix ?
I don’t think that is enough. Often a problem will happen and will not
be reproducible.
Maybe this code:
if ((arg.error != JDWP_ERROR(NONE))
(arg.startCount == 0)
initOnStartup) {
EXIT_ERROR(map2jvmtiError(arg.error), No transports
initialized);
}
can use some variant of EXIT_ERROR that does not call fatalError() ?
/Staffan
Actually, I don't think that coredump on every call to
jni_FatalError()
(see jni.cpp:726) is necessary but this hotspot code is here for 6
years
and changing it probably out of scope of this fix.
-Dmitry
/Staffan
On 20 aug 2014, at 17:55, Dmitry Samersoff
dmitry.samers...@oracle.com
mailto:dmitry.samers...@oracle.com wrote:
Serguei,
After some additional testing I changed the fix a bit. Please take
a look at new version.
http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/
New version reports JVMTI error to stderr.
Also jniFatalError is not referenced any more so I remove it.
-Dmitry
On 2014-08-20 12:08, serguei.spit...@oracle.com wrote:
Ok.
Thank you for the explanation! Serguei
On 8/20/14 1:01 AM, Dmitry Samersoff wrote:
Serguei,
1. Historically JDI test-suite had no tests for failed
transport initialization behavior and invalid parameters
handling.
2. As a part of JDWP hardening work I added couple of such
tests to OptionTest.java - these tests pass invalid parameters
to dt_socket transport to make sure that transport doesn't
crash (one such crash was discovered and fixed) but just return
non-zero exit code to upper level.
3. After fix for JDK-6694099 any non-zero exit code from
transport cause VM to coredump. Dumping multiple cores on busy
machine takes a time so harness kills the test by timeout.
We can just increase timeout for this test but I don't think
it's a good idea to dump core when invalid parameters passed to
transport.
So there is the fix.
4. After the fix tests for negative parameters will return
non-zero exit code as expected but will not dump the core.
-Dmitry
On 2014-08-20 00:54, serguei.spit...@oracle.com wrote:
Hi Dmitry,
The fix seems to be Ok. Just want to make it clear... This
fix just changes the bug pattern. It a case of
Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again
Serguei,
Fixed (in-place, press shift reload)
-Dmitry
On 2014-08-27 14:25, serguei.spit...@oracle.com wrote:
One more minor comment.
The indent in this file must be 4.
Could you fix it?
Thanks,
Serguei
On 8/27/14 2:03 AM, Dmitry Samersoff wrote:
Serguei,
Fixed (in-place, press shift reload)
-Dmitry
On 2014-08-27 12:12, serguei.spit...@oracle.com wrote:
1284 enum exit_codes { EXIT_NO_ERRORS = 0, EXIT_TRANSPORT_ERROR,
EXIT_JVMTI_ERROR };
I'd suggest to swap the positions of EXIT_TRANSPORT_ERROR and
EXIT_JVMTI_ERROR
in the enum to keep this as compatible to previous behavior as possible.
Thanks,
Serguei
On 8/27/14 1:08 AM, serguei.spit...@oracle.com wrote:
This looks good.
Could you, please, remove extra spaces in the following conditions ?:
1291 if (error != JVMTI_ERROR_NONE docoredump ) {
1302 if ( gdata-jvmti != NULL ) {
1309 if ( error == JVMTI_ERROR_NONE ) {
Thanks,
Serguei
On 8/27/14 12:36 AM, Dmitry Samersoff wrote:
Serguei,
http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.04/
I refactored the debugInit_exit function.
Now we have separate exit code for transport error and better readable
code.
-Dmitry
On 2014-08-27 00:22, serguei.spit...@oracle.com wrote:
Dmitry,
It makes sense to consider a special exit code for transport init
error.
Other than that it looks good.
No need to re-review.
Thanks,
Serguei
On 8/26/14 10:46 AM, Dmitry Samersoff wrote:
Serguei,
exit_code set to 1 at ll. 1288
I thought of refactoring this code for better readability but
finally
decide to keep changes minimal.
-Dmitry
On 2014-08-26 21:35, serguei.spit...@oracle.com wrote:
Dmitry,
This looks good in general.
But what error code will be returned in the case of
AGENT_ERROR_TRANSPORT_INIT ?
Is it Ok to return whatever exit_code we already have or we better
return some special error code?
Probably, it is Ok to keep the comment at the line 1315 as it is.
Thanks,
Serguei
On 8/26/14 7:47 AM, Dmitry Samersoff wrote:
Staffan,
http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.03/
After couple of experiments I end up with simple fix - don't
coredump on
AGENT_ERROR_TRANSPORT_INIT
Current code don't propagate transport error to upper level, so
if we
need fine grained control I'll rewrite transport initialization.
-Dmitry
On 2014-08-25 15:53, Staffan Larsen wrote:
On 25 aug 2014, at 13:33, Dmitry Samersoff
dmitry.samers...@oracle.com
mailto:dmitry.samers...@oracle.com wrote:
Staffan,
On 2014-08-25 15:26, Staffan Larsen wrote:
Dmitry,
One problem with this fix is that debugInit_exit() is used
from many
places in the JDWP code. For some of these I think it still
does
make
sense to receive a core dump for analysis. I agree that when
parsing
options, we do not need a core dump.
jdwp has explicit option to request a coredump at
debugInit_exit().
Is it enough or I should create a more sophisticated fix ?
I don’t think that is enough. Often a problem will happen and
will not
be reproducible.
Maybe this code:
if ((arg.error != JDWP_ERROR(NONE))
(arg.startCount == 0)
initOnStartup) {
EXIT_ERROR(map2jvmtiError(arg.error), No transports
initialized);
}
can use some variant of EXIT_ERROR that does not call
fatalError() ?
/Staffan
Actually, I don't think that coredump on every call to
jni_FatalError()
(see jni.cpp:726) is necessary but this hotspot code is here
for 6
years
and changing it probably out of scope of this fix.
-Dmitry
/Staffan
On 20 aug 2014, at 17:55, Dmitry Samersoff
dmitry.samers...@oracle.com
mailto:dmitry.samers...@oracle.com wrote:
Serguei,
After some additional testing I changed the fix a bit.
Please take
a look at new version.
http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/
New version reports JVMTI error to stderr.
Also jniFatalError is not referenced any more so I remove it.
-Dmitry
On 2014-08-20 12:08, serguei.spit...@oracle.com wrote:
Ok.
Thank you for the explanation! Serguei
On 8/20/14 1:01 AM, Dmitry Samersoff wrote:
Serguei,
1. Historically JDI test-suite had no tests for failed
transport initialization behavior and invalid parameters
handling.
2. As a part of JDWP hardening work I added couple of such
tests to OptionTest.java - these tests pass invalid
parameters
to dt_socket transport to make sure that transport doesn't
crash (one such crash was discovered and fixed) but just
return
non-zero exit code to upper level.
3. After fix for JDK-6694099 any non-zero exit code from
transport cause VM to coredump. Dumping multiple cores on
busy
machine takes a time so harness kills the test by timeout.
We can just increase timeout for this test but I don't think
it's a good idea to dump core when invalid parameters
passed to
transport.
So there is the fix.
4. After the fix tests for negative parameters will return
Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again
Thanks!
Serguei
On 8/27/14 3:51 AM, Dmitry Samersoff wrote:
Serguei,
Fixed (in-place, press shift reload)
-Dmitry
On 2014-08-27 14:25, serguei.spit...@oracle.com wrote:
One more minor comment.
The indent in this file must be 4.
Could you fix it?
Thanks,
Serguei
On 8/27/14 2:03 AM, Dmitry Samersoff wrote:
Serguei,
Fixed (in-place, press shift reload)
-Dmitry
On 2014-08-27 12:12, serguei.spit...@oracle.com wrote:
1284 enum exit_codes { EXIT_NO_ERRORS = 0, EXIT_TRANSPORT_ERROR,
EXIT_JVMTI_ERROR };
I'd suggest to swap the positions of EXIT_TRANSPORT_ERROR and
EXIT_JVMTI_ERROR
in the enum to keep this as compatible to previous behavior as possible.
Thanks,
Serguei
On 8/27/14 1:08 AM, serguei.spit...@oracle.com wrote:
This looks good.
Could you, please, remove extra spaces in the following conditions ?:
1291 if (error != JVMTI_ERROR_NONE docoredump ) {
1302 if ( gdata-jvmti != NULL ) {
1309 if ( error == JVMTI_ERROR_NONE ) {
Thanks,
Serguei
On 8/27/14 12:36 AM, Dmitry Samersoff wrote:
Serguei,
http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.04/
I refactored the debugInit_exit function.
Now we have separate exit code for transport error and better readable
code.
-Dmitry
On 2014-08-27 00:22, serguei.spit...@oracle.com wrote:
Dmitry,
It makes sense to consider a special exit code for transport init
error.
Other than that it looks good.
No need to re-review.
Thanks,
Serguei
On 8/26/14 10:46 AM, Dmitry Samersoff wrote:
Serguei,
exit_code set to 1 at ll. 1288
I thought of refactoring this code for better readability but
finally
decide to keep changes minimal.
-Dmitry
On 2014-08-26 21:35, serguei.spit...@oracle.com wrote:
Dmitry,
This looks good in general.
But what error code will be returned in the case of
AGENT_ERROR_TRANSPORT_INIT ?
Is it Ok to return whatever exit_code we already have or we better
return some special error code?
Probably, it is Ok to keep the comment at the line 1315 as it is.
Thanks,
Serguei
On 8/26/14 7:47 AM, Dmitry Samersoff wrote:
Staffan,
http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.03/
After couple of experiments I end up with simple fix - don't
coredump on
AGENT_ERROR_TRANSPORT_INIT
Current code don't propagate transport error to upper level, so
if we
need fine grained control I'll rewrite transport initialization.
-Dmitry
On 2014-08-25 15:53, Staffan Larsen wrote:
On 25 aug 2014, at 13:33, Dmitry Samersoff
dmitry.samers...@oracle.com
mailto:dmitry.samers...@oracle.com wrote:
Staffan,
On 2014-08-25 15:26, Staffan Larsen wrote:
Dmitry,
One problem with this fix is that debugInit_exit() is used
from many
places in the JDWP code. For some of these I think it still
does
make
sense to receive a core dump for analysis. I agree that when
parsing
options, we do not need a core dump.
jdwp has explicit option to request a coredump at
debugInit_exit().
Is it enough or I should create a more sophisticated fix ?
I don’t think that is enough. Often a problem will happen and
will not
be reproducible.
Maybe this code:
if ((arg.error != JDWP_ERROR(NONE))
(arg.startCount == 0)
initOnStartup) {
EXIT_ERROR(map2jvmtiError(arg.error), No transports
initialized);
}
can use some variant of EXIT_ERROR that does not call
fatalError() ?
/Staffan
Actually, I don't think that coredump on every call to
jni_FatalError()
(see jni.cpp:726) is necessary but this hotspot code is here
for 6
years
and changing it probably out of scope of this fix.
-Dmitry
/Staffan
On 20 aug 2014, at 17:55, Dmitry Samersoff
dmitry.samers...@oracle.com
mailto:dmitry.samers...@oracle.com wrote:
Serguei,
After some additional testing I changed the fix a bit.
Please take
a look at new version.
http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/
New version reports JVMTI error to stderr.
Also jniFatalError is not referenced any more so I remove it.
-Dmitry
On 2014-08-20 12:08, serguei.spit...@oracle.com wrote:
Ok.
Thank you for the explanation! Serguei
On 8/20/14 1:01 AM, Dmitry Samersoff wrote:
Serguei,
1. Historically JDI test-suite had no tests for failed
transport initialization behavior and invalid parameters
handling.
2. As a part of JDWP hardening work I added couple of such
tests to OptionTest.java - these tests pass invalid
parameters
to dt_socket transport to make sure that transport doesn't
crash (one such crash was discovered and fixed) but just
return
non-zero exit code to upper level.
3. After fix for JDK-6694099 any non-zero exit code from
transport cause VM to coredump. Dumping multiple cores on
busy
machine takes a time so harness kills the test by timeout.
We can just increase timeout for this test but I don't think
it's a good idea to dump core when invalid parameters
passed to
transport.
So there is the fix.
4. After the fix tests for negative parameters will return
non-zero exit code as expected but will not dump the core.
Re: RFR 8055008: Clean up code that saves the previous versions of redefined classes
Dan,
Sorry for the big delay in reply...
It is because I did not have my arguments ready. :(
Please, see below.
On 8/20/14 3:37 PM, Daniel D. Daugherty wrote:
On 8/20/14 9:54 AM, Coleen Phillimore wrote:
Hi, it appears that my code is wrong and maybe the existing code is
wrong also. I have a spec question below.
Rely embedded below...
On 8/19/14, 7:52 PM, serguei.spit...@oracle.com wrote:
On 8/19/14 3:53 PM, Daniel D. Daugherty wrote:
On 8/19/14 3:39 PM, Daniel D. Daugherty wrote:
On 8/15/14 2:18 PM, Coleen Phillimore wrote:
Summary: Use scratch_class to find EMCP methods for breakpoints
if the old methods are still running
See bug for more details. This fix also includes a change to
nmethod::metadata_do() to not walk the _method multiple times
(the _method is added to the metadata section of the nmethod),
and an attempt to help the sweeper clean up these scratch_class
instance classes sooner.
Tested with nsk tests, java/lang/instrument tests and jck tests
(which include some jvmti tests).
open webrev at http://cr.openjdk.java.net/~coleenp/8055008/
src/share/vm/oops/instanceKlass.hpp
line 1047 // RedefineClass support
Should be 'RedefineClasses'.
line 1049: void mark_newly_obsolete_methods(ArrayMethod**
old_methods,
int emcp_method_count);
The 'obsolete' part of the function name does not match with
the 'emcp' part of the parameter name. EMCP methods are
'old',
but not 'obsolete'.
Update: OK, I think I get it. We want to mark methods that
are
newly transitioning from EMCP - obsolete and the
emcp_method_count
parameter tells us if there is any work to do.
src/share/vm/oops/instanceKlass.cpp
line 3023: } // pvw is cleaned up
'pvw' no longer exists so comment is stale.
line 3455: // check the previous versions array
This comment should move above this line:
3453 for (; pv_node != NULL; ) {
and 'array' should change to 'list'.
Sorry for the bad placement of the original comment.
line 3463: last-link_previous_versions(pv_node);
So now we've overwritten the previous value of
last-previous_versions. How does that InstanceKlass
get freed? Maybe a short comment?
line 3484: // Mark the emcp method as obsolete if it's not
executing
I'm not sure about whether this is a good idea. When we had a
redefine make a method obsolete before, we knew that we could
make all older but previously EMCP methods obsolete because
the new method version did make them obsolete.
With this optimization, we're saying that no thread is
executing
the method so we're going to make it obsolete even if the
current
redefine did not do so. I worry about a method call that
is in
the early stages of assembling the call stack being caught
calling a method that is now obsolete but not because of a
redefine made it obsolete. Just FYI, one of the tracing flags
catches unexpected calls to obsolete methods today and it
does
catch the current system's legitimate race.
JVM/TI has an isMethodObsolete() API:
jvmtiError
IsMethodObsolete(jvmtiEnv* env,
jmethodID method,
jboolean* is_obsolete_ptr)
It would be possible for an agent to observe a method changing from
not obsolete to obsolete when that's not expected. I suspect that
this would be a spec violation.
I agree that this looks like a spec violation.
The emcp methods by definition are non-obsolete,
or in opposite, the obsolete methods are non-emcp:
http://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#obsoleteMethods
Old method versions may become obsolete, not emcp:
http://docs.oracle.com/javase/7/docs/platform/jvmti/jvmti.html#RedefineClasses
But maybe I'm missing something...
If an EMCP method is not running, should we save it on a previous
version list anyway so that we can make it obsolete if it's redefined
and made obsolete?
Interesting question. The problem with an EMCP method is that it might
not be running right now, but we could have a Java thread that's in
the process of invoking it that is stopped on a safepoint. We resume
the world and the Java thread finishes calling the EMCP method...
It's really hard to catch in-progress uses of jmethodIDs and make
sure that the in-progress use switches from the EMCP method to the
latest version of the method. A rarely seen race, but it does happen...
It has to be invariant that if an EMCP is not running at redefinition time
then it has to be gone and can not become running in the future.
Otherwise, everything becomes overcomplicated.
This also means that non-running EMCP method can never be made obsolete.
The JVMTI spec is clear about the jmethodIDs and obsolete methods:
Obsolete Methods
The functions |RetransformClasses|
Re: RFR 8055008: Clean up code that saves the previous versions of redefined classes
On 8/20/14 3:45 PM, Daniel D. Daugherty wrote: On 8/20/14 2:01 PM, Coleen Phillimore wrote: On 8/20/14, 3:49 PM, serguei.spit...@oracle.com wrote: If an EMCP method is not running, should we save it on a previous version list anyway so that we can make it obsolete if it's redefined and made obsolete? I hope, Dan will catch me if I'm wrong... I think, we should not. An EMCP method can not be made obsolete if it is not running. It should be this way otherwise we'd have to hang onto things forever. An EMCP method should only be made obsolete if a RedefineClasses() or RetransformClasses() operation made it so. We should not be leveraging off the obsolete-ness attribute to solve a life-cycle problem. In the pre-PGR world, we could trust GC to make a completely unused EMCP method collectible and eventually our weak reference would go away. Just because an EMCP method is not on a stack does not mean that it is not used so we need a different way to determine whether it is OK to no longer track an EMCP method. Most likely, you are right. But I'm not convinced yet. Sorry. A convincing point would be a test that shows this behavior. I understand that it is not an easy task to write such a test though. However, such a test would serve nicely if we want a different way to determine whether it is OK to no longer track an EMCP method. Thanks, Serguei BTW, I'm reviewing the webrev too, but probably it'd be better to switch to updated webrev after it is ready. Yes, this is a good idea. I figured out why I made emcp methods obsolete, and I'm fixing that as well as Dan's comments. Thanks! Cool! I'm looking forward to the next review. Dan Coleen Thanks, Serguei
RFR(S): JDK-8054066 com/sun/jdi/DoubleAgentTest.java fails with timeout
Please review testfix. http://cr.openjdk.java.net/~dsamersoff/JDK-8054066/webrev.01/ The test is rewritten to use testlibrary instead of custom code. -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: RFR(XS) : 8055755: Information about loaded dynamic libraries is wrong on MacOSX
Looks good! Thanks, /Staffan On 27 aug 2014, at 11:01, Fredrik Arvidsson fredrik.arvids...@oracle.com wrote: Please help me review the following small patch: Review - http://cr.openjdk.java.net/~farvidsson/8055755/ Bug - https://bugs.openjdk.java.net/browse/JDK-8055755 There is information about the problem and the solution added to the bug as a comment. Cheers /Fredrik
Re: RFR(S): JDK-8054066 com/sun/jdi/DoubleAgentTest.java fails with timeout
Looks good! Thanks, /Staffan On 27 aug 2014, at 14:07, Dmitry Samersoff dmitry.samers...@oracle.com wrote: Please review testfix. http://cr.openjdk.java.net/~dsamersoff/JDK-8054066/webrev.01/ The test is rewritten to use testlibrary instead of custom code. -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: RFR(S): URG! JDK-8049226 com/sun/jdi/OptionTest.java test times out again
This version looks good. Reviewed.
/Staffan
On 27 aug 2014, at 12:51, Dmitry Samersoff dmitry.samers...@oracle.com wrote:
Serguei,
Fixed (in-place, press shift reload)
-Dmitry
On 2014-08-27 14:25, serguei.spit...@oracle.com wrote:
One more minor comment.
The indent in this file must be 4.
Could you fix it?
Thanks,
Serguei
On 8/27/14 2:03 AM, Dmitry Samersoff wrote:
Serguei,
Fixed (in-place, press shift reload)
-Dmitry
On 2014-08-27 12:12, serguei.spit...@oracle.com wrote:
1284 enum exit_codes { EXIT_NO_ERRORS = 0, EXIT_TRANSPORT_ERROR,
EXIT_JVMTI_ERROR };
I'd suggest to swap the positions of EXIT_TRANSPORT_ERROR and
EXIT_JVMTI_ERROR
in the enum to keep this as compatible to previous behavior as possible.
Thanks,
Serguei
On 8/27/14 1:08 AM, serguei.spit...@oracle.com wrote:
This looks good.
Could you, please, remove extra spaces in the following conditions ?:
1291 if (error != JVMTI_ERROR_NONE docoredump ) {
1302 if ( gdata-jvmti != NULL ) {
1309 if ( error == JVMTI_ERROR_NONE ) {
Thanks,
Serguei
On 8/27/14 12:36 AM, Dmitry Samersoff wrote:
Serguei,
http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.04/
I refactored the debugInit_exit function.
Now we have separate exit code for transport error and better readable
code.
-Dmitry
On 2014-08-27 00:22, serguei.spit...@oracle.com wrote:
Dmitry,
It makes sense to consider a special exit code for transport init
error.
Other than that it looks good.
No need to re-review.
Thanks,
Serguei
On 8/26/14 10:46 AM, Dmitry Samersoff wrote:
Serguei,
exit_code set to 1 at ll. 1288
I thought of refactoring this code for better readability but
finally
decide to keep changes minimal.
-Dmitry
On 2014-08-26 21:35, serguei.spit...@oracle.com wrote:
Dmitry,
This looks good in general.
But what error code will be returned in the case of
AGENT_ERROR_TRANSPORT_INIT ?
Is it Ok to return whatever exit_code we already have or we better
return some special error code?
Probably, it is Ok to keep the comment at the line 1315 as it is.
Thanks,
Serguei
On 8/26/14 7:47 AM, Dmitry Samersoff wrote:
Staffan,
http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.03/
After couple of experiments I end up with simple fix - don't
coredump on
AGENT_ERROR_TRANSPORT_INIT
Current code don't propagate transport error to upper level, so
if we
need fine grained control I'll rewrite transport initialization.
-Dmitry
On 2014-08-25 15:53, Staffan Larsen wrote:
On 25 aug 2014, at 13:33, Dmitry Samersoff
dmitry.samers...@oracle.com
mailto:dmitry.samers...@oracle.com wrote:
Staffan,
On 2014-08-25 15:26, Staffan Larsen wrote:
Dmitry,
One problem with this fix is that debugInit_exit() is used
from many
places in the JDWP code. For some of these I think it still
does
make
sense to receive a core dump for analysis. I agree that when
parsing
options, we do not need a core dump.
jdwp has explicit option to request a coredump at
debugInit_exit().
Is it enough or I should create a more sophisticated fix ?
I don’t think that is enough. Often a problem will happen and
will not
be reproducible.
Maybe this code:
if ((arg.error != JDWP_ERROR(NONE))
(arg.startCount == 0)
initOnStartup) {
EXIT_ERROR(map2jvmtiError(arg.error), No transports
initialized);
}
can use some variant of EXIT_ERROR that does not call
fatalError() ?
/Staffan
Actually, I don't think that coredump on every call to
jni_FatalError()
(see jni.cpp:726) is necessary but this hotspot code is here
for 6
years
and changing it probably out of scope of this fix.
-Dmitry
/Staffan
On 20 aug 2014, at 17:55, Dmitry Samersoff
dmitry.samers...@oracle.com
mailto:dmitry.samers...@oracle.com wrote:
Serguei,
After some additional testing I changed the fix a bit.
Please take
a look at new version.
http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/
New version reports JVMTI error to stderr.
Also jniFatalError is not referenced any more so I remove it.
-Dmitry
On 2014-08-20 12:08, serguei.spit...@oracle.com wrote:
Ok.
Thank you for the explanation! Serguei
On 8/20/14 1:01 AM, Dmitry Samersoff wrote:
Serguei,
1. Historically JDI test-suite had no tests for failed
transport initialization behavior and invalid parameters
handling.
2. As a part of JDWP hardening work I added couple of such
tests to OptionTest.java - these tests pass invalid
parameters
to dt_socket transport to make sure that transport doesn't
crash (one such crash was discovered and fixed) but just
return
non-zero exit code to upper level.
3. After fix for JDK-6694099 any non-zero exit code from
transport cause VM to coredump. Dumping multiple cores on
busy
machine takes a time so harness kills the test by timeout.
We can just increase timeout
Re: RFR 8055008: Clean up code that saves the previous versions of redefined classes
On 8/27/14 5:40 AM, serguei.spit...@oracle.com wrote: On 8/20/14 3:45 PM, Daniel D. Daugherty wrote: On 8/20/14 2:01 PM, Coleen Phillimore wrote: On 8/20/14, 3:49 PM, serguei.spit...@oracle.com wrote: If an EMCP method is not running, should we save it on a previous version list anyway so that we can make it obsolete if it's redefined and made obsolete? I hope, Dan will catch me if I'm wrong... I think, we should not. An EMCP method can not be made obsolete if it is not running. It should be this way otherwise we'd have to hang onto things forever. An EMCP method should only be made obsolete if a RedefineClasses() or RetransformClasses() operation made it so. We should not be leveraging off the obsolete-ness attribute to solve a life-cycle problem. In the pre-PGR world, we could trust GC to make a completely unused EMCP method collectible and eventually our weak reference would go away. Just because an EMCP method is not on a stack does not mean that it is not used so we need a different way to determine whether it is OK to no longer track an EMCP method. Most likely, you are right. But I'm not convinced yet. Sorry. A convincing point would be a test that shows this behavior. I understand that it is not an easy task to write such a test though. However, such a test would serve nicely if we want a different way to determine whether it is OK to no longer track an EMCP method. Running the Serviceability stack of tests with the following -XX:TraceRedefineClasses=NN flags: //0x1000 | 4096 - detect calls to obsolete methods //0x2000 | 8192 - fail a guarantee() in addition to detection will show failures of the guarantee(). I used to do this periodically and then chase down the failures to make sure only the legitimate races were happening. The reason that we had to add these flags was to find all the places where folks were caching methods in the VM. I think the last place I found and fixed was in reflection. Dan Thanks, Serguei BTW, I'm reviewing the webrev too, but probably it'd be better to switch to updated webrev after it is ready. Yes, this is a good idea. I figured out why I made emcp methods obsolete, and I'm fixing that as well as Dan's comments. Thanks! Cool! I'm looking forward to the next review. Dan Coleen Thanks, Serguei
Re: 8056148: Add java/lang/management/MemoryMXBean/LowMemoryTest.java to ProblemList.txt
Hi Stefan, Thanks for fixing the separate bug. Looks good. Bengt On 8/27/14 11:11 AM, Stefan Karlsson wrote: Hi all, Please review this patch to put the LowMemoryTest.java test in the ProblemLists.txt. It currently hangs and leaves processes running after the test run has completed. http://cr.openjdk.java.net/~stefank/8056148/webrev.00/ thanks, StefanK
Re: RFR: JDK-8055845 - Add trace event for promoted objects
Hi Staffan, Overall I think this change looks fine. I know that Erik Helin has some thoughts on the naming of the events. I'll let him communicate those thoughts. Apart from Erik's comments and the missing test case (that I know you are working on) I think the change looks good. Thanks, Bengt On 8/26/14 10:50 AM, Bengt Rutisson wrote: Hi all, Staffan sent me an updated webrev based on Erik's comments. I uploaded it here: http://cr.openjdk.java.net/~brutisso/8055845/webrev.01/ Thanks, Bengt On 2014-08-25 19:32, Staffan Friberg wrote: Hi Erik, No issue with naming the field class, since the event is similar to the Allocation event I used that as a starting point and it also uses class as name together with a couple of other events. Will go through the descriptions and remove periods. Object age is basically the how many times an object has been kept in survivor region, it is incremented each time a YC happens and the object is aged. Will see how I can update the description to make it clearer. Calling the field tenuringAge might help? From the documentation, http://docs.oracle.com/javase/8/docs/technotes/tools/unix/java.html -XX:MaxTenuringThreshold=/threshold/ Sets the maximum tenuring threshold for use in adaptive GC sizing. The largest value is 15. The default value is 15 for the parallel (throughput) collector, and 6 for the CMS collector. The following example shows how to set the maximum tenuring threshold to 10: -XX:MaxTenuringThreshold=10 -XX:+PrintTenuringDistribution Enables printing of tenuring age information. The following is an example of the output: Desired survivor size 48286924 bytes, new threshold 10 (max 10) - age 1: 28992024 bytes, 28992024 total - age 2: 1366864 bytes, 3035 total - age 3: 1425912 bytes, 31784800 total ... Age 1 objects are the youngest survivors (they were created after the previous scavenge, survived the latest scavenge, and moved from eden to survivor space). Age 2 objects have survived two scavenges (during the second scavenge they were copied from one survivor space to the next). And so on. In the preceding example, 28 992 024 bytes survived one scavenge and were copied from eden to survivor space, 1 366 864 bytes are occupied by age 2 objects, etc. The third value in each row is the cumulative size of objects of age n or less. By default, this option is disabled. Thanks for the comments, Staffan On 08/25/2014 09:55 AM, Erik Gahlin wrote: Did you manage to call the field class? It's a reserved word in C++, so we had to use klass in the past Descriptions with only one sentence shouldn't end with . How is Object Age measured? As a user, I would expect the number to be in seconds/minutes/hours, but that is not the case. Perhaps an explanation in the description and/or a field name that better reflects what we really mean with age. Otherwise trace.xml looks good! Erik Staffan Friberg skrev 25/08/14 18:28: Hi, Could I please get reviews for this RFE that adds a trace event for aging and promotion for young collections in G1, CMS and Parallel GC. It works similarly to allocation events, and generates the event on the slow path when either a direct copy occurs or a new PLAB is request. Since I'm adding an event I cc:ed the serviceability list in case anyone has any comments on the event and changes to trace.xml. RFE: https://bugs.openjdk.java.net/browse/JDK-8055845 Webrev: http://cr.openjdk.java.net/~brutisso/8055845/webrev.00 Bengt has been kind and agreed to both sponsor and host the the webrev. Thanks, Staffan
Re: RFR: JDK-8055845 - Add trace event for promoted objects
Hi Staffan, Thanks for submitting RFE and implementing it! This event seems to be used widely. So it's very important to design it correctly. May I ask you to provide some extra information: - format of event (this could be included in the CR description) - regression tests for that event will be also required - please include hotspot-dev on CC Thanks, Dima On 25.08.2014 20:28, Staffan Friberg wrote: Hi, Could I please get reviews for this RFE that adds a trace event for aging and promotion for young collections in G1, CMS and Parallel GC. It works similarly to allocation events, and generates the event on the slow path when either a direct copy occurs or a new PLAB is request. Since I'm adding an event I cc:ed the serviceability list in case anyone has any comments on the event and changes to trace.xml. RFE: https://bugs.openjdk.java.net/browse/JDK-8055845 Webrev: http://cr.openjdk.java.net/~brutisso/8055845/webrev.00 Bengt has been kind and agreed to both sponsor and host the the webrev. Thanks, Staffan
Re: RFR(XS) : 8055755: Information about loaded dynamic libraries is wrong on MacOSX
Thank you for fixing this. Please consider it reviewed (with lowercase r) cheers On 8/27/2014 4:01 AM, Fredrik Arvidsson wrote: Please help me review the following small patch: Review - http://cr.openjdk.java.net/~farvidsson/8055755/ Bug - https://bugs.openjdk.java.net/browse/JDK-8055755 There is information about the problem and the solution added to the bug as a comment. Cheers /Fredrik
Re: RFR: JDK-8055845 - Add trace event for promoted objects
HI Dima, The event is specified in trace.xml as all other events, please see the updated webrev as some changes was made to the descriptions. http://cr.openjdk.java.net/~brutisso/8055845/webrev.01 I'm in the process of writing regression tests, but they will be a separate webrev as they will be located in a separate repository. Regards, Staffan On 08/27/2014 04:08 AM, Dmitry Fazunenko wrote: Hi Staffan, Thanks for submitting RFE and implementing it! This event seems to be used widely. So it's very important to design it correctly. May I ask you to provide some extra information: - format of event (this could be included in the CR description) - regression tests for that event will be also required - please include hotspot-dev on CC Thanks, Dima On 25.08.2014 20:28, Staffan Friberg wrote: Hi, Could I please get reviews for this RFE that adds a trace event for aging and promotion for young collections in G1, CMS and Parallel GC. It works similarly to allocation events, and generates the event on the slow path when either a direct copy occurs or a new PLAB is request. Since I'm adding an event I cc:ed the serviceability list in case anyone has any comments on the event and changes to trace.xml. RFE: https://bugs.openjdk.java.net/browse/JDK-8055845 Webrev: http://cr.openjdk.java.net/~brutisso/8055845/webrev.00 Bengt has been kind and agreed to both sponsor and host the the webrev. Thanks, Staffan
Re: RFR: JDK-8055845 - Add trace event for promoted objects
Staffan, Yes, I saw trace.xml. But I think this event not specified there, but implemented ) I will appreciate if you copy it in the message, people might review that and decide if the event contain all the necessary fields. And thank you for working on tests! -- Dima On 27.08.2014 20:52, Staffan Friberg wrote: HI Dima, The event is specified in trace.xml as all other events, please see the updated webrev as some changes was made to the descriptions. http://cr.openjdk.java.net/~brutisso/8055845/webrev.01 I'm in the process of writing regression tests, but they will be a separate webrev as they will be located in a separate repository. Regards, Staffan On 08/27/2014 04:08 AM, Dmitry Fazunenko wrote: Hi Staffan, Thanks for submitting RFE and implementing it! This event seems to be used widely. So it's very important to design it correctly. May I ask you to provide some extra information: - format of event (this could be included in the CR description) - regression tests for that event will be also required - please include hotspot-dev on CC Thanks, Dima On 25.08.2014 20:28, Staffan Friberg wrote: Hi, Could I please get reviews for this RFE that adds a trace event for aging and promotion for young collections in G1, CMS and Parallel GC. It works similarly to allocation events, and generates the event on the slow path when either a direct copy occurs or a new PLAB is request. Since I'm adding an event I cc:ed the serviceability list in case anyone has any comments on the event and changes to trace.xml. RFE: https://bugs.openjdk.java.net/browse/JDK-8055845 Webrev: http://cr.openjdk.java.net/~brutisso/8055845/webrev.00 Bengt has been kind and agreed to both sponsor and host the the webrev. Thanks, Staffan
Re: RFR 8055008: Clean up code that saves the previous versions of redefined classes
On 8/27/14 6:54 AM, Daniel D. Daugherty wrote: On 8/27/14 5:40 AM, serguei.spit...@oracle.com wrote: On 8/20/14 3:45 PM, Daniel D. Daugherty wrote: On 8/20/14 2:01 PM, Coleen Phillimore wrote: On 8/20/14, 3:49 PM, serguei.spit...@oracle.com wrote: If an EMCP method is not running, should we save it on a previous version list anyway so that we can make it obsolete if it's redefined and made obsolete? I hope, Dan will catch me if I'm wrong... I think, we should not. An EMCP method can not be made obsolete if it is not running. It should be this way otherwise we'd have to hang onto things forever. An EMCP method should only be made obsolete if a RedefineClasses() or RetransformClasses() operation made it so. We should not be leveraging off the obsolete-ness attribute to solve a life-cycle problem. In the pre-PGR world, we could trust GC to make a completely unused EMCP method collectible and eventually our weak reference would go away. Just because an EMCP method is not on a stack does not mean that it is not used so we need a different way to determine whether it is OK to no longer track an EMCP method. Most likely, you are right. But I'm not convinced yet. Sorry. A convincing point would be a test that shows this behavior. I understand that it is not an easy task to write such a test though. However, such a test would serve nicely if we want a different way to determine whether it is OK to no longer track an EMCP method. Running the Serviceability stack of tests with the following -XX:TraceRedefineClasses=NN flags: //0x1000 | 4096 - detect calls to obsolete methods //0x2000 | 8192 - fail a guarantee() in addition to detection will show failures of the guarantee(). I used to do this periodically and then chase down the failures to make sure only the legitimate races were happening. The reason that we had to add these flags was to find all the places where folks were caching methods in the VM. I think the last place I found and fixed was in reflection. Ok, thanks. We threat this as buggy behavior, right? Thanks, Serguei Dan Thanks, Serguei BTW, I'm reviewing the webrev too, but probably it'd be better to switch to updated webrev after it is ready. Yes, this is a good idea. I figured out why I made emcp methods obsolete, and I'm fixing that as well as Dan's comments. Thanks! Cool! I'm looking forward to the next review. Dan Coleen Thanks, Serguei
Re: RFR 8055008: Clean up code that saves the previous versions of redefined classes
On 8/27/14 12:41 PM, serguei.spit...@oracle.com wrote: On 8/27/14 6:54 AM, Daniel D. Daugherty wrote: On 8/27/14 5:40 AM, serguei.spit...@oracle.com wrote: On 8/20/14 3:45 PM, Daniel D. Daugherty wrote: On 8/20/14 2:01 PM, Coleen Phillimore wrote: On 8/20/14, 3:49 PM, serguei.spit...@oracle.com wrote: If an EMCP method is not running, should we save it on a previous version list anyway so that we can make it obsolete if it's redefined and made obsolete? I hope, Dan will catch me if I'm wrong... I think, we should not. An EMCP method can not be made obsolete if it is not running. It should be this way otherwise we'd have to hang onto things forever. An EMCP method should only be made obsolete if a RedefineClasses() or RetransformClasses() operation made it so. We should not be leveraging off the obsolete-ness attribute to solve a life-cycle problem. In the pre-PGR world, we could trust GC to make a completely unused EMCP method collectible and eventually our weak reference would go away. Just because an EMCP method is not on a stack does not mean that it is not used so we need a different way to determine whether it is OK to no longer track an EMCP method. Most likely, you are right. But I'm not convinced yet. Sorry. A convincing point would be a test that shows this behavior. I understand that it is not an easy task to write such a test though. However, such a test would serve nicely if we want a different way to determine whether it is OK to no longer track an EMCP method. Running the Serviceability stack of tests with the following -XX:TraceRedefineClasses=NN flags: //0x1000 | 4096 - detect calls to obsolete methods //0x2000 | 8192 - fail a guarantee() in addition to detection will show failures of the guarantee(). I used to do this periodically and then chase down the failures to make sure only the legitimate races were happening. The reason that we had to add these flags was to find all the places where folks were caching methods in the VM. I think the last place I found and fixed was in reflection. Ok, thanks. We threat this as buggy behavior, right? Not necessarily. If it's because of a cached method that didn't get updated to the new version, then that's a bug. However, if it's because of a call that is in progress, then we have not considered that a bug in the past. I don't remember the exact details, but the dance is something like this: - jmethodID converted to methodHandle - call to methodHandle prepared: - methodHandle - methodOop - parameters marshalled - methodOop converted to method impl - method called - somewhere in the middle of call sequence the method is redefined - jmethodID and methodHandle are updated to refer to the new method, but we already converted to the methodOop and the underlying method implementation for the final stages of the call - when we've actually called the now obsolete method, the guarantee() mentioned above fires and we have a false positive for the tracing code Of course, now that we don't have methodOops, maybe this doesn't happen anymore. I haven't done a test run with the above flags enabled in quite some time... Dan Thanks, Serguei Dan Thanks, Serguei BTW, I'm reviewing the webrev too, but probably it'd be better to switch to updated webrev after it is ready. Yes, this is a good idea. I figured out why I made emcp methods obsolete, and I'm fixing that as well as Dan's comments. Thanks! Cool! I'm looking forward to the next review. Dan Coleen Thanks, Serguei
Re: RFR(XS) : 8055755: Information about loaded dynamic libraries is wrong on MacOSX
Great guys and thanks. /Fredrik On 2014-08-27 18:27, Gerard Ziemski wrote: Thank you for fixing this. Please consider it reviewed (with lowercase r) cheers On 8/27/2014 4:01 AM, Fredrik Arvidsson wrote: Please help me review the following small patch: Review - http://cr.openjdk.java.net/~farvidsson/8055755/ Bug - https://bugs.openjdk.java.net/browse/JDK-8055755 There is information about the problem and the solution added to the bug as a comment. Cheers /Fredrik
Re: RFR 8055008: Clean up code that saves the previous versions of redefined classes
On 8/27/14, 3:33 PM, Daniel D. Daugherty wrote: On 8/27/14 12:41 PM, serguei.spit...@oracle.com wrote: On 8/27/14 6:54 AM, Daniel D. Daugherty wrote: On 8/27/14 5:40 AM, serguei.spit...@oracle.com wrote: On 8/20/14 3:45 PM, Daniel D. Daugherty wrote: On 8/20/14 2:01 PM, Coleen Phillimore wrote: On 8/20/14, 3:49 PM, serguei.spit...@oracle.com wrote: If an EMCP method is not running, should we save it on a previous version list anyway so that we can make it obsolete if it's redefined and made obsolete? I hope, Dan will catch me if I'm wrong... I think, we should not. An EMCP method can not be made obsolete if it is not running. It should be this way otherwise we'd have to hang onto things forever. An EMCP method should only be made obsolete if a RedefineClasses() or RetransformClasses() operation made it so. We should not be leveraging off the obsolete-ness attribute to solve a life-cycle problem. In the pre-PGR world, we could trust GC to make a completely unused EMCP method collectible and eventually our weak reference would go away. Just because an EMCP method is not on a stack does not mean that it is not used so we need a different way to determine whether it is OK to no longer track an EMCP method. Most likely, you are right. But I'm not convinced yet. Sorry. A convincing point would be a test that shows this behavior. I understand that it is not an easy task to write such a test though. However, such a test would serve nicely if we want a different way to determine whether it is OK to no longer track an EMCP method. Running the Serviceability stack of tests with the following -XX:TraceRedefineClasses=NN flags: //0x1000 | 4096 - detect calls to obsolete methods //0x2000 | 8192 - fail a guarantee() in addition to detection will show failures of the guarantee(). I used to do this periodically and then chase down the failures to make sure only the legitimate races were happening. The reason that we had to add these flags was to find all the places where folks were caching methods in the VM. I think the last place I found and fixed was in reflection. Ok, thanks. We threat this as buggy behavior, right? Not necessarily. If it's because of a cached method that didn't get updated to the new version, then that's a bug. However, if it's because of a call that is in progress, then we have not considered that a bug in the past. I don't remember the exact details, but the dance is something like this: - jmethodID converted to methodHandle - call to methodHandle prepared: - methodHandle - methodOop - parameters marshalled - methodOop converted to method impl - method called - somewhere in the middle of call sequence the method is redefined - jmethodID and methodHandle are updated to refer to the new method, but we already converted to the methodOop and the underlying method implementation for the final stages of the call - when we've actually called the now obsolete method, the guarantee() mentioned above fires and we have a false positive for the tracing code Of course, now that we don't have methodOops, maybe this doesn't happen anymore. I haven't done a test run with the above flags enabled in quite some time... Do you mean methodHandle or MethodHandle above? Or java_lang_reflect_Method? The methodOop in little m methodHandles are not updated to refer to the new method, and Method* isn't either, so that really hasn't changed. I'm not following the call sequence above. But yes, I believe we could get into javaCalls::call() with an method, stop for a safepoint, and end up calling an obsolete method. But that obsolete method is considered already running at that stage because the methodHandle logic marks it as such, so it's not considered a bug. I ran the tests with the -XX:TraceRedefineClasses=0x2000 and didn't get the guarantee though, but that doesn't mean much. I'm reading these mails in reverse... Thanks, Coleen Dan Thanks, Serguei Dan Thanks, Serguei BTW, I'm reviewing the webrev too, but probably it'd be better to switch to updated webrev after it is ready. Yes, this is a good idea. I figured out why I made emcp methods obsolete, and I'm fixing that as well as Dan's comments. Thanks! Cool! I'm looking forward to the next review. Dan Coleen Thanks, Serguei
