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

[Olivier Lagneau]

Thanks david for reviewing too.

Please see comments inlined.

David Holmes said  on date 5/6/2012 2:30 PM:
> 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.
Agreed. I think we should just ignore these interrupts since I don't 
think any rmi test of code in framework
might interrupts threads running that code. So thin we never need to 
re-assert.
>
>> 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.
Ok. I will go for volatile field however.

Thanks,
Olivier.
>
> 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