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

[Rob McKenna]

Hi folks,

Thanks for the feedback so far. I've uploaded a new webrev to:

http://cr.openjdk.java.net/~robm/5049299/webrev.02/ 
<http://cr.openjdk.java.net/%7Erobm/5049299/webrev.02/>

I've made the following headline changes:

- Initial effort to get this stuff into the new build-infra. Hoping 
build-infra can steer me in the right direction. (note to build infra 
reviewers: see links below)

- Source thats shared between jspawnhelper.c and UNIXProcess_md.c now 
resides in childproc.h/c. This results in cleaner changes to the makefiles.

- jspawnhelper moved to <jdk_home>/lib/<arch> on solaris (ipc 
necessitate the use of a separate jspawnhelper for each arch) and just 
/lib on macosx.

The following links to earlier threads are well worth reading for 
additional context:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-November/012417.html
http://mail.openjdk.java.net/pipermail/core-libs-dev/2009-June/001747.html

     -Rob

On 30/11/12 03:48, Rob McKenna wrote:
> 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
>>>
>