2

this is my first time using thread pools and I dont quite understand how the executorservice works. I put watermarks over an image and merge them onto an empty picture. But even if i only use one thread it will still only draw half.

This is my WorkerThread class:

public class WorkerThread implements Runnable {

    BufferedImage source;
    BufferedImage toDraw;
    int x;
    int y;
    BufferedImage target;
    ParallelWatermarkFilter pf;

    public WorkerThread(BufferedImage source, BufferedImage toDraw, int x, int y, BufferedImage target){
        this.source = source;
        this.toDraw = toDraw;
        this.x = x;
        this.y = y;
        this.target = target;
        pf = new ParallelWatermarkFilter(source, 5);
    }

    @Override
    public void run() {
        pf.mergeImages(source, toDraw, x, y, target);
    }
}

And this is is how i use the ExecutorService in my FilterClass:

    public BufferedImage apply(BufferedImage input) {

        ExecutorService threadpool = Executors.newFixedThreadPool(numThreads);

                for (int w = 0; w < imgWidth; w += watermarkWidth) {
      for (int h = 0; h < imgHeight; h += watermarkHeight) {
            Runnable worker = new WorkerThread(input, watermark, w, h, result);
            System.out.println("WIDTH: " + w + "   HEIGHT: " + h);
            threadpool.execute(worker);
      }
    }

    threadpool.shutdown();

Do the threads not wait until one thread is done ?

Selfie21
  • 57
  • 5
  • Welcome to Stack Overflow. I’m not sure exactly what you mean. The thread creating the thread pool (your main thread) does not await completion of the work submitted to the thread pool, if that was what you meant. – Ole V.V. Jul 01 '18 at 18:38
  • 2
    Possible duplicate of [How to wait for all threads to finish, using ExecutorService?](https://stackoverflow.com/questions/1250643/how-to-wait-for-all-threads-to-finish-using-executorservice) – d.j.brown Jul 01 '18 at 18:39
  • 1
    If your model is to start a thread, then immediately wait for that thread to finish, then you're almost always better off just executing the code directly, without launching a separate thread. It's a common mistake but threads don't automatically make things go faster or work better, there has to be some exploitable concurrency for a thread to work better, and that often means careful design up front. – markspace Jul 01 '18 at 18:42
  • mhh okay i thought the thread pool would wait until completion of one task and then start the other one right away thanks! – Selfie21 Jul 01 '18 at 21:18
  • If your thread pool has only one thread, then that thread will execute one task at a time and not embark on the next until the first is complete. In this you were correct. If the pool has two or more threads, each thread will execute a task without waiting for any task to complete first. This is what may give the speed-up that is the usual reason for using a thread pool. – Ole V.V. Jul 02 '18 at 06:53

1 Answers1

0

The thing ThreadPoolExecutor shutdown and task execution/draining work queue/taking from work queue is racy. So you cannot rely on thread interruptiong mechanism or something else. All you are guaranteed with is:

Initiates an orderly shutdown in which previously submitted tasks are executed, but no new tasks will be accepted. Invocation has no additional effect if already shut down.

This method does not wait for previously submitted tasks to complete execution.

To dig a bit deeper into the ThreadPoolExecutor implementation lets take a look at the main execution method:

final void runWorker(Worker w) {
    Thread wt = Thread.currentThread();
    Runnable task = w.firstTask;
    w.firstTask = null;
    w.unlock(); // allow interrupts
    boolean completedAbruptly = true;
    try {
        while (task != null || (task = getTask()) != null) {
            w.lock();
            // If pool is stopping, ensure thread is interrupted;
            // if not, ensure thread is not interrupted.  This
            // requires a recheck in second case to deal with
            // shutdownNow race while clearing interrupt
            if ((runStateAtLeast(ctl.get(), STOP) ||
                 (Thread.interrupted() &&
                  runStateAtLeast(ctl.get(), STOP))) &&
                !wt.isInterrupted())
                wt.interrupt();
            try {
                beforeExecute(wt, task);
                Throwable thrown = null;
                try {
                    task.run();
                } catch (RuntimeException x) {
                    thrown = x; throw x;
                } catch (Error x) {
                    thrown = x; throw x;
                } catch (Throwable x) {
                    thrown = x; throw new Error(x);
                } finally {
                    afterExecute(task, thrown);
                }
            } finally {
                task = null;
                w.completedTasks++;
                w.unlock();
            }
        }
        completedAbruptly = false;
    } finally {
        processWorkerExit(w, completedAbruptly);
    }
}

The crucial part here is calling to getTask(). Its fragment is:

 if (rs >= SHUTDOWN && (rs >= STOP || workQueue.isEmpty())) {
     decrementWorkerCount();
     return null;
 }

The method is not synchronized and relies only on ordering supplied by CAS'ed ctl value. ctl here is the global pool state stored inside AtomicInteger (for non-blocking atomic ThreadPoolExecutor state acquiring).

So the following case is possible.

  1. Worker thread called getTask
  2. Worker thread acquired run state of the pool. It is still RUNNING.
  3. Another thread initiated an order shutdown and modify ctl accordingly.
  4. Worker thread already took a task from the work queue.
St.Antario
  • 26,175
  • 41
  • 130
  • 318