Request for Review: 5049299: (process) Use posix_spawn, not fork, on S10 to avoid swap exhaustion

[Martin Buchholz]

Rob,

Thanks for taking on this big scary change.

Our experience having run with the vfork based implementation on Linux has
been very positive.  This addresses a real need that is shared by all big
processes that desire offspring.

You might want to look through my comments from the last round of review,
back when I had more brain cells.

I agree giving people the choice to use the algorithm using a system
property is a good one.

The strategy of using posix_spawn of a small helper process seems good
(more portable than vfork or clone).  If it works well universally, you
might even consider making it the default on Linux (although I worry about
- if it works, don't break it).

clone is not known to work reliably (I tried and failed).  I would leave it
#ifdef'ed out by default.

don't put the helper program in JAVAHOME/bin because users should never
invoke it directly - it shouldn't be on anyone's PATH.

Not sure why so many dirs in HELPERLDFLAGS.  I would think that the
prochelper program would be a tiny C programs with no need to pull in any
other jdk libraries.

+                String osarch = System.getProperty("os.arch");
+                if (osarch.equals("sparcv9") || osarch.equals("amd64")) {
+                    osarch += "/";

On Solaris bi-arch I think you need only one jprochelper, not two, since a
32-bit helper can exec a 64-bit subprocess.

+#if defined(__solaris__) || defined(__APPLE__)
+#include <spawn.h>
+#endif

I think you want to autoconfiscate something like HAVE_POSIX_SPAWN instead.

+#ifdef _CS_PATH

I would separate out smaller less risky improvements like use of _CS_PATH
into a separate change.

Martin

On Fri, Nov 23, 2012 at 3:56 AM, Alan Bateman <Alan.Bateman at oracle.com>wrote:

> On 22/11/2012 21:27, Rob McKenna wrote:
>
>> Hi folks,
>>
>> Looking for a review for the webrev below, which also resolves:
>>
>> 7175692: (process) Process.exec should use posix_spawn [macosx]
>>
>> For additional context and a brief description it would be well worth
>> looking at the following thread started by Michael McMahon, who did the
>> brunt of the work for this fix:
>>
>> http://mail.openjdk.java.net/**pipermail/core-libs-dev/2009-**
>> May/thread.html#1644<http://mail.openjdk.java.net/pipermail/core-libs-dev/2009-May/thread.html#1644>
>>
>> Basically the fix aims to swap fork for posix_spawn as the default
>> process launch mechanism on Solaris and Mac OSX in order to avoid swap
>> exhaustion issues encountered with fork()/exec(). It also offers a flag
>> (java.lang.useFork) to allow a user to revert to the old behaviour.
>>
>> I'm having trouble seeing the wood for the trees at this point so I'm
>> anticipating plenty of feedback. In particular I'd appreciate some
>> discussion on:
>>
>> - The binary launcher name & property name may need some work. A more
>> general property ("java.lang.launchMechanism") to allow a user to specify a
>> particular call was mooted too. It may be more future proof and I'm
>> completely open to that. (e.g. launchMechanism=spawn|fork|**vfork|clone
>> - we would obviously ignore inapplicable values on a per-platform basis)
>> - I'd like a more robust way of checking that someone isn't trying to use
>> jprochelper outside of the context for which it is meant.
>> - The decision around which call to use getting moved to the java level
>> and away from the native preprocessor.
>>
>> The webrev is at:
>>
>> http://cr.openjdk.java.net/~**robm/5049299/webrev.01/<http://cr.openjdk.java.net/~robm/5049299/webrev.01/><
>> http://cr.openjdk.java.net/%**7Erobm/5049299/webrev.01/<http://cr.openjdk.java.net/%7Erobm/5049299/webrev.01/>
>> >
>>
> It's great to get this one moving again, we should have helped Michael
> more to get this over the line on the first outing.
>
> This one will require very careful review, I don't have cycles this week,
> I hope others do. For now I think that naming the trampoline jprochelper or
> jspawnhelper is okay. To make it more robust then I'd probably prepend a
> magic number or some token. Also I'd probably fstat stdin and uses S_FIFO
> to check the mode.
>
> As the property is implementation specific then I think something like
> jdk.lang.process.useFork is a better start. It would be nice not to require
> it although I agree we have to walk carefully as this area has a tendency
> to bite back. I don't think you need to make it any more configurable than
> that.
>
> If this is successful then I think we should look at updating the hotspot
> code too as it has the same issue with VM options such as OnError.
>
> -Alan.
>
>