Review Request for CR : 7144861 RMI activation tests are too slow
stuart.marks at oracle.com
Sat May 5 01:50:56 UTC 2012
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:
> 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.
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.
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.
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.
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. :-(
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.
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
In destroy() I see that the maxTrials variable here really is the maximum
number of trials, which is good. :-)
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.
* * *
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! :-)
More information about the core-libs-dev