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

[Rob McKenna]

Hi David,

On 30/11/12 02:31, David Holmes wrote:
> Hi Rob,
>
> This is only a superficial scan.
>
> The changes in java/java/makefile look pretty horrible. What are all 
> those -R entries?
Library search paths. Currently jprochelper is linked to libjava. I'm 
hoping to either cut their number (by altering jprochelpers home) or get 
rid of them altogether (by avoiding linking at all) in the next draft, 
they are indeed ungainly.
>
> We will need equivalent changes for the new build system before this 
> is pushed.
>
Indeed.
> Is the spawn use BSD specific (as per UnixProcess.java.BSD) or Apple 
> specific (as per __APPLE_ in UNIXProcess_md.c) ?
>
Eesh, thanks, it applies to both platforms.
> Are the UnixProcess.java files similar enough that we could use a 
> single template and generate the per-OS variants?
>
Before this change .bsd & .linux were identical (iirc) unfortunately, no 
longer. Solaris has  differences. When you say "generate the per-OS 
variants" how do you mean? I'd like to keep it as straightforward as 
possible from a sustaining perspective. (personally I'd like to just 
extend a base class and try to get away from the makefiles as much as 
possible - we can discuss this in 8000975 which I'll revisit once I get 
through this)
> In UNIXProcess_md.c:
>
> 209 #ifdef _CS_PATH
>  210     char *pathbuf;
>  211     size_t n;
>  212     n = confstr(_CS_PATH,NULL,(size_t) 0);
>  213     pathbuf = malloc(n);
>  214     if (pathbuf == NULL)
>  215         abort();
>  216     confstr(_CS_PATH, pathbuf, n);
>  217     return pathbuf;
>  218 #else
>
> what is _CS_PATH and why are we calling abort()? !!!!
>
As per Martins comments I'm going to separate this into another change. See:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2009-May/001686.html

and

http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-November/012456.html

for context. I'll look to fall back to the previous code if the pathbuf 
malloc fails.
> What is all the xx_ naming ??
>
I believe Michael was using it to denote shared calls. (functions called 
from both jprochelper and within UNIXProcess_md.c). I presumed it was 
placeholder text actually, in any case it may go away in the next 
iteration as per previous comments. If not, I'm happy to replace it with 
whatever gets it past codereview.

     -Rob

> David
> -----
>
>
> On 23/11/2012 7:27 AM, 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 
>>
>>
>>
>> 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/%7Erobm/5049299/webrev.01/>
>>
>> Thanks a lot,
>>
>> -Rob
>>