/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 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
/
/
/
/
/
