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

[David Holmes]

On 5/05/2012 11:50 AM, Stuart Marks 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/
> 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.

Does anything in this testing framework actually interrupt any threads? 
Elsewhere in this test the interrupt, should it be possible, just skips 
to the next loop iteration without being re-asserted. So either we need 
to re-assert always or never - and I suspect never.

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

And naming wise setStarted() would be better than started() IMHO.

David
-----


> 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