Review Request for CR : 7144861 RMI activation tests are too slow
olivier.lagneau at oracle.com
Mon May 7 17:35:09 UTC 2012
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
>>>> run faster.
>>>> It brings a near to x2 speed factor on a Solaris/SPARC machine.
>>>> The webrev for the fix is here:
>> 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
>> 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.
>> 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! :-)
More information about the core-libs-dev