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

Thu Dec 20 04:04:29 UTC 2012

Thanks for this.  I agree with the strategy.

I'm hoping build folks and macosx folks also take a look at this hairy
change.

+LINKFLAG =
+ifeq ($(ARCH_DATA_MODEL), 64)
+LINKFLAG = -m64
+endif

It looks wrong to be specifying toolchain-specific flags here.   Also, -m64
is logically a compiler flag, i.e. belongs in a variable with a name like
CFLAGS.

---
 ifeq ($(OPENJDK_TARGET_OS), solaris)
- ifeq ($(OPENJDK_TARGET_CPU_BITS), 32)
- BUILD_JEXEC := 1
- endif
+    ifeq ($(OPENJDK_TARGET_CPU_BITS), 32)
+        BUILD_JEXEC := 1
+    endif
 endif

Why mess with jexec?

---

+                String s = System.getProperty("java.lang.useFork");

"java.lang.Process.useFork" is a better name.
Also, useFork suggests it's a binary choice.  Wouldn't it be better to have
the system property
"java.lang.Process.spawn.mechanism" with values fork, vfork, posix_spawn,
clone

---

+    enum LaunchMechanism {
+        CLONE(1), FORK(2),
+        VFORK(3), SPAWN(4);

I would rename SPAWN to POSIX_SPAWN.
The enum can be moved to a unix-flavor-independent file.

---

+                helperpath = javahome + "/lib/" + osarch + "/jspawnhelper";

There ought to be a standard way to get the "libarchdir"

---

Of course, WhyCantJohnnyExec should have been WhyJohnnyCantExec
This is your chance to correct my mistake!

---

Sometimes I see __APPLE__ sometimes I see _ALLBSD_SOURCE.  Hopefully such
things will be removed later when the new build system is obligatory.

---

+    /* Initialize xx_parentPathv[] */

It's not called xx_anything any more.

---

+            shutItDown();

How about renaming to usageError() ?

---

+    r = sscanf (argv[argc-1], "%d:%d", &fdin, &fdout);

How about checking for argc == 2 ?

Martin

On Wed, Dec 19, 2012 at 6:28 PM, Rob McKenna <rob.mckenna at oracle.com> wrote:

> 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/~robm/5049299/webrev.02/><
> http://cr.openjdk.java.net/%**7Erobm/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/2012-November/012417.html>
> http://mail.openjdk.java.net/**pipermail/core-libs-dev/2009-**
> June/001747.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<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<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<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/>
>>>> >
>>>>
>>>> Thanks a lot,
>>>>
>>>> -Rob
>>>>
>>>>
>>
>