答复: 答复: RFR for JDK-7190106 RMI benchmark fails intermittently because of use of fixed port
Tristan Yan
tristan.yan at oracle.com
Tue Dec 3 01:12:34 UTC 2013
Thank you Stuart, next time I will keep the last version.
I'm appreciated that you can sponsor this for me.
Thank you very much.
Tristan.
-----邮件原件-----
发件人: Stuart Marks
发送时间: Tuesday, December 03, 2013 9:11 AM
收件人: Tristan Yan
抄送: core-libs-dev at openjdk.java.net
主题: 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 at 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 at 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 nice organization that was there
>>>>>>>>> before is now spoiled. Is this rearrangement necessary in
>>>>>>>>> order to convert the scripts to Java? I would prefer the original arrangement be left in place.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ** The RMI benchmark Main.java file has a @run tag of the
>>>>>>>>> form,
>>>>>>>>>
>>>>>>>>> @run main/othervm/policy=policy.all/timeout=1800 -server
>>>>>>>>> Main -c config
>>>>>>>>>
>>>>>>>>> There is a subtle but serious problem here: the -server option
>>>>>>>>> is passed to the >>JVM<< and not as an argument to the main() method.
>>>>>>>>> The main() method gets neither a -server nor a -client
>>>>>>>>> argument, so its default "run mode" as defined by the
>>>>>>>>> benchmark itself is SAMEVM. This runs the client and server in
>>>>>>>>> the same JVM, which is different from the shell script, which
>>>>>>>>> ran separate client and server JVMs.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ** The logic to process the -server argument still expects to
>>>>>>>>> take a port, even though the port is assigned automatically.
>>>>>>>>> So the obvious fix to the above,
>>>>>>>>>
>>>>>>>>> @run main/othervm/policy=policy.all/timeout=1800 Main
>>>>>>>>> -server -c config
>>>>>>>>>
>>>>>>>>> doesn't work, since a port is missing. The logic to increment
>>>>>>>>> the argument index to collect the port argument should be removed.
>>>>>>>>> Also, the -server line should be restored to the usage
>>>>>>>>> message, but without the port argument.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ** After this is done, the client's command line is
>>>>>>>>> constructed improperly.
>>>>>>>>> The command line ends up looking something like this:
>>>>>>>>>
>>>>>>>>> java client -cp <classpath> Main client localhost:58583
>>>>>>>>> -c config
>>>>>>>>>
>>>>>>>>> The word "client" appears twice, but what's really required is
>>>>>>>>> "-client" to appear as an argument after Main.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ** The client is run using ProcessBuilder, which by default
>>>>>>>>> sends stdout and stderr to pipes to be read by the parent. But
>>>>>>>>> the parent never reads them, thus any messages from the client
>>>>>>>>> are never seen. The client is the one that emits the benchmark
>>>>>>>>> report, so its output needs to be seen. It might be sufficient
>>>>>>>>> to have the client inherit the parent's stdout and stderr.
>>>>>>>>> This might intermix the client's and server's output, but it's better than nothing.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ** The config file is checked with the following code:
>>>>>>>>>
>>>>>>>>> try {
>>>>>>>>> confFile = args[i];
>>>>>>>>> confstr = new FileInputStream(System.getProperty("test.src")
>>>>>>>>> + System.getProperty("file.separator") + confFile);
>>>>>>>>> } catch (IOException e) {
>>>>>>>>> die("Error: unable to open \"" + args[i] + "\"");
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> This is potentially misleading, as the message doesn't print
>>>>>>>>> the actual filename that was attempted to be opened.
>>>>>>>>>
>>>>>>>>> This is important, as the test.src property doesn't exist in
>>>>>>>>> the client JVM.
>>>>>>>>>
>>>>>>>>> Note that the original shell script passed full pathnames for
>>>>>>>>> the config file to both the client and the server. The new
>>>>>>>>> @run tag merely says "-c config" which redefines the config
>>>>>>>>> filename to be relative to the test.src directory. You could pass -Dtest.src=...
>>>>>>>>> to the client, but it seems like there should be something
>>>>>>>>> better than can be done.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ** The client needs to have its security policy set up. This
>>>>>>>>> is missing from the construction of the client's command line.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ** ProcessBuilder takes a List<String> for its command; there
>>>>>>>>> is no need to turn the list into an array.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ** In the benchmark main methods, code of the form,
>>>>>>>>>
>>>>>>>>> while (true) {
>>>>>>>>> runBenchmarks();
>>>>>>>>> if (exitRequested) {
>>>>>>>>> System.exit();
>>>>>>>>> }
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> was replaced with
>>>>>>>>>
>>>>>>>>> while (!exitRequested) {
>>>>>>>>> runBenchmarks();
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> This is a subtle logic change, in that the former code always
>>>>>>>>> executed the loop at least once. It seems unlikely, but it's
>>>>>>>>> possible that a timer could set exitRequested before loop
>>>>>>>>> entry, resulting in the benchmark running zero times. I guess,
>>>>>>>>> if you really want to clean this up (we do need to avoid
>>>>>>>>> System.exit in jtreg tests), use a do-while loop instead.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ** Don't forget to remove the 7190106/runRmiBench.sh entry
>>>>>>>>> from ProblemList.txt.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ** Remove the references to RMISecurityManager and just use
>>>>>>>>> SecurityManager. This is just general cleanup. (I deprecated
>>>>>>>>> RMISecurityManager last week.) :-)
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> It would be good if you could fix up these issues and post another webrev.
>>>>>>>>>
>>>>>>>>> Thanks.
>>>>>>>>>
>>>>>>>>> s'marks
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Thank you
>>>>>>>>>> Tristan
>>>>>>>>>>
>>>>>>>>>> On 01/11/2013 23:58, Stuart Marks wrote:
>>>>>>>>>>> On 10/31/13 10:22 PM, Tristan Yan wrote:
>>>>>>>>>>>> I am working on bug
>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-7190106. Based on
>>>>>>>>>>>> my research, it looks like the issue of fixed port was
>>>>>>>>>>>> already addressed by Stuart Marks in other RMI tests which
>>>>>>>>>>>> are Java based. I would like to reuse his solution, however
>>>>>>>>>>>> it does not work for shell based tests.
>>>>>>>>>>>
>>>>>>>>>>> (Darryl Mocek did the unique port work for the RMI tests.)
>>>>>>>>>>>
>>>>>>>>>>> Was the patch attached to your message? If so, it didn't get
>>>>>>>>>>> through. Most OpenJDK mailing lists strip off attachments
>>>>>>>>>>> before forwarding 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 function provide same functionality. Maybe
>>>>>>>>>>> we should think about to merge them.
>>>>>>>>>>> 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/webr
>>>>>>>>>>>>>>>> ev/
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> 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 nice organization that was there before is now spoiled.
>>>>>>>>>>>>>>> Is this rearrangement necessary in order to convert the
>>>>>>>>>>>>>>> scripts to Java? I would prefer the original arrangement
>>>>>>>>>>>>>>> be left in place.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> ** The RMI benchmark Main.java file has a @run tag of
>>>>>>>>>>>>>>> the form,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> @run main/othervm/policy=policy.all/timeout=1800
>>>>>>>>>>>>>>> -server Main -c config
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> There is a subtle but serious problem here: the -server
>>>>>>>>>>>>>>> option is passed to the >>JVM<< and not as an argument
>>>>>>>>>>>>>>> to the
>>>>>>>>>>>>>>> main() method.
>>>>>>>>>>>>>>> The main() method gets neither a -server nor a -client
>>>>>>>>>>>>>>> argument, so its default "run mode" as defined by the
>>>>>>>>>>>>>>> benchmark itself is SAMEVM.
>>>>>>>>>>>>>>> This runs the client and server in the same JVM, which
>>>>>>>>>>>>>>> is different from the shell script, which ran separate
>>>>>>>>>>>>>>> client and server JVMs.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> ** The logic to process the -server argument still
>>>>>>>>>>>>>>> expects to take a port, even though the port is assigned
>>>>>>>>>>>>>>> automatically. So the obvious fix to the above,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> @run main/othervm/policy=policy.all/timeout=1800
>>>>>>>>>>>>>>> Main -server -c config
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> doesn't work, since a port is missing. The logic to
>>>>>>>>>>>>>>> increment the argument index to collect the port
>>>>>>>>>>>>>>> argument should be removed. Also, the -server line
>>>>>>>>>>>>>>> should be restored to the usage message, but without the port argument.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> ** After this is done, the client's command line is
>>>>>>>>>>>>>>> constructed improperly. The command line ends up looking
>>>>>>>>>>>>>>> something like this:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> java client -cp <classpath> Main client
>>>>>>>>>>>>>>> localhost:58583 -c config
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> The word "client" appears twice, but what's really
>>>>>>>>>>>>>>> required is "-client" to appear as an argument after Main.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> ** The client is run using ProcessBuilder, which by
>>>>>>>>>>>>>>> default sends stdout and stderr to pipes to be read by the parent.
>>>>>>>>>>>>>>> But the parent never reads them, thus any messages from
>>>>>>>>>>>>>>> the client are never seen.
>>>>>>>>>>>>>>> The client is the one that emits the benchmark report,
>>>>>>>>>>>>>>> so its output needs to be seen. It might be sufficient
>>>>>>>>>>>>>>> to have the client inherit the parent's stdout and
>>>>>>>>>>>>>>> stderr. This might intermix the client's and server's
>>>>>>>>>>>>>>> output, but it's better than nothing.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> ** The config file is checked with the following code:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> try {
>>>>>>>>>>>>>>> confFile = args[i];
>>>>>>>>>>>>>>> confstr = new
>>>>>>>>>>>>>>> FileInputStream(System.getProperty("test.src")
>>>>>>>>>>>>>>> + System.getProperty("file.separator") + confFile);
>>>>>>>>>>>>>>> } catch (IOException e) {
>>>>>>>>>>>>>>> die("Error: unable to open \"" + args[i] + "\"");
>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> This is potentially misleading, as the message doesn't
>>>>>>>>>>>>>>> print the actual filename that was attempted to be opened.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> This is important, as the test.src property doesn't
>>>>>>>>>>>>>>> exist in the client JVM.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Note that the original shell script passed full
>>>>>>>>>>>>>>> pathnames for the config file to both the client and the
>>>>>>>>>>>>>>> server. The new @run tag merely says "-c config" which
>>>>>>>>>>>>>>> redefines the config filename to be relative to the test.src directory.
>>>>>>>>>>>>>>> You could pass -Dtest.src=... to the client, but it
>>>>>>>>>>>>>>> seems like there should be something better than can be done.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> ** The client needs to have its security policy set up.
>>>>>>>>>>>>>>> This is missing from the construction of the client's command line.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> ** ProcessBuilder takes a List<String> for its command;
>>>>>>>>>>>>>>> there is no need to turn the list into an array.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> ** In the benchmark main methods, code of the form,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> while (true) {
>>>>>>>>>>>>>>> runBenchmarks();
>>>>>>>>>>>>>>> if (exitRequested) {
>>>>>>>>>>>>>>> System.exit();
>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> was replaced with
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> while (!exitRequested) {
>>>>>>>>>>>>>>> runBenchmarks();
>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> This is a subtle logic change, in that the former code
>>>>>>>>>>>>>>> always executed the loop at least once. It seems
>>>>>>>>>>>>>>> unlikely, but it's possible that a timer could set
>>>>>>>>>>>>>>> exitRequested before loop entry, resulting in the
>>>>>>>>>>>>>>> benchmark running zero times. I guess, if you really
>>>>>>>>>>>>>>> want to clean this up (we do need to avoid System.exit
>>>>>>>>>>>>>>> in jtreg tests), use a do-while loop instead.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> ** Don't forget to remove the 7190106/runRmiBench.sh
>>>>>>>>>>>>>>> entry from ProblemList.txt.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> ** Remove the references to RMISecurityManager and just
>>>>>>>>>>>>>>> use SecurityManager. This is just general cleanup. (I
>>>>>>>>>>>>>>> deprecated RMISecurityManager last week.) :-)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> It would be good if you could fix up these issues and
>>>>>>>>>>>>>>> post another webrev.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> s'marks
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thank you
>>>>>>>>>>>>>>>> Tristan
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 01/11/2013 23:58, Stuart Marks wrote:
>>>>>>>>>>>>>>>>> On 10/31/13 10:22 PM, Tristan Yan wrote:
>>>>>>>>>>>>>>>>>> I am working on bug
>>>>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-7190106.
>>>>>>>>>>>>>>>>>> Based on my research, it looks like the issue of
>>>>>>>>>>>>>>>>>> fixed port was already addressed by Stuart Marks in
>>>>>>>>>>>>>>>>>> other RMI tests which are Java based. I would like to
>>>>>>>>>>>>>>>>>> reuse his solution, however it does not work for shell based tests.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> (Darryl Mocek did the unique port work for the RMI
>>>>>>>>>>>>>>>>> tests.)
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Was the patch attached to your message? If so, it
>>>>>>>>>>>>>>>>> didn't get through. Most OpenJDK mailing lists strip
>>>>>>>>>>>>>>>>> off attachments before forwarding the message to the
>>>>>>>>>>>>>>>>> recipients.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> 2. My recommendation would be to convert this shell
>>>>>>>>>>>>>>>>>> script test into Java based test and re-use the
>>>>>>>>>>>>>>>>>> dynamic port allocation solution by Stuart Marks to
>>>>>>>>>>>>>>>>>> address the issue
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> 3. Also this test was written with server/client mode
>>>>>>>>>>>>>>>>>> in shell script. In the past there have been sync
>>>>>>>>>>>>>>>>>> issues between server/client which caused the test to
>>>>>>>>>>>>>>>>>> fail. If we convert the shell script into Java based
>>>>>>>>>>>>>>>>>> test, it would avoid using "sleep 10" mechanism to
>>>>>>>>>>>>>>>>>> allow for server and client to start up and also give
>>>>>>>>>>>>>>>>>> us better control in synchronizing server and client.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> (Background for interested readers.) In general, yes,
>>>>>>>>>>>>>>>>> it's quite difficult to make reliable shell tests,
>>>>>>>>>>>>>>>>> especially for multi-process tests like this one.
>>>>>>>>>>>>>>>>> There is the unique port issue, and there is also the
>>>>>>>>>>>>>>>>> issue of how long for the client to wait until the
>>>>>>>>>>>>>>>>> server is ready. Error handling is also a problem, for
>>>>>>>>>>>>>>>>> example, if one of the JVMs gets an unexpected
>>>>>>>>>>>>>>>>> exception, it's easy for shell tests to mishandle this
>>>>>>>>>>>>>>>>> case. They might hang or erroneously report success.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> --
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> If this is a rewrite, it's probably fairly large, so
>>>>>>>>>>>>>>>>> you need to upload it somewhere (e.g.,
>>>>>>>>>>>>>>>>> cr.openjdk.java.net) and then post a link to it.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> s'marks
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> the message to
>>>>>>>>>>> the recipients.
>>>>>>>>>>>
>>>>>>>>>>>> 2. My recommendation would be to convert this shell script
>>>>>>>>>>>> test into Java based test and re-use the dynamic port
>>>>>>>>>>>> allocation solution by Stuart Marks to address the issue
>>>>>>>>>>>>
>>>>>>>>>>>> 3. Also this test was written with server/client mode in shell script.
>>>>>>>>>>>> In the
>>>>>>>>>>>> past there have been sync issues between server/client
>>>>>>>>>>>> which caused the test to fail. If we convert the shell
>>>>>>>>>>>> script into Java based test, it would avoid using "sleep
>>>>>>>>>>>> 10" mechanism to allow for server and client to start up
>>>>>>>>>>>> and also give us better control in synchronizing server and client.
>>>>>>>>>>>
>>>>>>>>>>> (Background for interested readers.) In general, yes, it's
>>>>>>>>>>> quite difficult to make reliable shell tests, especially for
>>>>>>>>>>> multi-process tests like this one.
>>>>>>>>>>> There is the unique port issue, and there is also the issue
>>>>>>>>>>> of how long for the client to wait until the server is
>>>>>>>>>>> ready. Error handling is also a problem, for example, if one
>>>>>>>>>>> of the JVMs gets an unexpected exception, it's easy for
>>>>>>>>>>> shell tests to mishandle this case. They might hang or erroneously report success.
>>>>>>>>>>>
>>>>>>>>>>> --
>>>>>>>>>>>
>>>>>>>>>>> If this is a rewrite, it's probably fairly large, so you
>>>>>>>>>>> need to upload it somewhere (e.g., cr.openjdk.java.net) and
>>>>>>>>>>> then post a link to it.
>>>>>>>>>>>
>>>>>>>>>>> Thanks.
>>>>>>>>>>>
>>>>>>>>>>> s'marks
>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>
More information about the core-libs-dev
mailing list