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

[Rob McKenna]

Thanks for the build review Erik,

Hi Alan,

Thanks for the review. I'm hoping this webrev has dealt with your comments.

http://cr.openjdk.java.net/~robm/5049299/webrev.06/ 
<http://cr.openjdk.java.net/%7Erobm/5049299/webrev.06/>

A couple of notes:

- The jexec comments in CompileLauncher referenced the old build too. I 
figured it was a handy way to find the corresponding makefile in the old 
build. Removed though.

- I've added a return -1 default case to startChild to avoid a "control 
reaches end of non-void function" warning on mac & linux. forkAndExec 
looks for a negative integer specifically, but this should never 
actually be reached.

- Added the ifdef's around spawnChild and its use to avoid an implicit 
declaration warning on Linux. (posix_spawn)

I've tested with the attached (quick and very dirty) test. Forgive the 
System.gc & the sleep, but as you can see from the output below, it 
looks like we're correctly closing our fd's. (a much longer running 
version followed the same pattern) Clearly the test is totally 
unsuitable for integration, but let me know if you can think of a better 
way to do this.

bash-3.00$ 
/suspool/home/rob/8/tl/build/solaris-sparcv9-normal-server-release/jdk/bin/java 
FDLeakTest
21360      <--- this is the pid
10
3010
358
3358
850
3850
1288
4288
1636
4636
10

     -Rob


On 28/07/13 22:18, Alan Bateman wrote:
> On 25/07/2013 09:17, Rob McKenna wrote:
>> Thanks a lot Erik,
>>
>> I've added the dependency to the makefile here:
>>
>> http://cr.openjdk.java.net/~robm/5049299/webrev.05/ 
>> <http://cr.openjdk.java.net/%7Erobm/5049299/webrev.05/>
>>
>> Is it ok between the ifeq block?
>>
>> Alan,
>>
>> I've altered the launchMechanism code to use valueOf (and lower case 
>> names) - much cleaner, thanks for that. I've also limited each 
>> platform to supported mechanisms only. With the new layout I don't 
>> believe a separate test is needed here. (the default is now 
>> posix_spawn/vfork and Basic.java has been altered to run with fork too)
>>
>>     -Rob
> Thanks Rob, I've taken another pass over the latest webrev and I think 
> the finish line is close.
>
> The launchMechanism determination is much cleaner now (thanks). For 
> consistency the enum values should probably be in uppercase and so 
> this means you'll need to uppercase the property value to use valueOf.
>
> It would be nice if launchMechanism were final too (which you do by 
> having the privileged action be of type LaunchMechanism rather than Void).
>
> The comment in UNIXProcess.java.bsd references vfork/exec which I 
> assume is copied from the Linux version and should be removed.
>
> Did you consider having forkAndExec take the helper as a parameter? 
> Just wonder if this would be cleaner than having to use JNI to grab 
> the field value.
>
> It's usually best to use sprintnf rather than sprintf (in spawnChild) 
> to avoid any concerns (or static analysis tools) that wonder about 
> buffer overflow.
>
> Style nit in the C code is that many places have spurious space 
> between the function name and the parentheses.
>
> A pre-existing bug (only noticed because it has moved from 
> UNIXProcess_md.c to childproc.c) is that close is not restartable.
>
> The comment in in makefiles/CompileLauncher.gmk references the old 
> build, is that needed?
>
> So overall I don't see any issues, it would be really good to stress 
> this to make sure that we aren't leaking file descriptors. I don't 
> know if we have an existing test that would help with that.
>
> One final comment is that the new files seems to have the pure GPL 
> comment, I assume you will add the GPL+ Classpath Exception comment 
> before you push this.
>
> -Alan.
>