Thanks for the careful review.
On Wed, Apr 21, 2010 at 17:47, David Schlosnagle <schlo...@gmail.com> wrote:
> Martin,
>
> In src/solaris/classes/java/lang/UNIXProcess.java.linux, are lines
> 177, 181, and 185 needed? If the assignment of these streams were in
> the constructor, they could be final.
Fixed.
Yes, the streams really should be final,
but it's not worth the boilerplate handsprings to make them so.
- private OutputStream stdin_stream;
- private InputStream stdout_stream;
- private InputStream stderr_stream;
+ private /* final */ OutputStream stdin;
+ private /* final */ InputStream stdout;
+ private /* final */ InputStream stderr;
> 176 synchronized void processExited(int exitcode) {
> 177 stdout = this.stdout;
> 178 if (stdout instanceof ProcessPipeInputStream)
> 179 ((ProcessPipeInputStream) stdout).processExited();
> 180
> 181 stderr = this.stderr;
> 182 if (stderr instanceof ProcessPipeInputStream)
> 183 ((ProcessPipeInputStream) stderr).processExited();
> 184
> 185 stdin = this.stdin;
> 186 if (stdin instanceof ProcessPipeOutputStream)
> 187 ((ProcessPipeOutputStream) stdin).processExited();
> 188
> 189 this.exitcode = exitcode;
> 190 hasExited = true;
> 191 notifyAll();
> 192 }
>
> Minor nit, the java.io.ByteArrayOutputStream import is not used.
Fixed. (do you have a tool to detect extra imports?)
Thanks,
Martin
> Thanks,
> Dave
>
> On Wed, Apr 21, 2010 at 7:15 PM, Martin Buchholz <marti...@google.com> wrote:
>>
>> I now have the second part of my planned improvements to
>> Linux process handling, and is now a serious proposal.
>> for Michael and others to review.
>>
>> http://cr.openjdk.java.net/~martin/webrevs/openjdk7/UNIXProcess/
>> http://cr.openjdk.java.net/~martin/webrevs/openjdk7/UNIXProcess2/
>>
>> This is a huge performance and reliability improvement for
>> java programs that create many processes.
>> The program below (ManyProcesses) runs 4 times faster on my machine.
>>
>> This doesn't try to address Solaris.
>>
>> Martin
>>
>> import java.util.*;
>> import java.util.concurrent.atomic.*;
>>
>> public class ManyProcesses {
>> public static void main(String[] args) throws Throwable {
>> final int threadCount = 2;
>> final int processCount = 2000;
>> final AtomicLong count = new AtomicLong(0);
>> Runnable r = new Runnable() {
>> public void run() {
>> try {
>> for (int i = 0; i < processCount; i++) {
>> Process p = new ProcessBuilder(new String[] {
>> "/bin/true" }).start();
>> //if (p.waitFor() != 0) throw new Error();
>> // p.getInputStream().close();
>> // p.getOutputStream().close();
>> // p.getErrorStream().close();
>> //count.getAndIncrement();
>> }
>> } catch (Throwable t) {
>> throw new Error(t);
>> }}};
>> List<Thread> threads = new ArrayList<Thread>();
>> for (int i = 0; i < threadCount; i++) {
>> threads.add(new Thread(r));
>> }
>>
>> for (Thread thread : threads) thread.start();
>> //Thread.sleep(1000 * 10);
>> //System.out.println(count.get());
>> for (Thread thread : threads) thread.join();
>> System.out.println(new Thread().getId());
>> }
>> }
>>
>> On Tue, Apr 20, 2010 at 08:58, Michael McMahon <michael.mcma...@sun.com>
>> wrote:
>> > Martin,
>> >
>> > Thanks for the answers. The changes look fine to me.
>> >
>> > - Michael.
>> >
>> > Martin Buchholz wrote:
>> >>
>> >> On Mon, Apr 19, 2010 at 09:04, Michael McMahon <michael.mcma...@sun.com>
>> >> wrote:
>> >>
>> >>>
>> >>> Martin Buchholz wrote:
>> >>>
>> >>>>
>> >>>> On Fri, Apr 16, 2010 at 09:18, Mark Reinhold <m...@sun.com> wrote:
>> >>>>
>> >>>>
>> >>>>
>> >>>>>
>> >>>>> For now I suggest leaving old @author tags as-is.
>> >>>>>
>> >>>>>
>> >>>>
>> >>>> OK, done.
>> >>>>
>> >>>> Version 0.2 of the webrev is published.
>> >>>>
>> >>>> Martin
>> >>>>
>> >>>>
>> >>>
>> >>> Martin,
>> >>>
>> >>> From what I can see, you've cleaned up the code and the functional
>> >>> changes
>> >>> are the use of a thread pool, and an explicit (8 k sized) stack.
>> >>>
>> >>
>> >> Exceptions thrown in the "process reaper" thread,
>> >> which were not IOExceptions,
>> >> were not being caught, and would cause the user thread to hang.
>> >>
>> >>
>> >>>
>> >>> Also, the threads created now belong to the root thread group rather than
>> >>> the application's thread group.
>> >>>
>> >>
>> >> Well, they have to belong to some thread group,
>> >> and they get reused, so in general the thread group will have
>> >> no relation to the thread group of the user thread,
>> >> so may as well use the root thread group.
>> >> I stole this technique from elsewhere in the JDK.
>> >>
>> >>
>> >>>
>> >>> Is this so you can handle uncaught
>> >>> exceptions
>> >>> as you mentioned before, and if so, I guess some other change is coming
>> >>> to
>> >>> complete
>> >>> this work. Is that right?
>> >>>
>> >>
>> >> Yes. This change by itself is a clear win,
>> >> except that because it is more memory efficient,
>> >> GC is less likely to get called,
>> >> which means file descriptors of defunct processes
>> >> are less likely to get cleaned up in the face of
>> >> lazy user code, which means it is more subject
>> >> to file descriptor exhaustion problems.
>> >> Which I would like to fix.
>> >>
>> >> Martin
>> >>
>> >>
>> >>>
>> >>> Thanks,
>> >>> Michael
>> >>>
>> >>>
>> >>>
>> >
>> >
>
