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
