Review request for 5049299 (original) (raw)

Martin Buchholz martinrb at google.com
Sat May 23 00:57:26 UTC 2009

On Fri, May 22, 2009 at 15:13, Michael McMahon <Michael.McMahon at sun.com>wrote:

Martin,

Thanks. Great comments. Just a few comments of my own on a couple of points. 1. Linux won't benefit from this change as much as solaris, since due to its "memory overcommit" architecture, it doesn't suffer from the problem (so much) in the first place (though memory overcommit causes some problems of its own). Nevertheless, maybe it could simplify the code a bit if we use posixspawn() on Linux as well. So, I will look into that.

Thank you very much.

Any company running server farms (think "Sun" or "Google") would like to "bin-pack" as many processes as possible onto them, and transient doubling of process size is a big problem in such an environment. Think of this as a saving-the-planet-from-global-warning feature.

  1. Support for older Solaris releases. My understanding was that jdk7

    doesn't need to support older releases of Solaris (than S10). Can someone clarify that situation?

Sun used to be so conservative. I think 5 years of support after OS FCS is a minimum. Especially for Solaris, which is very much a server OS.

3. Avoiding invocation of processhelper sometimes. The biggest issue (I found) with posixspawn() is that it doesn't work too well in multi-threaded environments like the JVM. The specific problem is that you have to set up a set of file-descriptor actions, prior to calling the function, whose purpose is to close file descriptors from the parent process in the child. Because the two parts to this are not atomic, other file descriptors could be opened/closed in the intervening period, and you couldn't guarantee that they would be handled correctly. So, for that reason, I see no way to avoid the "processhelper" approach, where we take care of the child's file descriptors after the child is spawned.

I think you are perfectly right. I think I came to the same conclusion myself once, but then forgot about it.

The other approach on Linux is to try clone(2) with flag CLONE_VM but not CLONE_VFORK or CLONE_FS or CLONE_FILES, instead of fork(). Then the child can modify file descriptors and chdir without interference except for the "small" problem that all memory is shared. This doesn't work for environment variables, so more special-casing or trickery may be required.

Martin

Thanks, Michael. Martin Buchholz wrote:

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(RTLDDEFAULT, "functionname") != NULL Wait! libc also has posixspawn and friends! On many systems we should be able to find posixspawn. 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 processutilmd.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. --- processutilmd.h can be made smaller. Utilities such as isAsciiDigit can be static functions called from processutilmd.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 CHDIRERROR 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 LARGEFILE64SOURCE 27 #define LARGEFILE64SOURCE 1 There's probably a reason why we want LARGEFILE64SOURCE defined in the new source files. --- Martin --- On Fri, May 22, 2009 at 03:05, Michael McMahon <Michael.McMahon at sun.com<mailto:_ _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/> <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/ef596c17/attachment.html>

More information about the core-libs-dev mailing list