Review request for 5049299

[Michael McMahon]

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 
posix_spawn() on Linux
     as well. So, I will look into that.

2. 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?

3. Avoiding invocation of processhelper sometimes. The biggest issue (I 
found) with posix_spawn()
    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.

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(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 <mailto:Michael.McMahon at sun.com>> wrote:
>
>     Hi,
>
>     I have just posted a webrev for 5049299: (process) Use
>     posix_spawn, 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 posix_spawn() 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 posix_spawn 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
>     UNIXProcess_md.c, are also needed by the
>     processhelper binary, have been moved into new source files
>     (processutil_md.[ch]).
>     Because the functions were originally static in UNIXProcess_md.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 posix_spawn() 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.
>
>