4

I have a PriorityBlockingQueue. A single thread consumes one message at a time from this queue and processes it. Several further threads are inserting messages into the queue. The producer threads assign an integral priority to each message they submit. A static AtomicLong is used to assign every message a unique, monotonically incrementing ID. The Comparator of the queue orders messages by this priority first, and then equal priority messages are ordered by ID (lowest ID first.)

Problem: Sometimes one producer submits a large number of messages. This then starves the other producers of having their messages processed. What I'd like to do, is have the consumer round-robin between producers for equal priority messages (while still processing equal priority messages of a single producer in submission order). But I cannot work out how to write the Comparator to do this.

Another alternative I considered is having a separate queue for each producer. However, I don't think that could work, since I'm not aware of any way for single thread to wait on multiple queues.

Simon Kissane
  • 4,373
  • 3
  • 34
  • 59

4 Answers4

3

I feel like it's more straightforward to implement this with one Queue for each producer. One thread can't wait on multiple Queues, but you could combine all of the Queues into one helper class so that it doesn't need to.

import java.util.ArrayList;
import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.NoSuchElementException;
import java.util.Queue;
import java.util.concurrent.PriorityBlockingQueue;
import java.util.concurrent.locks.Condition;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;

import javax.annotation.concurrent.GuardedBy;

public class RoundRobin<P, E> {
    private final Lock lock = new ReentrantLock();
    private final Condition added = lock.newCondition();

    @GuardedBy("lock") private final Map<P, Queue<E>> queues = new LinkedHashMap<>();

    public boolean add(P producer, E item) {
        lock.lock();
        try {
            if (!queues.containsKey(producer)) {
                queues.put(producer, new PriorityBlockingQueue<>());
            }

            added.signalAll();
            return queues.get(producer).add(item);
        } finally {
            lock.unlock();
        }
    }

    public Iterator<E> roundRobinIterator() {
        return new Iterator<E>() {
            private Iterator<? extends Queue<E>> i = null;
            private boolean singlePass = true;

            @Override
            public boolean hasNext() {
                return true;
            }

            @Override
            public E next() {
                lock.lock();
                try {
                    while (true) {
                        if (i == null || !i.hasNext()) {
                            i = queues.values().iterator();
                            singlePass = true;
                        }

                        while (i.hasNext()) {
                            Queue<E> q = i.next();
                            if (!q.isEmpty()) {
                                if (singlePass) {
                                    // copy the iterator to prevent
                                    // ConcurrentModificationExceptions
                                    singlePass = false;
                                    i = copy(i);
                                }
                                return q.poll();
                            }
                        }

                        if (singlePass) {
                            // If singlePass is true then we just checked every
                            // queue and they were all empty.
                            // Wait for another element to be added.
                            added.await();
                        }
                    }
                } catch (InterruptedException e) {
                    throw new NoSuchElementException(e.getMessage());
                } finally {
                    lock.unlock();
                }
            }

            private <T> Iterator<? extends T> copy(Iterator<? extends T> i) {
                List<T> copy = new ArrayList<>();
                while (i.hasNext()) {
                    copy.add(i.next());
                }
                return copy.iterator();
            }
        };
    }
}
Jeffrey
  • 44,417
  • 8
  • 90
  • 141
  • I like this idea (in particular use of ReenterantLock and Condition). I'm going to try something similar. – Simon Kissane Jan 03 '15 at 12:22
  • 1
    Since you've got a ReentrantLock on the outside, you don't need a PriorityBlockingQueue on the inside, a PriorityQueue will be fine - double locking would give worse performance and greater risk of deadlocks (not with your current code, but possible with future changes). – Simon Kissane Jan 03 '15 at 23:29
  • Also, your solution has a separate priority queue for each producer, when I need a higher priorities to apply across messages of different producers. You have LinkedHashMap; I ended up doing essentially Map>. I'm accepting your answer since it got me thinking on the right track and is the closest to what I ended up actually doing. – Simon Kissane Jan 03 '15 at 23:30
2

I think I would do something like that:

import java.util.ArrayList;
import java.util.LinkedList;
import java.util.List;
import java.util.Queue;

public class RRQueue<M> {
    private final ThreadLocal<Queue<M>> threadQueue = new ThreadLocal<>();
    private final List<Queue<M>> queues;
    private int current = 0;

    public RRQueue() {
        this.queues = new ArrayList<>();
    }

    public synchronized void add(M msg) {
        Queue<M> queue = threadQueue.get();
        if (queue == null) {
            queue = new LinkedList<>(); // or whatever
            queues.add(queue);
            threadQueue.set(queue);
        }
        queue.add(msg);
        notify();
    }

    public synchronized M get() throws InterruptedException {
        while (true) {
            for (int i = 0; i < queues.size(); ++i) {
                Queue<M> queue = queues.get(current);
                current = (current+1)%queues.size();
                if (!queue.isEmpty()) {
                    return queue.remove();
                }
            }
            wait();
        }
    }
}
Maurice Perry
  • 32,610
  • 9
  • 70
  • 97
0

It's all in how you allocate the IDs. Allocate them N apart, where N is the number of producers, and add each producer's index to that. Then reading them sequentially will yield a round-robin order. You'll have a tiny bit of bookkeeping to do to know when to increment the underlying ID, which will happen when you reach Nx-1 for any x.

user207421
  • 305,947
  • 44
  • 307
  • 483
  • With two producers, are you saying to allocate even IDs to the first producer, odd IDs to the second? Suppose producer #1 submits 10 messages, which are fully processed - it consumes the first ten odd IDs. Some time later, producer #1 submits 10 messages, which get the next ten odd IDs. Simultaneously, producer #2 submits 10 messages, which consume the first ten even IDs. Won't this result in producer #2's first batch being processed before producer #1's second batch? That's not round robin of outstanding messages. – Simon Kissane Jan 02 '15 at 07:24
  • No. You would have incremented the underlying ID as per my last sentence, so in your example the IDs would run 0,2,4,6,8,10,12,14,16,18,19, ... – user207421 Jan 02 '15 at 07:35
  • So you would have a counter, which increments by N (where N is current producer count), and an array sized N of boolean flags, initially all false. When producer x requests an ID, if flags[x] is false, set flags[x]=true; if flags[x] is true, increment counter by N, set all flags to false, set flags[x]=true; then, in either case, return counter+x? I assume also a synchronised block to protect the counter and the array. Also, to change producer count dynamically, would need to resize array, setting all its values to false, incrementing counter. – Simon Kissane Jan 02 '15 at 07:48
  • I haven't thought out the details of the bookkeeping, and that's not necessarily the only way to do it, but the sequence I gave is the one you wanted, yes? – user207421 Jan 02 '15 at 07:49
  • Suppose producer #1 submits 100 messages. So far 50 of them have been processed. Now producer #2 submits 100 messages. Due to incrementing the underlying ID, won't all of producer #2's messages have higher ID numbers than producer #1's, so none of them will be processed until producer #1's are done? That isn't round-robin. – Simon Kissane Jan 03 '15 at 05:54
0

A round robin among N producers can be achieved using a PriorityBlockingQueue as follows.

Maintain N AtomicInteger counters for each of the producers and a global counter as a tie breaker in case the producer counters are equal.

When adding to the Q incement counter for the producer and also global counter and store into Q object. The comparator for Q will order based on Producer counter 1st and then the global counter value stored in the Q object.

However when objects of one producer is empty for sometime in the Q the corresponding counter falls behind and it will start hogging the Q when objects start coming in.

To avoid this maintain a volatile variable which is updated with the producer counter of the object when de-queued. After incrementing the producer counter during en-queue if the value is less than the last de-queued counter in volatile variable reset the counter to that value + 1.

Suresh Mahalingam
  • 95
  • 4