UNIXProcess improvements

[David Schlosnagle]

On Wed, Apr 21, 2010 at 9:14 PM, Martin Buchholz <martinrb at google.com> wrote:
> Thanks for the careful review.

No problem, I wanted test this patch out on OS X since the current
UNIXProcess.java.bsd is virtually identical to UNIXProcess.java.linux.
I slightly tweaked the ManyProcesses test to exec /usr/bin/true and
log the elapsed time within main, as well as using `time` for overall
process time.

Running ManyProcesses with Apple's version of 1.6.0_17-b04-248,
averages around 45 seconds and 4010 started threads.
Running ManyProcesses with the bsd-port of OpenJDK7 on OS X without
these changes, averages around 16 seconds and 4010 started threads.
Running ManyProcesses with the bsd-port of OpenJDK7 on OS X with these
changes, averages around 14 seconds and around 14 started threads.

One definite benefit is that the total number of started threads drops
from 4010 to around 14. Compared to the Apple version of Java 6, I see
the multiple factor speedup of 3x. The speedup on my machine for
OpenJDK7 bsd-port with this patch versus without is much smaller, of
course that could be due differences in OS and hardware (I'm running
on an older Intel Core 2 Duo).

> Yes, the streams really should be final,
> but it's not worth the boilerplate handsprings to make them so.

I'm not sure I quite follow, I was thinking something along the lines of:

    private final OutputStream stdin;
    private final InputStream  stdout;
    private final InputStream  stderr;

// ...snip...

    UNIXProcess(final byte[] prog,
                final byte[] argBlock, final int argc,
                final byte[] envBlock, final int envc,
                final byte[] dir,
                final int[] fds,
                final boolean redirectErrorStream)
            throws IOException {

        pid = forkAndExec(prog,
                          argBlock, argc,
                          envBlock, envc,
                          dir,
                          fds,
                          redirectErrorStream);

        stdin = (fds[0] == -1) ?
                ProcessBuilder.NullOutputStream.INSTANCE :
                    new ProcessPipeOutputStream(fds[0]);

        stdout = (fds[1] == -1) ?
                ProcessBuilder.NullInputStream.INSTANCE :
                    new ProcessPipeInputStream(fds[1]);

        stderr = (fds[2] == -1) ?
                ProcessBuilder.NullInputStream.INSTANCE :
                    new ProcessPipeInputStream(fds[2]);

        startReaper();
    }

    private void startReaper() throws IOException {
        try {
            AccessController.doPrivileged
            (new PrivilegedExceptionAction<Void>() {
                public Void run() throws IOException {
                    processReaperExecutor.execute(new Runnable() {
                        public void run() {
                            int exitcode = waitForProcessExit(pid);
                            UNIXProcess.this.processExited(exitcode);
                        }});
                    return null;
                }});
        } catch (PrivilegedActionException ex) {
            throw (IOException) ex.getException();
        }
    }


>> Minor nit, the java.io.ByteArrayOutputStream import is not used.
>
> Fixed.  (do you have a tool to detect extra imports?)

I had just dropped your changes into Eclipse to quickly rebuild the
classes to test out on my OS X build, and noticed that it was unused.
It really shouldn't matter, javac won't include it in the class file
anyway.

Thanks,
Dave