Review request for 5049299 (original) (raw)

Martin Buchholz martinrb at google.com
Fri May 22 19:30:01 UTC 2009

Hi Michael,

I am very happy to see this being worked on; I would have done something like this already if I could fork() myself.

This code is very tricky and has had many subtle bugs over the years.

Any way we could add Linux support to this, via some flavor of low-level clone+exec? Let me restate that more strongly - we would really really want to solve the memory problem for Linux as well - but how?

Remove space before (

return System.getProperty ("java.home");

Our C coding style is idiosyncratic, but there also remove spaces for function calls.

Isn't spawn only supported as of Solaris 10? I don't see anything in the change for older versions of Solaris.

The standard way of detecting presence of a function at runtime is dlsym(RTLD_DEFAULT, "function_name") != NULL

Wait! libc also has posix_spawn and friends! On many systems we should be able to find posix_spawn. Perhaps in most common special cases we can dispense with processhelper and spawn the target executable directly?

processhelper should probably be named something more obviously java-related e.g. javaprocesshelper. Can we try to not install it in BINDIR, since it is not intended to be run by users, but hide it in some private subdir?

Please add include guards to

processutil_md.h

even though you can get away with not having them.

296 * Reads nbyte bytes from file descriptor fd into buf,

Change comma to period.

processutil_md.h

can be made smaller. Utilities such as

isAsciiDigit

can be static functions called from processutil_md.c

41 * Utility used by j.l.ProcessBuilder (via UNIXProcess) to launch

In source code, let's splurge and write out java.lang instead of j.l.

47 * argv[1] working directory for command

Add "or the empty string if none provided"

You appear to be both reading and writing to single fd pipefd. I don't think bidirectional pipes are standard Unix. I'd prefer to see separate read and write pipes.

65 * - Msg type (4 bytes) 66 * 1 = chdir error (detail in Field 1) 67 * 2 = exec error (detail in Field 1) 68 * 3 = target terminated (exit code in Field 1)

Please use constants

#define CHDIR_ERROR 1 etc...

It appears that Msg type 3 is never used with reply.

100 FILE *f;

Unused?

92 int child;

Unused?

97 char name [256];

Unused?

164 } else { 165 /* empty environment */ 166 env = &nullptr; 167 }

It feels wrong to have to special-case the empty environment.

26 #undef _LARGEFILE64_SOURCE 27 #define _LARGEFILE64_SOURCE 1

There's probably a reason why we want _LARGEFILE64_SOURCE defined in the new source files.

Martin

---On Fri, May 22, 2009 at 03:05, Michael McMahon <Michael.McMahon at sun.com>wrote:

Hi,

I have just posted a webrev for 5049299: (process) Use posixspawn, not fork, on S10 to avoid swap exhaustion. webrev location: http://cr.openjdk.java.net/~michaelm/5049299/webrev.00/<http://cr.openjdk.java.net/%7Emichaelm/5049299/webrev.00/> **I'd like to give an outline of the change here, to make reviewing the webrev a bit easier. Basically, while posixspawn() is a fairly elaborate and complicated system call, it can't quite do everything that the old fork()/exec() combination can do, so the only feasible way to do this, is to use posixspawn to launch a new helper executable "processhelper", which in turn execs the target (after cleaning up file-descriptors etc.) The end result is the same as before, a child process linked to the parent in the same way, but it avoids the problem of duplicating the parent (VM) process address space temporarily, before launching the target command. In the original implementation, the native code for UNIXProcess.forkAndExec() was the same for both Linux and Solaris. Now, we have split it, so that Linux retains the original implementation but for Solaris, the native method is renamed spawn() where the implementation is partly in this function, but also partly in the new processhelper binary, which is spawned by the spawn() method. A number of utility functions, which were originally in UNIXProcessmd.c, are also needed by the processhelper binary, have been moved into new source files (processutilmd.[ch]). Because the functions were originally static in UNIXProcessmd.c, a prefix is added to their names (jlup) (from initials of java.lang.UNIXProcess), to avoid any conflict in global scope. This applies to the Linux version as well as the Solaris version. So, this looks like new code, but the body of these functions are not changed from before. I'm proposing not to add any unit/regression tests for this change, since the point of it is kind of self-evident from the source code (ie. stop using fork() and use posixspawn() instead), and there shouldn't be any behaviour change other than memory usage. This area of the platform seems to be well covered by existing regression/unit tests. Hope this helps, Thanks, Michael. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://mail.openjdk.java.net/pipermail/core-libs-dev/attachments/20090522/0d99bc21/attachment.html>

More information about the core-libs-dev mailing list