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

[Olivier Lagneau]

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.