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> 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
>>>>
>>>>
>>
>
