4
\$\begingroup\$

I've a application which runs with two threads. The main thread runs in one infinity-while-loop:

while (true) {
   try {
      ArrayList<Info> infoList = zabbixHandler.APIRequest();
      handleInfo.handleInformation(infoList);
      Thread.sleep(5000);
   }
}

The second thread, which uses more than 50% of my CPU, must be able to stop and run again, which I asked already with this question on Stack Overflow.

The solution works perfectly, but as I already said, it uses >50% of the CPU. Here are the two loops of the solution:

private volatile boolean finished = true;
public void run() {
   Process p;

   while (true) {
        // loop until the thread is finished
        while (!finished) {
            try {

                // let every lamp shine...
                p = Runtime.getRuntime().exec(onCommand + "-green");
                p.waitFor();

                // ... for 0.5 seconds ...
                Thread.sleep(500);

                // ... then turn it off again ...
                p = Runtime.getRuntime().exec(offCommand + "-green");
                p.waitFor();
                // ... and let the next lamp shine
                p = Runtime.getRuntime().exec(onCommand + "-yellow");
                p.waitFor();

                Thread.sleep(500);

                p = Runtime.getRuntime().exec(offCommand + "-yellow");
                p.waitFor();
                p = Runtime.getRuntime().exec(onCommand + "-red");
                p.waitFor();

                Thread.sleep(500);

                p = Runtime.getRuntime().exec(offCommand + "-red");
                p.waitFor();
                p = Runtime.getRuntime().exec(onCommand + "-blue");
                p.waitFor();

                Thread.sleep(500);

                p = Runtime.getRuntime().exec(offCommand + "-blue");
                p.waitFor();

            } catch (InterruptedException e) {
                e.printStackTrace();
            } catch (IOException e) {
                e.printStackTrace();
            }
        }
    }
}

How could I reduce the use of CPU while retaining the functionality?

EDIT:
The finish element will be changed by a getter and setter. Because it is volaitle, it shouldn't get complications.

\$\endgroup\$
8
  • \$\begingroup\$ please provide the actual code inside the second code block. \$\endgroup\$
    – Malachi
    Jan 13, 2014 at 14:25
  • \$\begingroup\$ Are you aware that Thread.sleep(milliseconds) is a rough estimation only? \$\endgroup\$
    – Bobby
    Jan 13, 2014 at 15:07
  • 2
    \$\begingroup\$ where does finished change to true? \$\endgroup\$
    – Malachi
    Jan 13, 2014 at 15:28
  • \$\begingroup\$ How are you measuring that the second thread takes > 50% of the time? \$\endgroup\$
    – rolfl
    Jan 13, 2014 at 16:04
  • \$\begingroup\$ -1 vote because you are not showing us the most important part of the code: how does the variable finished get declared, and how is it modified. Will remove -1 when question is corrected. \$\endgroup\$
    – rolfl
    Jan 13, 2014 at 16:10

2 Answers 2

9
\$\begingroup\$

don't use a busy wait but instead use a wait condition (here using the built-in monitor in Object):

public void run() {
   while(true){
      while(!finished){
         // do stuff
      }
      try{
          synchronized(this){
              while(finished)
                   wait();//wait until notify gets called in startThread
          }
      }catch(InterruptedException e){}
   }

}

public synchronized void startThread() {
   finished = false;
   notify();//wake up the wait
}

I note that finished should remain volatile.

I put the wait() in the synchronized block in a while loop to avoid the race where the thread has just tested the first while condition and is about to enter the synchronized block but startThread() then gets called; putting finished back to false and the thread waiting anyway. This can lead to the thread blocking while waiting on a notify() that never happens.

To help any more I need to see what exactly happens inside that while loop.

\$\endgroup\$
6
  • \$\begingroup\$ This answer makes no sense to me.... where is there a spin lock? \$\endgroup\$
    – rolfl
    Jan 13, 2014 at 16:01
  • 1
    \$\begingroup\$ @rolfl when finished is true the thread keeps spinning in the outer while but never getting in the inner while, this is equivalent to while(true){while(finished){}/*do stuff*/} \$\endgroup\$ Jan 13, 2014 at 16:04
  • 2
    \$\begingroup\$ Ahh, that makes sense, but why didn't you say that in your answer? ... ;-) \$\endgroup\$
    – rolfl
    Jan 13, 2014 at 16:07
  • \$\begingroup\$ Assuming you are right about the finished variable (which I suspect you are), then the better/right/modern solution would be using Reentrant-lock and conditions. \$\endgroup\$
    – rolfl
    Jan 13, 2014 at 16:12
  • \$\begingroup\$ @rolfl maybe but I think that's a bit overkill in this case \$\endgroup\$ Jan 13, 2014 at 16:17
6
\$\begingroup\$

The right tool for this job in Java6 and newer is the combination of a ReentrantLock and a Condition

EDIT: You should also consider changing your Process-running code.... consider creating a function:

private static void runAndWaitFor(String command) {
    Process p = Runtime.getRuntime().exec(command);
    p.waitFor();
}

Then, in your blinking loop, you can reduce your code duplication with:

            // let every lamp shine...
            runAndWaitFor(onCommand + "-green");

            // ... for 0.5 seconds ...
            Thread.sleep(500);

            // ... then turn it off again ...
            runAndWaitFor(offCommand + "-green");
            // ... and let the next lamp shine
            runAndWaitFor(onCommand + "-yellow");

            ......  etc. .....

Now, even with the simpler code, you still have the two threads, one needs to wait until a condition is met, and when the condition is met, it needs to execute some work until the condition is reverted.

In your 'working' thread you want a method to call workStarted() which will start the lights blinking, and then workCompleted() which will stop the lights.

The pattern to use is:

import java.util.concurrent.locks.Condition;
import java.util.concurrent.locks.ReentrantLock;


public class ThreadBlocker implements Runnable {

    private final ReentrantLock mylock = new ReentrantLock();
    private final Condition workready = mylock.newCondition();
    private boolean finished = true;

    public void workStarted() {
        mylock.lock();
        try {
            finished = false;
            workready.signalAll();
        } finally {
            mylock.unlock();
        }
    }

    public void workCompleted() {
        mylock.lock();
        try {
            finished = true;
        } finally {
            mylock.unlock();
        }
    }

    private void waitForWork() throws InterruptedException {
        mylock.lock();
        try {
            while (finished) {
                workready.await();
            }
        } finally {
            mylock.unlock();
        }
    }

    public void run () {
        try {
            while (true) {
                // the following method will block unless there is work to do.
                waitForWork();

                // change your lights in here.

            }
        } catch (InterruptedException ie) {
            // do some handling for the thread.
            ie.printStackTrace();
            Thread.currentThread().interrupt();
        }
    }



}
\$\endgroup\$
3
  • \$\begingroup\$ you don't need to signalAll in workCompleted() \$\endgroup\$ Jan 13, 2014 at 16:48
  • \$\begingroup\$ is this sort of like having two objects where one goes and when it's done it starts (calls) the other, and when the other is done it starts (calls) the first? creating it's own loop \$\endgroup\$
    – Malachi
    Jan 13, 2014 at 16:59
  • \$\begingroup\$ Not really, this is like a motion-sensitive security light. If there is motion, it turns the light on for a few seconds. At the end of that time, if there is ongoing motion, it stays on for a few more seconds. If there is no motion, it turns off. \$\endgroup\$
    – rolfl
    Jan 13, 2014 at 17:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.