Request for Review: 5049299: (process) Use posix_spawn, not fork, on S10 to avoid swap exhaustion
Rob McKenna
rob.mckenna at oracle.com
Sun Dec 23 01:19:59 UTC 2012
Hi Martin, thanks a lot for this.
I've renamed LINKFLAG to the more appropriate (and common) ARCHFLAG. It
seems to pop up all around our source but if build-dev know of a better
(or canonical) way of doing it I'll take it!
The BUILD_JEXEC differences don't show up in my webrev or hg diff, but I
see them in the jdk.patch generated by webrev. I have no idea why this
is. (I did use the JEXEC stuff as a template for the JSPAWNHELPER code,
but I don't recall altering the JEXEC stuff in any way. It looks like
its limited to indentation. Strange.)
W.r.t. useFork, I'll discuss this with Alan once he has a spare minute.
I believe he may have had some concerns, but I'll let you know how that
goes either way.
The only __APPLE__ should be to get the call to _NSGetEnviron().
Everything else should be bsd.
I've put a webrev at:
http://cr.openjdk.java.net/~robm/5049299/webrev.03/
<http://cr.openjdk.java.net/%7Erobm/5049299/webrev.03/>
I hope this addresses the rest of your comments.
-Rob
P.S. Gah, just noticed you requested the move of that enum to a single
flavour independent file. I'll do that for the next round.
(src/solaris/classes/java/lang/ProcessImpl.java?)
On 20/12/12 04:04, Martin Buchholz wrote:
> 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
> <mailto: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/%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/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/>
> <http://cr.openjdk.java.net/%7Erobm/5049299/webrev.01/>
>
> Thanks a lot,
>
> -Rob
>
>
>
>
More information about the build-dev
mailing list