<br><br><div class="gmail_quote">On Fri, May 22, 2009 at 15:13, Michael McMahon <span dir="ltr"><<a href="mailto:Michael.McMahon@sun.com" target="_blank">Michael.McMahon@sun.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">

Martin,<br>
<br>
Thanks. Great comments. Just a few comments of my own<br>
on a couple of points.<br>
<br>
1. Linux won't benefit from this change as much as solaris, since due to its<br>
   "memory overcommit" architecture, it doesn't suffer from the problem (so much)<br>
    in the first place (though memory overcommit causes some problems of its own).<br>
    Nevertheless, maybe it could simplify the code a bit if we use posix_spawn() on Linux<br>
    as well. So, I will look into that.<br>
</blockquote><div><br>Thank you very much.<br><br>Any company running server farms (think "Sun" or "Google")<br>would like to "bin-pack" as many processes as possible onto them,<br>and transient doubling of process size is a big problem in such an<br>
environment.  Think of this as a <br>saving-the-planet-from-global-warning feature. <br><br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">

2. Support for older Solaris releases. My understanding was that jdk7 doesn't need to support<br>
   older releases of Solaris (than S10). Can someone clarify that situation?</blockquote><div><br>Sun used to be *so* conservative.  <br>I think 5 years of support after OS FCS is a minimum.<br>Especially for Solaris, which is very much a server OS.<br>
<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
3. Avoiding invocation of processhelper sometimes. The biggest issue (I found) with posix_spawn()<br>
   is that it doesn't work too well in multi-threaded environments like the JVM.  The specific problem<br>
   is that you have to set up a set of file-descriptor actions, prior to calling the function, whose purpose<br>
   is to close file descriptors from the parent process in the child. Because the two parts to this are not<br>
   atomic, other file descriptors could be opened/closed in the intervening period, and you couldn't<br>
   guarantee that they would be handled correctly. So, for that reason, I see no way to avoid the<br>
   "processhelper" approach, where we take care of the child's file descriptors after the child is spawned.<br>
</blockquote><div><br>I think you are perfectly right.  <br>I think I came to the same conclusion myself once,<br>but then forgot about it.<br><br>---<br><br>The other approach on Linux is to try clone(2)<br>with flag CLONE_VM<br>
but not CLONE_VFORK or CLONE_FS or CLONE_FILES,<br>instead of fork().  Then the child can modify<br>file descriptors and chdir without interference<br>except for the "small" problem that all memory is shared.<br>
This doesn't work for environment variables,<br>so more special-casing or trickery may be required.<br><br>Martin<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
Thanks,<br>
Michael.<br>
<br>
Martin Buchholz wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div><div></div><div>
Hi Michael,<br>
<br>
I am very happy to see this being worked on;<br>
I would have done something like this<br>
already if I could fork() myself.<br>
<br>
This code is very tricky and has had many subtle bugs over the years.<br>
<br>
---<br>
<br>
Any way we could add Linux support to this,<br>
via some flavor of low-level clone+exec?<br>
Let me restate that more strongly -<br>
we would really really want to solve the<br>
memory problem for Linux as well - but how?<br>
<br>
---<br>
<br>
Remove space before (<br>
return System.getProperty ("java.home");<br>
<br>
Our C coding style is idiosyncratic,<br>
but there also remove spaces for<br>
function calls.<br>
  ---<br>
<br>
Isn't spawn only supported as of Solaris 10?<br>
I don't see anything in the change for<br>
older versions of Solaris.<br>
<br>
---<br>
<br>
The standard way of detecting presence of a function<br>
at runtime is<br>
dlsym(RTLD_DEFAULT, "function_name") != NULL<br>
<br>
Wait! libc also has posix_spawn and friends!<br>
On many systems we should be able to find posix_spawn.<br>
Perhaps in most common special cases we can dispense with processhelper<br>
and spawn the target executable directly?<br>
<br>
---<br>
<br>
processhelper should probably be named something more obviously java-related<br>
e.g. javaprocesshelper.<br>
Can we try to not install it in BINDIR, since it is not intended to be run by users,<br>
but hide it in some private subdir?<br>
<br>
---<br>
<br>
Please add include guards to<br>
processutil_md.h<br>
<br>
even though you can get away with not having them.<br>
<br>
---<br>
 296  * Reads nbyte bytes from file descriptor fd into buf,<br>
Change comma to period.<br>
<br>
---<br>
processutil_md.h<br>
can be made smaller.<br>
Utilities such as<br>
isAsciiDigit<br>
<br>
can be static functions called from processutil_md.c<br>
  ---<br>
  41  * Utility used by j.l.ProcessBuilder (via UNIXProcess) to launch<br>
<br>
In source code, let's splurge and write out java.lang instead of j.l.<br>
<br>
---<br>
  47  * argv[1] working directory for command<br>
<br>
<br>
Add "or the empty string if none provided"<br>
<br>
---<br>
<br>
You appear to be both reading and writing to single fd pipefd.<br>
I don't think bidirectional pipes are standard Unix.<br>
I'd prefer to see separate read and write pipes.<br>
<br>
<br>
---<br>
<br>
  65  * - Msg type (4 bytes)<br>
  66  *      1 = chdir error (detail in Field 1)<br>
  67  *      2 = exec error (detail in Field 1)<br>
  68  *      3 = target terminated (exit code in Field 1)<br>
<br>
Please use constants<br>
<br>
<br>
#define CHDIR_ERROR 1<br>
etc...<br>
<br>
<br>
---<br>
<br>
It appears that Msg type 3 is never used with reply.<br>
<br>
---<br>
<br>
 100     FILE *f;<br>
<br>
Unused?<br>
<br>
---<br>
  92     int child;<br>
<br>
Unused?<br>
<br>
---<br>
<br>
<br>
  97     char name [256];<br>
<br>
Unused?<br>
<br>
---<br>
<br>
 164     } else {<br>
 165         /* empty environment */<br>
 166         env = &nullptr;<br>
 167     }<br>
<br>
It feels wrong to have to special-case the<br>
<br>
empty environment.<br>
  ---<br>
  26 #undef  _LARGEFILE64_SOURCE<br>
  27 #define _LARGEFILE64_SOURCE 1<br>
<br>
There's probably a reason why we want _LARGEFILE64_SOURCE<br>
defined in the new source files.<br>
<br>
---<br>
<br>
Martin<br>
<br>
  ---<br></div></div><div>
On Fri, May 22, 2009 at 03:05, Michael McMahon <<a href="mailto:Michael.McMahon@sun.com" target="_blank">Michael.McMahon@sun.com</a> <mailto:<a href="mailto:Michael.McMahon@sun.com" target="_blank">Michael.McMahon@sun.com</a>>> wrote:<br>


<br>
    Hi,<br>
<br>
    I have just posted a webrev for 5049299: (process) Use<br>
    posix_spawn, not fork, on S10 to avoid swap exhaustion.<br>
<br>
    webrev location:<br>
    <a href="http://cr.openjdk.java.net/%7Emichaelm/5049299/webrev.00/" target="_blank">http://cr.openjdk.java.net/~michaelm/5049299/webrev.00/</a><br></div>
    <<a href="http://cr.openjdk.java.net/%7Emichaelm/5049299/webrev.00/" target="_blank">http://cr.openjdk.java.net/%7Emichaelm/5049299/webrev.00/</a>><div><div></div><div><br>
<br>
    **I'd like to give an outline of the change here, to make<br>
    reviewing the webrev a bit easier.<br>
    Basically, while posix_spawn() is a fairly elaborate and<br>
    complicated system call, it can't quite do<br>
    everything that the old fork()/exec() combination can do, so the<br>
    only feasible way to do this, is to<br>
    use posix_spawn to launch a new helper executable "processhelper",<br>
    which in turn execs the<br>
    target (after cleaning up file-descriptors etc.) The end result is<br>
    the same as before, a child process<br>
    linked to the parent in the same way, but it avoids the problem of<br>
    duplicating the parent (VM) process<br>
    address space temporarily, before launching the target command.<br>
<br>
    In the original implementation, the native code for<br>
    UNIXProcess.forkAndExec() was the same<br>
    for both Linux and Solaris.  Now, we have split it, so that Linux<br>
    retains the original implementation<br>
    but for Solaris, the native method is renamed spawn() where the<br>
    implementation is partly in this function,<br>
    but also partly in the new processhelper binary, which is spawned<br>
    by the spawn() method.<br>
<br>
    A number of utility functions, which were originally in<br>
    UNIXProcess_md.c, are also needed by the<br>
    processhelper binary, have been moved into new source files<br>
    (processutil_md.[ch]).<br>
    Because the functions were originally static in UNIXProcess_md.c,<br>
    a prefix is added to their names (jlup_)<br>
    (from initials of java.lang.UNIXProcess), to avoid any conflict in<br>
    global scope. This applies to the Linux<br>
    version as well as the Solaris version. So, this looks like new<br>
    code, but the body of these functions<br>
    are not changed from before.<br>
<br>
    I'm proposing not to add any unit/regression tests for this<br>
    change, since the point of it<br>
    is kind of self-evident from the source code (ie. stop using<br>
    fork() and use posix_spawn() instead),<br>
    and there shouldn't be any behaviour change other than memory usage.<br>
    This area of the platform seems to be well covered by existing<br>
    regression/unit tests.<br>
<br>
    Hope this helps,<br>
    Thanks,<br>
    Michael.<br>
<br>
<br>
</div></div></blockquote>
<br>
</blockquote></div><br>