(original) (raw)

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.McMahon@sun.com Michael.McMahon@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.