Review Request for CR : 7144861 RMI activation tests are too slow (original) (raw)

Olivier Lagneau olivier.lagneau at oracle.com
Mon May 7 16:30:58 UTC 2012

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.

More information about the core-libs-dev mailing list