Review Request for CR : 7144861 RMI activation tests are too slow
olivier.lagneau at oracle.com
Mon May 7 16:30:58 UTC 2012
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
>>> 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
>> 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
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. :-(
As I said in the request mail. I just did not want to fix the
I have noticed many places where the code is not clean. In an ideal
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
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).
More information about the core-libs-dev