Review request for 5049299
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
> posix_spawn() 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.
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?
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 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.
>
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(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/>
>> <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.
>>
>>
>>
>
-------------- 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