On 10/08/2013 17:16, Rob McKenna wrote:
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
Thanks for the update.
In UnixProcess.java.XXX then it looks like the statics javaHome and arch
aren't needed now. You should be able to change LaunchMechanism to
private static too. One suggestion to consider too is save the byte
representation of the jspawnhelper path as it shouldn't be necessary to
have to convert that to bytes each time. It's okay to do this with
change-set if you want to get the current patch push.
Otherwise I think this is good, you've sorted out all other issues. Also
thanks for double checking that we aren't leaking file descriptors.
-Alan
