Review Request for CR : 7144861 RMI activation tests are too slow

[Stuart Marks]

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.

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.

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. :-(

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.

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.

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! :-)

Thanks.

s'marks