Hi Tristan,
(Sorry, for the delay; I'm traveling)
I think the current package (bench.rmi and bench.serial) organization makes good
sense, and I don't want to change it. The need to shuffle things around to
different packages indicates a factoring error. In this case the culprit is
TestLibrary, which is in the unnamed package.
The constraints are:
(1) bench.rmi.* benchmarks depend on Main
(2) Main depends on TestLibrary
From (1), Main wants to be in bench.rmi. But (2) wants Main to be in the
unnamed package, since it can't get at TestLibrary otherwise.
Eventually we want to move TestLibrary into a named package. (This is
JDK-8003358). But this involves updating almost all of the RMI tests, so we
probably don't want to do that as part of this test fix.
The alternative is to put in a short-term fix (i.e., a hack :-)) to deal with
one of the dependencies. Since TestLibrary is causing the problem, which will
eventually be fixed, it makes sense to quick-fix the way Main depends on
TestLibrary. This would be cleaned up when TestLibrary gets moved into a named
package.
A class in a named package cannot import classes from the unnamed package, but
it can load them. So, one could leave the benchmarks and Main in bench.rmi, and
use a few lines of reflection code to load TestLibrary and to call
getUnusedRandomPort() on it. There's only one call to
TestLibrary.getUnusedRandomPort() from Main, so a few lines of reflective code
could be added right there.
So, that's my recommendation. If you do this you can leave all of the benchmarks
and the Main class in bench.rmi and avoid all the file moves.
Thanks,
s'marks
On 11/10/13 4: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 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
