Review request for 5049299 (original) (raw)

David Holmes - Sun Microsystems David.Holmes at Sun.COM
Fri May 22 10:51:18 UTC 2009

Hi Michael,

I can't say that I grok all the detail in this but I get the gist and overall I don't see anything obviously problematic. I have a couple of small comments:

In the Makefile:

Isn't EXE_SUFFIX superfluous here? It has to be an empty string otherwise the Java code won't know the name of the helper.

In UnixProcess_md.c:

116 jlup_xmalloc(void *env, int size)

Why did you have to lose the type of env ? Why is this function defined differnetly in two files?

Cheers, David

Michael McMahon said the following on 05/22/09 20:05:

Hi,

I have just posted a webrev for 5049299: (process) Use posixspawn, not fork, on S10 to avoid swap exhaustion. webrev location: http://cr.openjdk.java.net/~michaelm/5049299/webrev.00/ **I'd like to give an outline of the change here, to make reviewing the webrev a bit easier. Basically, while posixspawn() is a fairly elaborate and complicated system call, it can't quite do everything that the old fork()/exec() combination can do, so the only feasible way to do this, is to use posixspawn to launch a new helper executable "processhelper", which in turn execs the target (after cleaning up file-descriptors etc.) The end result is the same as before, a child process linked to the parent in the same way, but it avoids the problem of duplicating the parent (VM) process address space temporarily, before launching the target command. In the original implementation, the native code for UNIXProcess.forkAndExec() was the same for both Linux and Solaris. Now, we have split it, so that Linux retains the original implementation but for Solaris, the native method is renamed spawn() where the implementation is partly in this function, but also partly in the new processhelper binary, which is spawned by the spawn() method. A number of utility functions, which were originally in UNIXProcessmd.c, are also needed by the processhelper binary, have been moved into new source files (processutilmd.[ch]). Because the functions were originally static in UNIXProcessmd.c, a prefix is added to their names (jlup) (from initials of java.lang.UNIXProcess), to avoid any conflict in global scope. This applies to the Linux version as well as the Solaris version. So, this looks like new code, but the body of these functions are not changed from before. I'm proposing not to add any unit/regression tests for this change, since the point of it is kind of self-evident from the source code (ie. stop using fork() and use posixspawn() instead), and there shouldn't be any behaviour change other than memory usage. This area of the platform seems to be well covered by existing regression/unit tests. Hope this helps, Thanks, Michael.

More information about the core-libs-dev mailing list