Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
Okay. But I think we all agree we should at least bring this test back for further tacking. http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.06/ Thank you. Tristan On 01/14/2014 10:39 AM, David Holmes wrote: On 13/01/2014 4:21 PM, Tristan Yan wrote: Hi All I add more trace output to track down possible reason of this failure. Please help to review it again. http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.05/ You seem to have dragged in some unrelated changes to ProblemList.txt Also I don't see much in the way of trace output. I see two potentially useful printlns (and one unnecessary one). Further all the changes from the static variables are still there but we already determined that they should have no affect on the running of the test, nor did they explain the failure mode. So other than reenabling this test to see if it actually still fails, I don't see the point of the functional changes. Sorry. David Thank you Tristan On 01/10/2014 07:20 AM, Tristan Yan wrote: Hi David I wasn't able to reproduce this failure either in local or in our same binaries running(This is a continuous running with same JDK binaries). So intention for this code change is bringing this test back; add some debug info and try to avoid possible issues in this test. I agree this code change won't solve the failure happened. But this test was put into ProblemList two years ago better move for this is move out it from ProblemList and trace down the issue in our normal nightly. Thank you Tristan On 01/10/2014 06:35 AM, David Holmes wrote: On 9/01/2014 10:14 PM, Alan Bateman wrote: On 09/01/2014 11:27, David Holmes wrote: Okay I think I get it now. Both MonitorTest and WaitersTest use the Context class, so if both tests run in the same VM the second test will see the static total_turns_taken and turn in the state they were left from the first test - hence the second test will always fail. The bug report suggests making the tests othervm to avoid the problem but instead you have changed from using static state to instance state so that there is no interference. I haven't been following this one closely but I thought that jtreg created a class loader for each test (irrespective of mode) so I wouldn't expect statics to be an issue. That aside DemoRun forks off its own JVM to run a given test anyway! So I don't understand how the proposed fixes could actually be addressing the hangs that are occurring. Even if the statics were being shared I don't see how that leads to the failure mode in the bug report. David -Alan.
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
On 14/01/2014 12:40, Tristan Yan wrote: Okay. But I think we all agree we should at least bring this test back for further tacking. http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.06/ This make sense so I think this should be pushed. If JDK-7027502 is used for this then the synopsis should be changed as it would otherwise not reflect the fact that this is not fixing the issue, rather just liberating the test with some additional output to help in the event that it fails in the future. -Alan
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
Thank you Alan. I have changed this bug's synopsis. Is it okay you could push this? Tristan On 01/14/2014 09:48 PM, Alan Bateman wrote: On 14/01/2014 12:40, Tristan Yan wrote: Okay. But I think we all agree we should at least bring this test back for further tacking. http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.06/ This make sense so I think this should be pushed. If JDK-7027502 is used for this then the synopsis should be changed as it would otherwise not reflect the fact that this is not fixing the issue, rather just liberating the test with some additional output to help in the event that it fails in the future. -Alan
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
Looks good! Thanks, /Staffan On 13 jan 2014, at 07:21, Tristan Yan tristan@oracle.com wrote: Hi All I add more trace output to track down possible reason of this failure. Please help to review it again. http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.05/ Thank you Tristan On 01/10/2014 07:20 AM, Tristan Yan wrote: Hi David I wasn't able to reproduce this failure either in local or in our same binaries running(This is a continuous running with same JDK binaries). So intention for this code change is bringing this test back; add some debug info and try to avoid possible issues in this test. I agree this code change won't solve the failure happened. But this test was put into ProblemList two years ago better move for this is move out it from ProblemList and trace down the issue in our normal nightly. Thank you Tristan On 01/10/2014 06:35 AM, David Holmes wrote: On 9/01/2014 10:14 PM, Alan Bateman wrote: On 09/01/2014 11:27, David Holmes wrote: Okay I think I get it now. Both MonitorTest and WaitersTest use the Context class, so if both tests run in the same VM the second test will see the static total_turns_taken and turn in the state they were left from the first test - hence the second test will always fail. The bug report suggests making the tests othervm to avoid the problem but instead you have changed from using static state to instance state so that there is no interference. I haven't been following this one closely but I thought that jtreg created a class loader for each test (irrespective of mode) so I wouldn't expect statics to be an issue. That aside DemoRun forks off its own JVM to run a given test anyway! So I don't understand how the proposed fixes could actually be addressing the hangs that are occurring. Even if the statics were being shared I don't see how that leads to the failure mode in the bug report. David -Alan.
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
On 13/01/2014 4:21 PM, Tristan Yan wrote: Hi All I add more trace output to track down possible reason of this failure. Please help to review it again. http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.05/ You seem to have dragged in some unrelated changes to ProblemList.txt Also I don't see much in the way of trace output. I see two potentially useful printlns (and one unnecessary one). Further all the changes from the static variables are still there but we already determined that they should have no affect on the running of the test, nor did they explain the failure mode. So other than reenabling this test to see if it actually still fails, I don't see the point of the functional changes. Sorry. David Thank you Tristan On 01/10/2014 07:20 AM, Tristan Yan wrote: Hi David I wasn't able to reproduce this failure either in local or in our same binaries running(This is a continuous running with same JDK binaries). So intention for this code change is bringing this test back; add some debug info and try to avoid possible issues in this test. I agree this code change won't solve the failure happened. But this test was put into ProblemList two years ago better move for this is move out it from ProblemList and trace down the issue in our normal nightly. Thank you Tristan On 01/10/2014 06:35 AM, David Holmes wrote: On 9/01/2014 10:14 PM, Alan Bateman wrote: On 09/01/2014 11:27, David Holmes wrote: Okay I think I get it now. Both MonitorTest and WaitersTest use the Context class, so if both tests run in the same VM the second test will see the static total_turns_taken and turn in the state they were left from the first test - hence the second test will always fail. The bug report suggests making the tests othervm to avoid the problem but instead you have changed from using static state to instance state so that there is no interference. I haven't been following this one closely but I thought that jtreg created a class loader for each test (irrespective of mode) so I wouldn't expect statics to be an issue. That aside DemoRun forks off its own JVM to run a given test anyway! So I don't understand how the proposed fixes could actually be addressing the hangs that are occurring. Even if the statics were being shared I don't see how that leads to the failure mode in the bug report. David -Alan.
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
Hi All I add more trace output to track down possible reason of this failure. Please help to review it again. http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.05/ Thank you Tristan On 01/10/2014 07:20 AM, Tristan Yan wrote: Hi David I wasn't able to reproduce this failure either in local or in our same binaries running(This is a continuous running with same JDK binaries). So intention for this code change is bringing this test back; add some debug info and try to avoid possible issues in this test. I agree this code change won't solve the failure happened. But this test was put into ProblemList two years ago better move for this is move out it from ProblemList and trace down the issue in our normal nightly. Thank you Tristan On 01/10/2014 06:35 AM, David Holmes wrote: On 9/01/2014 10:14 PM, Alan Bateman wrote: On 09/01/2014 11:27, David Holmes wrote: Okay I think I get it now. Both MonitorTest and WaitersTest use the Context class, so if both tests run in the same VM the second test will see the static total_turns_taken and turn in the state they were left from the first test - hence the second test will always fail. The bug report suggests making the tests othervm to avoid the problem but instead you have changed from using static state to instance state so that there is no interference. I haven't been following this one closely but I thought that jtreg created a class loader for each test (irrespective of mode) so I wouldn't expect statics to be an issue. That aside DemoRun forks off its own JVM to run a given test anyway! So I don't understand how the proposed fixes could actually be addressing the hangs that are occurring. Even if the statics were being shared I don't see how that leads to the failure mode in the bug report. David -Alan.
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
On 09/01/2014 23:20, Tristan Yan wrote: Hi David I wasn't able to reproduce this failure either in local or in our same binaries running(This is a continuous running with same JDK binaries). So intention for this code change is bringing this test back; add some debug info and try to avoid possible issues in this test. I agree this code change won't solve the failure happened. But this test was put into ProblemList two years ago better move for this is move out it from ProblemList and trace down the issue in our normal nightly. If we can't duplicate it now, and the output from previously reported failures (in 2011) is insufficient to diagnose it properly, then it seems reasonable to add better output so as to improve our chances of understanding if it fails again. So better output + removing from the exclude list seems fine here. (cc'ing serviceability-dev as that is actually the mailing list for the HPROF and JVM TI areas). -Alan
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
On 10 jan 2014, at 09:34, Alan Bateman alan.bate...@oracle.com wrote: On 09/01/2014 23:20, Tristan Yan wrote: Hi David I wasn't able to reproduce this failure either in local or in our same binaries running(This is a continuous running with same JDK binaries). So intention for this code change is bringing this test back; add some debug info and try to avoid possible issues in this test. I agree this code change won't solve the failure happened. But this test was put into ProblemList two years ago better move for this is move out it from ProblemList and trace down the issue in our normal nightly. If we can't duplicate it now, and the output from previously reported failures (in 2011) is insufficient to diagnose it properly, then it seems reasonable to add better output so as to improve our chances of understanding if it fails again. So better output + removing from the exclude list seems fine here. (cc'ing serviceability-dev as that is actually the mailing list for the HPROF and JVM TI areas). Sounds good to me, /Staffan -Alan
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
On 10/01/2014 6:40 PM, Staffan Larsen wrote: On 10 jan 2014, at 09:34, Alan Bateman alan.bate...@oracle.com wrote: On 09/01/2014 23:20, Tristan Yan wrote: Hi David I wasn't able to reproduce this failure either in local or in our same binaries running(This is a continuous running with same JDK binaries). So intention for this code change is bringing this test back; add some debug info and try to avoid possible issues in this test. I agree this code change won't solve the failure happened. But this test was put into ProblemList two years ago better move for this is move out it from ProblemList and trace down the issue in our normal nightly. If we can't duplicate it now, and the output from previously reported failures (in 2011) is insufficient to diagnose it properly, then it seems reasonable to add better output so as to improve our chances of understanding if it fails again. So better output + removing from the exclude list seems fine here. (cc'ing serviceability-dev as that is actually the mailing list for the HPROF and JVM TI areas). Sounds good to me, I'm not sure what is actually being proposed. I don't really see anything that would help diagnoze the original issue. But bring back the test - maybe this was a bug elsewhere that has now been fixed. David /Staffan -Alan
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
On 10/01/2014 10:37, David Holmes wrote: I'm not sure what is actually being proposed. I don't really see anything that would help diagnoze the original issue. But bring back the test - maybe this was a bug elsewhere that has now been fixed. I think the proposal is as we said, more diagnostic output + remove from exclude list. If Tristan's agrees then I'm sure he'll update the patch. On your second point then you may be right, there were a number of issues during JDK 8 and it's very possible that the ghost being chasing now is gone. -Alan.
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
Can someone else give a second review on this.
Thank you
Tristan
On 01/07/2014 07:29 PM, David Holmes wrote:
On 7/01/2014 8:36 PM, Tristan Yan wrote:
Hi David
You're totally right. Sorry I ask you review it again.
http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.02/
Looks good now.
Thanks,
David
Thank you very much.
Tristan
On 01/07/2014 05:18 PM, David Holmes wrote:
On 7/01/2014 6:16 PM, Tristan Yan wrote:
Thank you, David
I fixed copyright and change back sleep.
println was intended to be left in. This test was failed with timeout,
printf could help us to detect the value of total_turns_taken and
expected_turns_taken.
Please review it again
http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.01/
Comma after 2014 still missing in copyright.
You need to read total_turns_taken.get() once and use that value in
both the println and the == test, so that you print the value you
actually tested.
David
Regards
Tristan
On 01/07/2014 03:10 PM, David Holmes wrote:
Hi Tristan,
On 7/01/2014 4:43 PM, Tristan Yan wrote:
Hi All
Please help to review the code change for JDK-7027502.
http://cr.openjdk.java.net/~tyan/JDK-7027502/
Description:
This test was failed in JPRT test but recently we test with same
binaries run, it doesn't fail any more. The intention for this code
change is bringing this test back to normal test and make this test
robust and more informative. Change includes
1. Remove this test from ProblemList.
2. Change static member total_turns_taken into a local variable.
Please let me know your comment on this.
2 * Copyright (c) 2004,2014 Oracle and/or its affiliates. All
rights reserved.
Correct copyright format should have a space before 2014 and a comma
after:
* Copyright (c) 2004, 2014, Oracle and/or its affiliates. All rights
reserved.
---
Was this println intended to be left in?
114 System.out.println(total_turns_taken = +
total_turns_taken +
115 ;expected_turns_taken = +
expected_turns_taken);
116 if ( total_turns_taken.get() ==
expected_turns_taken ) {
You only want to read total_turns_taken once otherwise you may see
misleading print outs.
---
119 /* Create some monitor contention by sleeping
with
lock */
120 if ( default_contention_sleep 0 ) {
121 System.out.println(Context sleeping, to
create contention);
122 try {
123 turn.wait((long)
default_contention_sleep);
124 } catch (InterruptedException ignore) { }
125 }
By changing the Thread.sleep to turn.wait you no longer introduce any
contention as the wait() will release the monitor.
David
Thank you.
Tristan
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
On Jan 9, 2014, at 10:52 AM, Tristan Yan tristan@oracle.com wrote: Can someone else give a second review on this. In a comment the bug you state: here total_turns_taken is a static variable, it could affect by other tests I don't quite know under what test circumstances that can happen, but if so is the following also an issue: 52 private final static TurnChecker turn = new TurnChecker(-1); ? FWIW an alternative to using an AtomicInteger would be for the main loop to sum up thread_turns of each Context, since read/writes are all performed in a synchronized block. Paul.
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
Thank you Paul I change turn to local variable as well. http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.03/ http://cr.openjdk.java.net/%7Etyan/JDK-7027502/webrev.03/ I am not sure I understand your second suggestion here, sum up thread_turns of each Context(This is a fixed value) doesn't equal total_turns_taken. Regards Tristan On 01/09/2014 06:28 PM, Paul Sandoz wrote: On Jan 9, 2014, at 10:52 AM, Tristan Yan tristan@oracle.com wrote: Can someone else give a second review on this. In a comment the bug you state: here total_turns_taken is a static variable, it could affect by other tests I don't quite know under what test circumstances that can happen, but if so is the following also an issue: 52 private final static TurnChecker turn = new TurnChecker(-1); ? FWIW an alternative to using an AtomicInteger would be for the main loop to sum up thread_turns of each Context, since read/writes are all performed in a synchronized block. Paul.
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
On 9/01/2014 8:28 PM, Paul Sandoz wrote: On Jan 9, 2014, at 10:52 AM, Tristan Yan tristan@oracle.com wrote: Can someone else give a second review on this. In a comment the bug you state: here total_turns_taken is a static variable, it could affect by other tests I don't quite know under what test circumstances that can happen, but if so is the following also an issue: 52 private final static TurnChecker turn = new TurnChecker(-1); ? FWIW an alternative to using an AtomicInteger would be for the main loop to sum up thread_turns of each Context, since read/writes are all performed in a synchronized block. Now that you mention it ... I had mistakenly thought the switch to an AtomicInteger was to ensure the ++ was actually atomic, but as you note these updates all take place within a synchronized block that locks the same turn instance of TurnChecker. So now I'm confused by the change. Tristan what was the basis of making the change that you did ? If the issue was that concurrent hprof tests all used the same Context class in the same VM (how??) then as Paul says the TurnChecker instance would also be a problem. Otherwise the change makes no real difference to the semantics of the test as far as I can see. David Paul.
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
On Jan 9, 2014, at 12:07 PM, Tristan Yan tristan@oracle.com wrote: Thank you Paul I change turn to local variable as well. http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.03/ I am not sure I understand your second suggestion here, sum up thread_turns of each Context(This is a fixed value) doesn't equal total_turns_taken. The suggestion (feel free to ignore it!) was to take the local variable: 161 int turns_taken = 0; and move it to a field on Context. Then in the synchronized block of the main loop sum 'em up. e.g.: ListContext cs = ; ... int current_total_turns_taken = cs.stream().mapToInt(c - c.turns_taken).sum(); Obviously that is less efficient but it does separate concerns. Paul.
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
On 9/01/2014 9:07 PM, Tristan Yan wrote: Thank you Paul I change turn to local variable as well. http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.03/ http://cr.openjdk.java.net/%7Etyan/JDK-7027502/webrev.03/ Okay I think I get it now. Both MonitorTest and WaitersTest use the Context class, so if both tests run in the same VM the second test will see the static total_turns_taken and turn in the state they were left from the first test - hence the second test will always fail. The bug report suggests making the tests othervm to avoid the problem but instead you have changed from using static state to instance state so that there is no interference. But you can't pass a reference to a simple int, for total_turns_taken, hence you turned it into the only mutable Integer type: AtomicInteger. I'm okay with this final form of the change. othervm would have been simpler :) These are ugly tests even with your changes. BTW: 107 Thread.yield(); 108 Thread.sleep(sleepTime); The yield() before the sleep is completely pointless. David - I am not sure I understand your second suggestion here, sum up thread_turns of each Context(This is a fixed value) doesn't equal total_turns_taken. Regards Tristan On 01/09/2014 06:28 PM, Paul Sandoz wrote: On Jan 9, 2014, at 10:52 AM, Tristan Yan tristan@oracle.com wrote: Can someone else give a second review on this. In a comment the bug you state: here total_turns_taken is a static variable, it could affect by other tests I don't quite know under what test circumstances that can happen, but if so is the following also an issue: 52 private final static TurnChecker turn = new TurnChecker(-1); ? FWIW an alternative to using an AtomicInteger would be for the main loop to sum up thread_turns of each Context, since read/writes are all performed in a synchronized block. Paul.
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
On 9/01/2014 9:27 PM, David Holmes wrote: On 9/01/2014 9:07 PM, Tristan Yan wrote: Thank you Paul I change turn to local variable as well. http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.03/ http://cr.openjdk.java.net/%7Etyan/JDK-7027502/webrev.03/ Okay I think I get it now. Both MonitorTest and WaitersTest use the Context class, so if both tests run in the same VM the second test will see the static total_turns_taken and turn in the state they were left from the first test - hence the second test will always fail. The bug report suggests making the tests othervm to avoid the problem but instead you have changed from using static state to instance state so that there is no interference. But you can't pass a reference to a simple int, for total_turns_taken, hence you turned it into the only mutable Integer type: AtomicInteger. I'm okay with this final form of the change. othervm would have been simpler :) These are ugly tests even with your changes. BTW: 107 Thread.yield(); 108 Thread.sleep(sleepTime); The yield() before the sleep is completely pointless. Ditto for: 126 Thread.yield(); 127 Thread.sleep((long)default_contention_sleep); David - David - I am not sure I understand your second suggestion here, sum up thread_turns of each Context(This is a fixed value) doesn't equal total_turns_taken. Regards Tristan On 01/09/2014 06:28 PM, Paul Sandoz wrote: On Jan 9, 2014, at 10:52 AM, Tristan Yan tristan@oracle.com wrote: Can someone else give a second review on this. In a comment the bug you state: here total_turns_taken is a static variable, it could affect by other tests I don't quite know under what test circumstances that can happen, but if so is the following also an issue: 52 private final static TurnChecker turn = new TurnChecker(-1); ? FWIW an alternative to using an AtomicInteger would be for the main loop to sum up thread_turns of each Context, since read/writes are all performed in a synchronized block. Paul.
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
Thanks David I removed those pointless yield, is it okay you can sponsor this for me? http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.04/ http://cr.openjdk.java.net/%7Etyan/JDK-7027502/webrev.04/ Regards Tristan On 01/09/2014 07:34 PM, David Holmes wrote: On 9/01/2014 9:27 PM, David Holmes wrote: On 9/01/2014 9:07 PM, Tristan Yan wrote: Thank you Paul I change turn to local variable as well. http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.03/ http://cr.openjdk.java.net/%7Etyan/JDK-7027502/webrev.03/ Okay I think I get it now. Both MonitorTest and WaitersTest use the Context class, so if both tests run in the same VM the second test will see the static total_turns_taken and turn in the state they were left from the first test - hence the second test will always fail. The bug report suggests making the tests othervm to avoid the problem but instead you have changed from using static state to instance state so that there is no interference. But you can't pass a reference to a simple int, for total_turns_taken, hence you turned it into the only mutable Integer type: AtomicInteger. I'm okay with this final form of the change. othervm would have been simpler :) These are ugly tests even with your changes. BTW: 107 Thread.yield(); 108 Thread.sleep(sleepTime); The yield() before the sleep is completely pointless. Ditto for: 126 Thread.yield(); 127 Thread.sleep((long)default_contention_sleep); David - David - I am not sure I understand your second suggestion here, sum up thread_turns of each Context(This is a fixed value) doesn't equal total_turns_taken. Regards Tristan On 01/09/2014 06:28 PM, Paul Sandoz wrote: On Jan 9, 2014, at 10:52 AM, Tristan Yan tristan@oracle.com wrote: Can someone else give a second review on this. In a comment the bug you state: here total_turns_taken is a static variable, it could affect by other tests I don't quite know under what test circumstances that can happen, but if so is the following also an issue: 52 private final static TurnChecker turn = new TurnChecker(-1); ? FWIW an alternative to using an AtomicInteger would be for the main loop to sum up thread_turns of each Context, since read/writes are all performed in a synchronized block. Paul.
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
On 09/01/2014 11:27, David Holmes wrote: Okay I think I get it now. Both MonitorTest and WaitersTest use the Context class, so if both tests run in the same VM the second test will see the static total_turns_taken and turn in the state they were left from the first test - hence the second test will always fail. The bug report suggests making the tests othervm to avoid the problem but instead you have changed from using static state to instance state so that there is no interference. I haven't been following this one closely but I thought that jtreg created a class loader for each test (irrespective of mode) so I wouldn't expect statics to be an issue. -Alan.
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
On 9/01/2014 10:14 PM, Alan Bateman wrote: On 09/01/2014 11:27, David Holmes wrote: Okay I think I get it now. Both MonitorTest and WaitersTest use the Context class, so if both tests run in the same VM the second test will see the static total_turns_taken and turn in the state they were left from the first test - hence the second test will always fail. The bug report suggests making the tests othervm to avoid the problem but instead you have changed from using static state to instance state so that there is no interference. I haven't been following this one closely but I thought that jtreg created a class loader for each test (irrespective of mode) so I wouldn't expect statics to be an issue. That aside DemoRun forks off its own JVM to run a given test anyway! So I don't understand how the proposed fixes could actually be addressing the hangs that are occurring. Even if the statics were being shared I don't see how that leads to the failure mode in the bug report. David -Alan.
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
Hi David I wasn't able to reproduce this failure either in local or in our same binaries running(This is a continuous running with same JDK binaries). So intention for this code change is bringing this test back; add some debug info and try to avoid possible issues in this test. I agree this code change won't solve the failure happened. But this test was put into ProblemList two years ago better move for this is move out it from ProblemList and trace down the issue in our normal nightly. Thank you Tristan On 01/10/2014 06:35 AM, David Holmes wrote: On 9/01/2014 10:14 PM, Alan Bateman wrote: On 09/01/2014 11:27, David Holmes wrote: Okay I think I get it now. Both MonitorTest and WaitersTest use the Context class, so if both tests run in the same VM the second test will see the static total_turns_taken and turn in the state they were left from the first test - hence the second test will always fail. The bug report suggests making the tests othervm to avoid the problem but instead you have changed from using static state to instance state so that there is no interference. I haven't been following this one closely but I thought that jtreg created a class loader for each test (irrespective of mode) so I wouldn't expect statics to be an issue. That aside DemoRun forks off its own JVM to run a given test anyway! So I don't understand how the proposed fixes could actually be addressing the hangs that are occurring. Even if the statics were being shared I don't see how that leads to the failure mode in the bug report. David -Alan.
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
Thank you, David
I fixed copyright and change back sleep.
println was intended to be left in. This test was failed with timeout,
printf could help us to detect the value of total_turns_taken and
expected_turns_taken.
Please review it again
http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.01/
Regards
Tristan
On 01/07/2014 03:10 PM, David Holmes wrote:
Hi Tristan,
On 7/01/2014 4:43 PM, Tristan Yan wrote:
Hi All
Please help to review the code change for JDK-7027502.
http://cr.openjdk.java.net/~tyan/JDK-7027502/
Description:
This test was failed in JPRT test but recently we test with same
binaries run, it doesn't fail any more. The intention for this code
change is bringing this test back to normal test and make this test
robust and more informative. Change includes
1. Remove this test from ProblemList.
2. Change static member total_turns_taken into a local variable.
Please let me know your comment on this.
2 * Copyright (c) 2004,2014 Oracle and/or its affiliates. All
rights reserved.
Correct copyright format should have a space before 2014 and a comma
after:
* Copyright (c) 2004, 2014, Oracle and/or its affiliates. All rights
reserved.
---
Was this println intended to be left in?
114 System.out.println(total_turns_taken = +
total_turns_taken +
115 ;expected_turns_taken = +
expected_turns_taken);
116 if ( total_turns_taken.get() ==
expected_turns_taken ) {
You only want to read total_turns_taken once otherwise you may see
misleading print outs.
---
119 /* Create some monitor contention by sleeping with
lock */
120 if ( default_contention_sleep 0 ) {
121 System.out.println(Context sleeping, to
create contention);
122 try {
123 turn.wait((long) default_contention_sleep);
124 } catch (InterruptedException ignore) { }
125 }
By changing the Thread.sleep to turn.wait you no longer introduce any
contention as the wait() will release the monitor.
David
Thank you.
Tristan
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
On 7/01/2014 6:16 PM, Tristan Yan wrote:
Thank you, David
I fixed copyright and change back sleep.
println was intended to be left in. This test was failed with timeout,
printf could help us to detect the value of total_turns_taken and
expected_turns_taken.
Please review it again
http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.01/
Comma after 2014 still missing in copyright.
You need to read total_turns_taken.get() once and use that value in both
the println and the == test, so that you print the value you actually
tested.
David
Regards
Tristan
On 01/07/2014 03:10 PM, David Holmes wrote:
Hi Tristan,
On 7/01/2014 4:43 PM, Tristan Yan wrote:
Hi All
Please help to review the code change for JDK-7027502.
http://cr.openjdk.java.net/~tyan/JDK-7027502/
Description:
This test was failed in JPRT test but recently we test with same
binaries run, it doesn't fail any more. The intention for this code
change is bringing this test back to normal test and make this test
robust and more informative. Change includes
1. Remove this test from ProblemList.
2. Change static member total_turns_taken into a local variable.
Please let me know your comment on this.
2 * Copyright (c) 2004,2014 Oracle and/or its affiliates. All
rights reserved.
Correct copyright format should have a space before 2014 and a comma
after:
* Copyright (c) 2004, 2014, Oracle and/or its affiliates. All rights
reserved.
---
Was this println intended to be left in?
114 System.out.println(total_turns_taken = +
total_turns_taken +
115 ;expected_turns_taken = +
expected_turns_taken);
116 if ( total_turns_taken.get() ==
expected_turns_taken ) {
You only want to read total_turns_taken once otherwise you may see
misleading print outs.
---
119 /* Create some monitor contention by sleeping with
lock */
120 if ( default_contention_sleep 0 ) {
121 System.out.println(Context sleeping, to
create contention);
122 try {
123 turn.wait((long) default_contention_sleep);
124 } catch (InterruptedException ignore) { }
125 }
By changing the Thread.sleep to turn.wait you no longer introduce any
contention as the wait() will release the monitor.
David
Thank you.
Tristan
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
Hi David
You're totally right. Sorry I ask you review it again.
http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.02/
Thank you very much.
Tristan
On 01/07/2014 05:18 PM, David Holmes wrote:
On 7/01/2014 6:16 PM, Tristan Yan wrote:
Thank you, David
I fixed copyright and change back sleep.
println was intended to be left in. This test was failed with timeout,
printf could help us to detect the value of total_turns_taken and
expected_turns_taken.
Please review it again
http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.01/
Comma after 2014 still missing in copyright.
You need to read total_turns_taken.get() once and use that value in
both the println and the == test, so that you print the value you
actually tested.
David
Regards
Tristan
On 01/07/2014 03:10 PM, David Holmes wrote:
Hi Tristan,
On 7/01/2014 4:43 PM, Tristan Yan wrote:
Hi All
Please help to review the code change for JDK-7027502.
http://cr.openjdk.java.net/~tyan/JDK-7027502/
Description:
This test was failed in JPRT test but recently we test with same
binaries run, it doesn't fail any more. The intention for this code
change is bringing this test back to normal test and make this test
robust and more informative. Change includes
1. Remove this test from ProblemList.
2. Change static member total_turns_taken into a local variable.
Please let me know your comment on this.
2 * Copyright (c) 2004,2014 Oracle and/or its affiliates. All
rights reserved.
Correct copyright format should have a space before 2014 and a comma
after:
* Copyright (c) 2004, 2014, Oracle and/or its affiliates. All rights
reserved.
---
Was this println intended to be left in?
114 System.out.println(total_turns_taken = +
total_turns_taken +
115 ;expected_turns_taken = +
expected_turns_taken);
116 if ( total_turns_taken.get() ==
expected_turns_taken ) {
You only want to read total_turns_taken once otherwise you may see
misleading print outs.
---
119 /* Create some monitor contention by sleeping with
lock */
120 if ( default_contention_sleep 0 ) {
121 System.out.println(Context sleeping, to
create contention);
122 try {
123 turn.wait((long)
default_contention_sleep);
124 } catch (InterruptedException ignore) { }
125 }
By changing the Thread.sleep to turn.wait you no longer introduce any
contention as the wait() will release the monitor.
David
Thank you.
Tristan
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
On 7/01/2014 8:36 PM, Tristan Yan wrote:
Hi David
You're totally right. Sorry I ask you review it again.
http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.02/
Looks good now.
Thanks,
David
Thank you very much.
Tristan
On 01/07/2014 05:18 PM, David Holmes wrote:
On 7/01/2014 6:16 PM, Tristan Yan wrote:
Thank you, David
I fixed copyright and change back sleep.
println was intended to be left in. This test was failed with timeout,
printf could help us to detect the value of total_turns_taken and
expected_turns_taken.
Please review it again
http://cr.openjdk.java.net/~tyan/JDK-7027502/webrev.01/
Comma after 2014 still missing in copyright.
You need to read total_turns_taken.get() once and use that value in
both the println and the == test, so that you print the value you
actually tested.
David
Regards
Tristan
On 01/07/2014 03:10 PM, David Holmes wrote:
Hi Tristan,
On 7/01/2014 4:43 PM, Tristan Yan wrote:
Hi All
Please help to review the code change for JDK-7027502.
http://cr.openjdk.java.net/~tyan/JDK-7027502/
Description:
This test was failed in JPRT test but recently we test with same
binaries run, it doesn't fail any more. The intention for this code
change is bringing this test back to normal test and make this test
robust and more informative. Change includes
1. Remove this test from ProblemList.
2. Change static member total_turns_taken into a local variable.
Please let me know your comment on this.
2 * Copyright (c) 2004,2014 Oracle and/or its affiliates. All
rights reserved.
Correct copyright format should have a space before 2014 and a comma
after:
* Copyright (c) 2004, 2014, Oracle and/or its affiliates. All rights
reserved.
---
Was this println intended to be left in?
114 System.out.println(total_turns_taken = +
total_turns_taken +
115 ;expected_turns_taken = +
expected_turns_taken);
116 if ( total_turns_taken.get() ==
expected_turns_taken ) {
You only want to read total_turns_taken once otherwise you may see
misleading print outs.
---
119 /* Create some monitor contention by sleeping with
lock */
120 if ( default_contention_sleep 0 ) {
121 System.out.println(Context sleeping, to
create contention);
122 try {
123 turn.wait((long)
default_contention_sleep);
124 } catch (InterruptedException ignore) { }
125 }
By changing the Thread.sleep to turn.wait you no longer introduce any
contention as the wait() will release the monitor.
David
Thank you.
Tristan
RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
Hi All Please help to review the code change for JDK-7027502. http://cr.openjdk.java.net/~tyan/JDK-7027502/ Description: This test was failed in JPRT test but recently we test with same binaries run, it doesn't fail any more. The intention for this code change is bringing this test back to normal test and make this test robust and more informative. Change includes 1. Remove this test from ProblemList. 2. Change static member total_turns_taken into a local variable. Please let me know your comment on this. Thank you. Tristan
Re: RFR for JDK-7027502: Test failures in demo/jvmti/hprof testcases, need to be othervm
Hi Tristan,
On 7/01/2014 4:43 PM, Tristan Yan wrote:
Hi All
Please help to review the code change for JDK-7027502.
http://cr.openjdk.java.net/~tyan/JDK-7027502/
Description:
This test was failed in JPRT test but recently we test with same
binaries run, it doesn't fail any more. The intention for this code
change is bringing this test back to normal test and make this test
robust and more informative. Change includes
1. Remove this test from ProblemList.
2. Change static member total_turns_taken into a local variable.
Please let me know your comment on this.
2 * Copyright (c) 2004,2014 Oracle and/or its affiliates. All
rights reserved.
Correct copyright format should have a space before 2014 and a comma after:
* Copyright (c) 2004, 2014, Oracle and/or its affiliates. All rights
reserved.
---
Was this println intended to be left in?
114 System.out.println(total_turns_taken = +
total_turns_taken +
115 ;expected_turns_taken = +
expected_turns_taken);
116 if ( total_turns_taken.get() ==
expected_turns_taken ) {
You only want to read total_turns_taken once otherwise you may see
misleading print outs.
---
119 /* Create some monitor contention by sleeping with
lock */
120 if ( default_contention_sleep 0 ) {
121 System.out.println(Context sleeping, to
create contention);
122 try {
123 turn.wait((long) default_contention_sleep);
124 } catch (InterruptedException ignore) { }
125 }
By changing the Thread.sleep to turn.wait you no longer introduce any
contention as the wait() will release the monitor.
David
Thank you.
Tristan
