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