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

Martin Buchholz martinrb at google.com
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)

endif

Why mess with jexec?

"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

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

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.

It's not called xx_anything any more.

How about renaming to usageError() ?

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 UNIXProcessmd.c now resides in childproc.h/c. This results in cleaner changes to the makefiles. - jspawnhelper moved to /lib/ 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 UNIXProcessmd.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 UNIXProcessmd.c: 209 #ifdef CSPATH 210 char *pathbuf; 211 sizet n; 212 n = confstr(CSPATH,NULL,(sizet) 0); 213 pathbuf = malloc(n); 214 if (pathbuf == NULL) 215 abort(); 216 confstr(CSPATH, pathbuf, n); 217 return pathbuf; 218 #else what is CSPATH 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 UNIXProcessmd.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 posixspawn [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 posixspawn 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

More information about the core-libs-dev mailing list