Review request for 5049299

[David Holmes - Sun Microsystems]

Hi Michael,

I can't say that I grok all the detail in this but I get the gist and 
overall I don't see anything obviously problematic. I have a couple of 
small comments:

In the Makefile:

+ HELPER_EXE = $(BINDIR)/processhelper$(EXE_SUFFIX)

Isn't EXE_SUFFIX superfluous here? It has to be an empty string 
otherwise the Java code won't know the name of the helper.

In UnixProcess_md.c:

116 jlup_xmalloc(void *env, int size)

Why did you have to lose the type of env ?
Why is this function defined differnetly in two files?

Cheers,
David


Michael McMahon said the following on 05/22/09 20:05:
> 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/
> 
> **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.
>