Hi Stuart,
Thanks for reviewing. Please see comments inlined.
Stuart Marks said on date 5/5/2012 3:50 AM:
On 5/3/12 10:53 AM, Alan Bateman wrote:
On 03/05/2012 14:09, Olivier Lagneau wrote:
Please review this fix for making the jdk regression tests for rmi
activation
run faster.
It brings a near to x2 speed factor on a Solaris/SPARC machine.
The webrev for the fix is here:
http://cr.openjdk.java.net/~olagneau/7144861/webrev.00/
It's great to see patches like this, thank you! The RMI tests have
always been
really slow, the activation tests in particular so I'm looking
forward to
seeing this patch go in. I'm also looking forward to trying it out in
conjunction with Darryl Mocek's patch as the combined patches should
mean we
get much better overall through-put.
I think Stuart is going to do the detailed review and help you get
this over
the finish line. I've just scanned the webrev (I didn't do a detailed
review)
and I didn't see anything obviously wrong, looks like the changes are
in the
right places.
Yes, good stuff. The tests don't get nearly enough maintenance, and a
2x speedup just by managing timeouts better is great.
A few relatively minor comments.
ActivationLibrary.java --
If we catch an InterruptedException while sleeping, we reassert the
interrupt bit by interrupting the current thread. This is usually good
practice. In this case though we go around the loop again, retry, and
sleep again. But if we call sleep() when the interrupt bit is set, it
returns immediately. As it stands, after being interrupted, this code
will retry a bunch of times effectively without sleeping and will then
return false.
I guess if we're going to go to the trouble of reasserting the
interrupt bit we might as well return false immediately.
Guess you are mentioning rmidRunning() method. You are right,
reasserting the interrupt should not be done. My mistake.
But I think we should just ignore the InterruptedException and just move
on next loop,
I could not find any other thread that may interrupt that one, neither
in the testlibrary, nor in all rmi tests.
JavaVM.java --
The started() method is synchronized because it needs to set shared
state (the started field) from another thread. However, code here uses
the started field without any synchronization. Since the state is just
a boolean probably the easiest thing to do is to is to make it
volatile, in which case the started() method doesn't need to be
synchronized.
Right. I will just use a volatile instead. Cleaner and simpler.
The maxTrials variable isn't actually the maximum number of trials,
it's the current number of trials. A small thing but it would be good
if it were renamed.
Sorry for that unnoticed misnaming. will fix it.
Urgh. The addOptions/addArguments methods concatenate all the
arguments and options into a single string, and then the start()
method tokenizes them out again into an array of strings. Heaven
forbid somebody pass a filename with a space. I don't expect you to
fix this. I just had to say something. :-(
+1 ;-)
As I said in the request mail. I just did not want to fix the
rmi+testlibrary code.
I have noticed many places where the code is not clean. In an ideal
world, an
overall cleanup of all this code would be needed.
I did not fix the bad things I found because I did not want to mix the
fix with cleanup.
RMID.java --
In the start() method, as before, the thread is reasserting the
interrupt bit on itself when it's interrupted. If it does this it
should just return immediately, otherwise it will spin around the loop
without sleeping until waitTime gets to zero.
Agreed. In this case I just wanted to minimize changes and kept that
interrupt reassert.
For the same reason as before, I think we can just ignore it.
I observe that the timer loop in start() doesn't necessarily wait for
an accurate amount of time, as sleep() can return early or late. There
are better techniques for dealing with this, but it's not clear to me
it's worth rewriting the code. This is something to bear in mind when
writing more time-critical code, however.
Thanks for the remark.
Guess the problem is the same for all usage of sleep in loops.
In destroy() I see that the maxTrials variable here really is the
maximum number of trials, which is good. :-)
I am usually trying to avoid misnaming of identifier ;-)
The timing loop here suffers from similar problems as the above. I
mean, it mostly works, but it could probably be simplified by calling
vm.waitFor() and having a task interrupt that call after a timeout
period. But it's probably not worth rewriting at this point.
Agreed for the two.
* * *
So, overall I'd like to see the interrupt handling changed and the
started field made volatile and maxTrials renamed in JavaVM.java. The
other issues probably should be taken care of at some point at some
point in the future; I think we all want the benefits of faster tests
now! :-)
Ok. will fix all of asap.
Hope to have another webrev Wednesday morning pdt (public holiday tomorrow).
Thanks,
Olivier.
