2
\$\begingroup\$

The purpose of a rhythmic task controller is to accept a task and execute all the queued tasks in a cycle or "rhythm" defined by the client code. A task returns true or false depending on whether or not it is "finished". A finished task should be removed from the controller's control and won't be executed again while an unfinished task should be executed on the next cycle.

I don't think my solution is as well optimized as it could be, and this is a solution that needs to be as fast as possible as it serves as the core task controller in my game framework!

import static java.util.concurrent.Executors.newSingleThreadScheduledExecutor;

import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentLinkedQueue;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;

public final class RhythmicTaskController implements TaskController, Runnable {

    private final ExecutorService service;
    private final ConcurrentLinkedQueue<Task> queue = new ConcurrentLinkedQueue<>();

    public RhythmicTaskController(int threads, int delay, TimeUnit unit) {
        service = Executors.newFixedThreadPool(threads);

        newSingleThreadScheduledExecutor().scheduleWithFixedDelay(this, delay, delay, unit);
    }

    @Override
    public void offer(Task task) {
        queue.offer(task);
    }

    @Override
    public void run() {
        int initial = queue.size();
        ConcurrentMap<Future<Boolean>, Task> futures = new ConcurrentHashMap<>(initial);
        queue.forEach(task -> futures.put(service.submit(task::finish), task));
        futures.forEach((r, task) -> {
            try {
                if (r.get())
                    queue.remove(task);
            } catch (Exception e) {
                e.printStackTrace();
            }
        });
    }

}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ while (futures.size() < initial); is really bad style. You should never use an empty while loop to wait for something. It will load one of your CPU cores to 100% for that time. Plus I don't see how futures.size() could ever be less than initial since you never remove anything from the map. \$\endgroup\$
    – Tesseract
    Apr 5, 2015 at 22:47
  • \$\begingroup\$ Thanks for the loop advice. My method is poorly engineered, I can see what you mean. The reason being is that I want tasks to be removed from the queue if they return true for their result. I don't know how to do this concurrently, so that's what I need help with. \$\endgroup\$
    – Jire
    Apr 5, 2015 at 22:59

2 Answers 2

1
\$\begingroup\$

and this is a solution that needs to be as fast as possible as it serves as the core task controller in my game framework!

Not necessarily. It all depends on how much a single task takes on the average. Unless your tasks are very fast, fooling around with their queue doesn't matter.


@Override
public void offer(Task task) {
    queue.offer(task);
}

Don't use offer without checking the return value. It's fine as long as you use ConcurrentLinkedQueue, but when you switch implementations, it stops working without any warning. Use something like

com.google.common.base.Verify(queue.offer(task));

Unless removals are common, I guess that your original implementation is faster than SpiderPig's solution removing and re-adding tasks on each iterations. Usually, iterating a collection is faster than modifying it (twice). But it's just a guess... measuring would tell you, but measuring Java performance is hard.

\$\endgroup\$
2
  • \$\begingroup\$ Or he could just call queue.add instead which would throw an exception. Also I believe that adding and removing elements from a queue is faster than populating a hash map which he did in his original implementation. \$\endgroup\$
    – Tesseract
    Apr 6, 2015 at 13:36
  • \$\begingroup\$ @SpiderPig I see, I overlooked the HashMap. I guess, your solution is better then. \$\endgroup\$
    – maaartinus
    Apr 6, 2015 at 21:46
1
\$\begingroup\$

You could remove all elements from the queue and then offer them back if necessary.

public void run() {
    ArrayList<Task> tasks = new ArrayList<>();
    {
      Task task;
      while((task = queue.poll()) != null) {
        tasks.add(task);
      }
    }
    for(Task task: tasks) {
      service.submit(() -> {
        if(!task.finish()) offer(task);
      });
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ I used your code and I think this is more optimized: pastebin.com/gVS7RMn1 What do you think? \$\endgroup\$
    – Jire
    Apr 6, 2015 at 3:09
  • 1
    \$\begingroup\$ Your application is multithreaded. So in between calling isEmpty and poll the queue might be changed by another thread. However as long as you can be sure that non of those other threads will remove elements, it should be fine. But it won't run faster. So it doesn't give you any advantage while having the disadvantage of potentially malfunctioning due to interactions with other threads. \$\endgroup\$
    – Tesseract
    Apr 6, 2015 at 3:26
  • 1
    \$\begingroup\$ BTW. Take a look at LinkedBlockingQueue.drainTo. That would allow you to shorten the code some more. \$\endgroup\$
    – Tesseract
    Apr 6, 2015 at 13:37

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.