I optimize PNG files by creating 5 pngout.exe processes to work in the PNG file directory. Since pngout is single-threaded, this leads to a lot of speedup. Some images take a long time to optimize, up to 30 seconds, while the norm is <5 seconds. Problem:
- File 1 is large, 2-5 are small, only 50 files, but the details of the rest are irrelevant.
- The first five pngout processes appear normally and start working
- 2-5 output for 10 seconds
- 1 takes 45 seconds.
- No new pngout processes are created during this process, even though they have four threads.
- Upon completion of 1, five more processes are generated.
code:
private final ExecutorService pool = Executors.newFixedThreadPool(5); CompletionService<Boolean> comp = new ExecutorCompletionService<Boolean>(pool); List<Callable<Boolean>> tasks = new ArrayList<Callable<Boolean>>(); for (int i = 0; i < files.length; i++) { File infile = files[i]; File outfile = new File(outdir, infile.getName()); tasks.add(new CrushTask(crusher, infile, outfile)); } for (Callable<Boolean> t : tasks) comp.submit(t); for (int i = 0; i < files.length; i++) { try { boolean res = comp.take().get(); System.out.println(res); } catch (Exception e) { e.printStackTrace(); } }
All files are optimized correctly, that part of the code works. The problem is that, waiting for large images, the whole process slows down significantly. I get only 40% improvement over single-threaded time.
What am I doing wrong?
edit: Fixed a bug using some really ugly code. The problem is that in order to get the output value of the processes that I spawned (to know when they were finished, and if they succeeded), I read their stdout to nothing, since the waitFor call would hang forever. However, using InputStreams seems to cause the threads to be suppressed.
So, to get the process output value, instead:
private static int discardStdOut(Process proc) throws IOException { final InputStream is = proc.getInputStream(); try { while (is.read() != -1) continue; return proc.exitValue(); } finally { close(is); } }
I use this generic code:
private static int discardStdOut(Process proc) { int ret = -1; while (true) { try { ret = proc.exitValue(); break; } catch (IllegalThreadStateException e) { try { Thread.sleep(100); } catch (InterruptedException e2) { e2.printStackTrace(); } } } return ret; }
It's rude, but now the system is working fine and 5 processes are always running.
later editing: StreamGobbler from here is probably more correct.