How can I prevent my UDP keepalive from using 100% CPU?

Question

I have a Runnable that watches for data to send out UDP as well as to send a keep alive every 10 seconds. The process is taking 100% CPU. I tried setting the thread to low priority but didn't seem to make any difference.

private Runnable keepAliveRunnable = new Runnable() {

    long nextSend = 0;
    byte[] sendData;

    @Override
    public void run() {
        if(DEBUG)
            System.out.println("Starting keepAlive.");
        while (socket != null) {
            synchronized (socketLock) {
                try {
                    sendData = sendQueue.poll();
                    if (sendData != null) {
                        socket.send(new DatagramPacket(sendData, sendData.length,
                                InetAddress.getByName(Main.ipAddress), 10024));
                    } else if (nextSend < System.currentTimeMillis()) {
                        if(DEBUG && nextSend < System.currentTimeMillis())
                            System.out.println("Update keepAlive.");
                        // Send /xremote
                        socket.send(new DatagramPacket(("/xremote").getBytes(),
                                ("/xremote").getBytes().length,
                                InetAddress.getByName(Main.ipAddress), 10024));
                        nextSend = System.currentTimeMillis() + keepAliveTimeout;
                        // Send /info
                        socket.send(new DatagramPacket(("/info").getBytes(),
                                ("/info").getBytes().length,
                                InetAddress.getByName(Main.ipAddress), 10024));
                    }
                } catch (IOException e) {
                    e.printStackTrace();
                    if(!e.getMessage().contains("Socket closed")) {
                        e.printStackTrace();
                    }
                }
            }
        }
        System.out.println("keepAliveRunnable ended.");
    }
};

You code needs to have some place where it waits for something to do when it has nothing to do. — David Schwartz, Aug 26 '15 at 21:51

score 2 · Accepted Answer · edited Jun 20 '20 at 09:12

Make `sendQueue` a `LinkedBlockingQueue`, and use poll timeouts.

You are busy waiting, which essentially forces your app to keep running the same logic over and over instead of giving the CPU back to the system.

Don't count on your own implementation of checking the time, that is unreliable and can result in what you're seeing. Instead, use blockingQueue.poll(10, TimeUnit.SECONDS), which automatically handles returning the CPU to the system.

I made a few other changes to your code; I put the duplicated packet construction code in a separate method, and I wrapped the synchronization of the socket only when the socket is actually being used. Notice how much cleaner it is when you let the queue do the work for you.

while(socket != null) {
  try {
    sendData = sendQueue.poll(10, TimeUnit.SECONDS);
    if (sendData != null) {
      sendPacket(sendData);
    } else {
      sendPacket("/xremote".getBytes());
      sendPacket("/info".getBytes());
    }
  } catch (IOException e) {
    e.printStackTrace();
    if (!e.getMessage().contains("Socket closed")) {
      e.printStackTrace();
    }
  }
}

And here's sendPacket:

private static void sendPacket(byte[] data) throws UnknownHostException, IOException {
  // Note, you probably only have to do this once, rather than looking it up every time.
  InetAddress address = InetAddress.getByName(Main.ipAddress);
  DatagramPacket p = new DatagramPacket(data, data.length, address, 10024);
  synchronized(socketLock) {
    socket.send(p);
  }
}

score 1 · Answer 2 · answered Aug 26 '15 at 20:35

You should add a Thread.sleep() at the bottom of your while loop, to slow down your loop. As is, you're busy-waiting and churning the CPU while you wait for the nextSend time to be reached. Thread.sleep() will actually pause the thread, allowing other threads and processes to use the CPU while this one sleeps.

Sleeping for a 10th of a second (100 milliseconds) should be a good amount of time to sleep between iterations of your loop, if your goal is to actually do work every 10 seconds.

There are more advanced techniques for dispatching work every so often, like ScheduledExecutorService, which you could also consider using. But for a small application the pattern you're using is fine, just avoid busy waiting.

score 0 · Answer 3 · answered Aug 26 '15 at 21:01

I think rather than polling your sendqueue, its better to use semaphore signal and wait.

When a packet is inserted in sendqueue, call semaphore signal. Use semaphore wait instead of call to sendqueue.poll().

I assume you have separate threads for pushing popping data from sendqueue.

This is standard consumer producer problem. https://en.wikipedia.org/wiki/Producer%E2%80%93consumer_problem

score 0 · Answer 4 · answered Aug 26 '15 at 21:48

After digging through my code, I had realized that over time I had whittled down the number of processes sending data to 1 (duh) so I really didn't need the runnable as I could just send the data directly. I also set up a separate runnable and used ScheduledExecutor. I thought I would just put that here for other to see. Durron597's code is a little prettier but since I'm only sending two packs now I decided to just put the code together.

// In main
pingXAir();


private void pingXAir() {
    System.out.println("Start keepAlive");
    ScheduledExecutorService executorService = Executors.newScheduledThreadPool(1);
    executorService.scheduleAtFixedRate(keepAliveRunnable, 0, 5, TimeUnit.SECONDS);
}

private Runnable keepAliveRunnable = new Runnable() {

    @Override
    public void run() {
        synchronized (socketLock) {
            try {
                if (DEBUG)
                    System.out.println("Update keepAlive.");

                // Send /xremote
                socket.send(new DatagramPacket(("/xremote").getBytes(),
                        ("/xremote").getBytes().length,
                        InetAddress.getByName(Main.ipAddress), 10024));

                // Send /info
                socket.send(new DatagramPacket(("/info").getBytes(),
                        ("/info").getBytes().length,
                        InetAddress.getByName(Main.ipAddress), 10024));

            } catch (IOException e) {
                e.printStackTrace();
                if (!e.getMessage().contains("Socket closed")) {
                    e.printStackTrace();
                }
            }
        }
    }
};

How can I prevent my UDP keepalive from using 100% CPU?

4 Answers4

Make sendQueue a LinkedBlockingQueue, and use poll timeouts.

Make `sendQueue` a `LinkedBlockingQueue`, and use poll timeouts.