Re: 答复: RFR for JDK-7190106 RMI benchmark fails intermittently because of use of fixed port
Hi Tristan, Sorry (again^2) for the delay. Holiday weekend here in the U.S. The latest webrev looks fine. Did you need a sponsor for this? I can push it for you. I guess we also need an official Reviewer before I can push it. (One additional point for the future. If there is more than one round of review, it's useful to post the new webrev alongside the old one, instead of overwriting it, so that old links are still valid. It's not likely to happen in this case, but if someone were to read the email archives and follow the link to an earlier webrev, they'd have no idea what I was talking about.) s'marks On 11/25/13 9:12 PM, Tristan Yan wrote: Hi Stuart Thanks for your code review. I updated the webrev according your suggestion. Could you review it again? http://cr.openjdk.java.net/~ewang/tristan/JDK-7190106/webrev.00/ Tristan On 11/26/2013 10:36 AM, Stuart Marks wrote: Hi Tristan, Finally getting back to this. Again, sorry for the delay. The changes look much better now. I've listed a bunch of items below, but they're all comparatively minor, even nitpicky. But there are a few things that should be cleaned up before we integrate this. Items listed below. ** bench/serial/Main.java - The description should simply be The Serialization benchmark test. This test has nothing to do with RMI, even though it's under the java/rmi hierarchy in the filesystem. - parseArgs() uses strings-in-switch! Good, but put break on its own line instead of following the close-brace of the catch clause. (rmi/Main.java doesn't have this issue.) ** bench/rmi/Main.java - Imports of java.util.logging.Level and Logger are unused? - Missing -server line from usage(). - Interesting use of enum. Note by convention an enum is like a class and names are in mixed case, thus use OutputFormat instead of OUTPUT_FORMAT. Also note that the enum doesn't buy much, at least in terms of lines of code, since the enum declaration and enum instance overrides add about as much code as the case statement that got removed from setupReporter(). It does buy a bit of type-safety, though, so might as well leave it in. - Enum instance HTML should be on a new line, like XML. - Reflection code can be chained instead of declaring several locals. This is mainly a matter of taste, but to my eye it's cleaner. The main advantage is avoiding the need to come up with names for intermediate locals. For example: port = (int) Class.forName(TestLibrary) .getMethod(getUnusedRandomPort) .invoke(null); - Catch clause at 389 can be of ReflectiveOperationException. This covers everything except IllegalArgumentException, which is unchecked, so you don't need to catch it. (Not sure why Method.invoke is declared to throw IllegalArgumentException, since generally only checked exceptions are declared in the throws clause.) - Line 416, no need to mention RMISecurityManager in a comment, just make the change to use SecurityManager. - It's kind of surprising that TEST_SRC_PATH appends the file separator to the test.src property. At line 241 test.src has to be fetched again to use it without the file separator. - Line 234, instead of the java.home property, use the test.jdk property. This will use the JDK under test instead of the JDK that's running jtreg. In practice it's unclear whether this makes any actual difference today, but it's good to try to keep this separation. Also, use file separators here instead of appending /bin/java. (Hmmm. I observe that the RMI testlibrary invokes JVM subprocesses using java.home.) Thanks, s'marks On 11/20/13 1:49 PM, Stuart Marks wrote: Hi, sorry about the delay, I'm still backlogged from traveling. I'll get to this soon. s'marks On 11/19/13 6:27 PM, Tristan Yan wrote: Hi Stuart Did you get chance to review it again. Let me know if you have any new comments or suggestions. Thanks Tristan -邮件原件- 发件人: Tristan Yan 发送时间: Thursday, November 14, 2013 11:09 PM 收件人: Stuart Marks 抄送: core-libs-dev@openjdk.java.net 主题: 答复: RFR for JDK-7190106 RMI benchmark fails intermittently because of use of fixed port Thank you Stuart It took me a little time to correct the code. sorry be late. This is new webrev for the code change. Please help to review it again. Description: 1. Convert shell script test to Java program test. 2. Using random server port by reusing Darryl Mocek's work(TestLibrary.getUnusedRandomPort) to replace fixed server port. 3. Because TestLibrary doesn't have package. Here is using reflection to call TestLibrary.getUnusedRandomPort. This is going to change when TestLibrary moves to named package. 4. Using java Process class to start client process. Client and server are sharing IO. 5. Also convert other shell script test runSerialBench.sh to java program test also. 6. ProblemList has been changed to get back java/rmi/reliability/benchmark/runRmiBench.sh test. http://cr.openjdk.java.net/~pzhang/Tristan/7190106/webrev/
Re: 答复: RFR for JDK-7190106 RMI benchmark fails intermittently because of use of fixed port
Hi Tristan, Finally getting back to this. Again, sorry for the delay. The changes look much better now. I've listed a bunch of items below, but they're all comparatively minor, even nitpicky. But there are a few things that should be cleaned up before we integrate this. Items listed below. ** bench/serial/Main.java - The description should simply be The Serialization benchmark test. This test has nothing to do with RMI, even though it's under the java/rmi hierarchy in the filesystem. - parseArgs() uses strings-in-switch! Good, but put break on its own line instead of following the close-brace of the catch clause. (rmi/Main.java doesn't have this issue.) ** bench/rmi/Main.java - Imports of java.util.logging.Level and Logger are unused? - Missing -server line from usage(). - Interesting use of enum. Note by convention an enum is like a class and names are in mixed case, thus use OutputFormat instead of OUTPUT_FORMAT. Also note that the enum doesn't buy much, at least in terms of lines of code, since the enum declaration and enum instance overrides add about as much code as the case statement that got removed from setupReporter(). It does buy a bit of type-safety, though, so might as well leave it in. - Enum instance HTML should be on a new line, like XML. - Reflection code can be chained instead of declaring several locals. This is mainly a matter of taste, but to my eye it's cleaner. The main advantage is avoiding the need to come up with names for intermediate locals. For example: port = (int) Class.forName(TestLibrary) .getMethod(getUnusedRandomPort) .invoke(null); - Catch clause at 389 can be of ReflectiveOperationException. This covers everything except IllegalArgumentException, which is unchecked, so you don't need to catch it. (Not sure why Method.invoke is declared to throw IllegalArgumentException, since generally only checked exceptions are declared in the throws clause.) - Line 416, no need to mention RMISecurityManager in a comment, just make the change to use SecurityManager. - It's kind of surprising that TEST_SRC_PATH appends the file separator to the test.src property. At line 241 test.src has to be fetched again to use it without the file separator. - Line 234, instead of the java.home property, use the test.jdk property. This will use the JDK under test instead of the JDK that's running jtreg. In practice it's unclear whether this makes any actual difference today, but it's good to try to keep this separation. Also, use file separators here instead of appending /bin/java. (Hmmm. I observe that the RMI testlibrary invokes JVM subprocesses using java.home.) Thanks, s'marks On 11/20/13 1:49 PM, Stuart Marks wrote: Hi, sorry about the delay, I'm still backlogged from traveling. I'll get to this soon. s'marks On 11/19/13 6:27 PM, Tristan Yan wrote: Hi Stuart Did you get chance to review it again. Let me know if you have any new comments or suggestions. Thanks Tristan -邮件原件- 发件人: Tristan Yan 发送时间: Thursday, November 14, 2013 11:09 PM 收件人: Stuart Marks 抄送: core-libs-dev@openjdk.java.net 主题: 答复: RFR for JDK-7190106 RMI benchmark fails intermittently because of use of fixed port Thank you Stuart It took me a little time to correct the code. sorry be late. This is new webrev for the code change. Please help to review it again. Description: 1. Convert shell script test to Java program test. 2. Using random server port by reusing Darryl Mocek's work(TestLibrary.getUnusedRandomPort) to replace fixed server port. 3. Because TestLibrary doesn't have package. Here is using reflection to call TestLibrary.getUnusedRandomPort. This is going to change when TestLibrary moves to named package. 4. Using java Process class to start client process. Client and server are sharing IO. 5. Also convert other shell script test runSerialBench.sh to java program test also. 6. ProblemList has been changed to get back java/rmi/reliability/benchmark/runRmiBench.sh test. http://cr.openjdk.java.net/~pzhang/Tristan/7190106/webrev/ Thank you so much Tristan -邮件原件- 发件人: Stuart Marks 发送时间: Tuesday, November 12, 2013 11:38 PM 收件人: Tristan Yan 抄送: core-libs-dev@openjdk.java.net; Alexandre (Shura) Iline 主题: Re: RFR for JDK-7190106 RMI benchmark fails intermittently because of use of fixed port Unfortunately we can't use jdk.testlibrary.Utils.getFreePort() for the RMI tests, since RMI's TestLibrary.getUnusedRandomPort() respects a reserved port range that's used by some RMI tests that have to used fixed ports. s'marks On 11/11/13 2:39 PM, Tristan Yan wrote: Hi Stuart Also there is one more solution, which is there is one jdk.testlibrary.Utils.getFreePort() method under test/lib. It's same function as TestLibrary.getUnusedRandomPort() and it has named package. Do you mind I use this one? Since these two functions provide same functionality. Maybe we should think about to
Re: 答复: RFR for JDK-7190106 RMI benchmark fails intermittently because of use of fixed port
Hi Stuart Thanks for your code review. I updated the webrev according your suggestion. Could you review it again? http://cr.openjdk.java.net/~ewang/tristan/JDK-7190106/webrev.00/ Tristan On 11/26/2013 10:36 AM, Stuart Marks wrote: Hi Tristan, Finally getting back to this. Again, sorry for the delay. The changes look much better now. I've listed a bunch of items below, but they're all comparatively minor, even nitpicky. But there are a few things that should be cleaned up before we integrate this. Items listed below. ** bench/serial/Main.java - The description should simply be The Serialization benchmark test. This test has nothing to do with RMI, even though it's under the java/rmi hierarchy in the filesystem. - parseArgs() uses strings-in-switch! Good, but put break on its own line instead of following the close-brace of the catch clause. (rmi/Main.java doesn't have this issue.) ** bench/rmi/Main.java - Imports of java.util.logging.Level and Logger are unused? - Missing -server line from usage(). - Interesting use of enum. Note by convention an enum is like a class and names are in mixed case, thus use OutputFormat instead of OUTPUT_FORMAT. Also note that the enum doesn't buy much, at least in terms of lines of code, since the enum declaration and enum instance overrides add about as much code as the case statement that got removed from setupReporter(). It does buy a bit of type-safety, though, so might as well leave it in. - Enum instance HTML should be on a new line, like XML. - Reflection code can be chained instead of declaring several locals. This is mainly a matter of taste, but to my eye it's cleaner. The main advantage is avoiding the need to come up with names for intermediate locals. For example: port = (int) Class.forName(TestLibrary) .getMethod(getUnusedRandomPort) .invoke(null); - Catch clause at 389 can be of ReflectiveOperationException. This covers everything except IllegalArgumentException, which is unchecked, so you don't need to catch it. (Not sure why Method.invoke is declared to throw IllegalArgumentException, since generally only checked exceptions are declared in the throws clause.) - Line 416, no need to mention RMISecurityManager in a comment, just make the change to use SecurityManager. - It's kind of surprising that TEST_SRC_PATH appends the file separator to the test.src property. At line 241 test.src has to be fetched again to use it without the file separator. - Line 234, instead of the java.home property, use the test.jdk property. This will use the JDK under test instead of the JDK that's running jtreg. In practice it's unclear whether this makes any actual difference today, but it's good to try to keep this separation. Also, use file separators here instead of appending /bin/java. (Hmmm. I observe that the RMI testlibrary invokes JVM subprocesses using java.home.) Thanks, s'marks On 11/20/13 1:49 PM, Stuart Marks wrote: Hi, sorry about the delay, I'm still backlogged from traveling. I'll get to this soon. s'marks On 11/19/13 6:27 PM, Tristan Yan wrote: Hi Stuart Did you get chance to review it again. Let me know if you have any new comments or suggestions. Thanks Tristan -邮件原件- 发件人: Tristan Yan 发送时间: Thursday, November 14, 2013 11:09 PM 收件人: Stuart Marks 抄送: core-libs-dev@openjdk.java.net 主题: 答复: RFR for JDK-7190106 RMI benchmark fails intermittently because of use of fixed port Thank you Stuart It took me a little time to correct the code. sorry be late. This is new webrev for the code change. Please help to review it again. Description: 1. Convert shell script test to Java program test. 2. Using random server port by reusing Darryl Mocek's work(TestLibrary.getUnusedRandomPort) to replace fixed server port. 3. Because TestLibrary doesn't have package. Here is using reflection to call TestLibrary.getUnusedRandomPort. This is going to change when TestLibrary moves to named package. 4. Using java Process class to start client process. Client and server are sharing IO. 5. Also convert other shell script test runSerialBench.sh to java program test also. 6. ProblemList has been changed to get back java/rmi/reliability/benchmark/runRmiBench.sh test. http://cr.openjdk.java.net/~pzhang/Tristan/7190106/webrev/ Thank you so much Tristan -邮件原件- 发件人: Stuart Marks 发送时间: Tuesday, November 12, 2013 11:38 PM 收件人: Tristan Yan 抄送: core-libs-dev@openjdk.java.net; Alexandre (Shura) Iline 主题: Re: RFR for JDK-7190106 RMI benchmark fails intermittently because of use of fixed port Unfortunately we can't use jdk.testlibrary.Utils.getFreePort() for the RMI tests, since RMI's TestLibrary.getUnusedRandomPort() respects a reserved port range that's used by some RMI tests that have to used fixed ports. s'marks On 11/11/13 2:39 PM, Tristan Yan wrote: Hi Stuart Also there is one more solution, which is there is one
Re: 答复: RFR for JDK-7190106 RMI benchmark fails intermittently because of use of fixed port
Hi, sorry about the delay, I'm still backlogged from traveling. I'll get to this soon. s'marks On 11/19/13 6:27 PM, Tristan Yan wrote: Hi Stuart Did you get chance to review it again. Let me know if you have any new comments or suggestions. Thanks Tristan -邮件原件- 发件人: Tristan Yan 发送时间: Thursday, November 14, 2013 11:09 PM 收件人: Stuart Marks 抄送: core-libs-dev@openjdk.java.net 主题: 答复: RFR for JDK-7190106 RMI benchmark fails intermittently because of use of fixed port Thank you Stuart It took me a little time to correct the code. sorry be late. This is new webrev for the code change. Please help to review it again. Description: 1. Convert shell script test to Java program test. 2. Using random server port by reusing Darryl Mocek's work(TestLibrary.getUnusedRandomPort) to replace fixed server port. 3. Because TestLibrary doesn't have package. Here is using reflection to call TestLibrary.getUnusedRandomPort. This is going to change when TestLibrary moves to named package. 4. Using java Process class to start client process. Client and server are sharing IO. 5. Also convert other shell script test runSerialBench.sh to java program test also. 6. ProblemList has been changed to get back java/rmi/reliability/benchmark/runRmiBench.sh test. http://cr.openjdk.java.net/~pzhang/Tristan/7190106/webrev/ Thank you so much Tristan -邮件原件- 发件人: Stuart Marks 发送时间: Tuesday, November 12, 2013 11:38 PM 收件人: Tristan Yan 抄送: core-libs-dev@openjdk.java.net; Alexandre (Shura) Iline 主题: Re: RFR for JDK-7190106 RMI benchmark fails intermittently because of use of fixed port Unfortunately we can't use jdk.testlibrary.Utils.getFreePort() for the RMI tests, since RMI's TestLibrary.getUnusedRandomPort() respects a reserved port range that's used by some RMI tests that have to used fixed ports. s'marks On 11/11/13 2:39 PM, Tristan Yan wrote: Hi Stuart Also there is one more solution, which is there is one jdk.testlibrary.Utils.getFreePort() method under test/lib. It's same function as TestLibrary.getUnusedRandomPort() and it has named package. Do you mind I use this one? Since these two functions provide same functionality. Maybe we should think about to merge them at the same time. Thank you Tristan On 11/10/2013 11:19 AM, Tristan Yan wrote: Hi Stuart I tried your suggestion but unfortunately all the benchmarks have dependencies to Main class because they need get stub from server. I suggest we move the benchmark tests to unnamed package unless we do want to put TestLibrary into a named package right now. Please let me know if you have objection on this. Thank you Tristan On 11/09/2013 02:28 AM, Stuart Marks wrote: Hi Tristan, Yes, it's kind of a problem that the RMI TestLibrary is in the unnamed package. Classes in a named package cannot import classes from the unnamed package. We've run into problems with this before. Eventually, we should move TestLibrary a named package. I think it's possible to work around this without too much difficulty. Note that classes in the unnamed package can import classes from named packages. So, perhaps you can put the RmiBench main class in the unnamed package so it has access to TestLibrary. Then have the benchmarks themselves in the bench.rmi package. The config file already references the benchmarks by fully qualified class name (e.g., bench.rmi.NullCalls) so with a bit of tinkering you ought to be able to get this to work. s'marks On 11/8/13 3:00 AM, Tristan Yan wrote: Thank you, Stuart There is one review point I want to ask you opinion. Which is the reason that I moved from test/java/rmi/reliability/benchmark/bench/rmi to test/java/rmi/reliability/benchmark is Main.java need access class TestLibrary for supporting random port. TestLibrary is a unpackage class, I couldn't find a way to let a class which has Package to access the class without package. Do you have suggestion on that? Thank you so much. Tristan On 11/06/2013 09:50 AM, Stuart Marks wrote: On 11/1/13 9:18 AM, Tristan Yan wrote: Hi Everyone http://cr.openjdk.java.net/~pzhang/Tristan/7190106/webrev/ Description: 1. Convert shell script test to Java program test. 2. Using random server port by reusing Darryl Mocek's work to replace fixed server port. 3. Using java Process class to start client process. 4. Also convert other shell script test runSerialBench.sh to java program test also Hi Tristan, Several comments on this webrev. ** The original arrangement within the test/java/rmi/reliability/benchmark directory had the main benchmark files (scripts) at the top, some benchmark framework files in the bench subdirectory, and the actual RMI and serialization benchmarks in bench/rmi and bench/serial subdirectories. The webrev moves all the RMI benchmarks to the top benchmark directory but leaves the serial benchmarks in bench/serial. The RMI benchmarks are now all cluttering the top directory, but the main serial benchmark test is now buried in the bench/serial directory. The
