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.mcke...@oracle.com
<mailto:rob.mcke...@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
