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.mcma...@sun.com <mailto:michael.mcma...@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.
