views:

132

answers:

4

I am creating a graphing calculator.

In an attempt to squeeze some more performance out of it, I added some multithreaded to the line calculator. Essentially what my current implementation does is construct a thread-safe Queue of X values, then start however many threads it needs, each one calculating a point on the line using the queue to get its values, and then ordering the points using a HashMap when the calculations are done. This implementation works great, and that's not where my race condition is (merely some background info).

In examining the performance results from this, I found that the HashMap is a performance bottleneck, since I do that synchronously on one thread. So I figured that ordering each point as its calculated would work best. I tried a PriorityQueue, but that was slower than the HashMap.

I ended up creating an algorithm that essentially works like this:

I construct a list of X values to calculate, like in my current algorithm.

I then copy that list of values into another class, unimaginatively and temporarily named BlockingList, which is responsible for ordering the points as they are calculated.

BlockingList contains a put() method, which takes in two BigDecimals as parameters, the first the X value, the second the calculated Y value. put() will only accept a value if the X value is the next one on the list to be accepted in the list of X values, and will block until another thread gives it the next excepted value.

For example, since that can be confusing, say I have two threads, Thread-1 and Thread-2. Thread-2 gets the X value 10.0 from the values queue, and Thread-1 gets 9.0. However, Thread-1 completes its calculations first, and calls put() before Thread-2 does. Because BlockingList is expecting to get 10.0 first, and not 9.0, it will block on Thread-1 until Thread-2 finishes and calls put(). Once Thread-2 gives BlockingList 10.0, it notify()s all waiting threads, and expects 9.0 next. This continues until BlockingList gets all of its expected values.

(I apologise if that was hard to follow, if you need more clarification, just ask.)

As expected by the question title, there is a race condition in here. If I run it without any System.out.printlns, it will sometimes lock because of conflicting wait() and notifyAll()s, but if I put a println in, it will run great.

A small implementation of this is included below, and exhibits the same behavior:

import java.math.BigDecimal;
import java.util.concurrent.ConcurrentLinkedQueue;

public class Example {
    public static void main(String[] args) throws InterruptedException {
        // Various scaling values, determined based on the graph size
        // in the real implementation
        BigDecimal xMax = new BigDecimal(10);
        BigDecimal xStep = new BigDecimal(0.05);

        // Construct the values list, from -10 to 10
        final ConcurrentLinkedQueue<BigDecimal> values = new ConcurrentLinkedQueue<BigDecimal>();

        for (BigDecimal i = new BigDecimal(-10); i.compareTo(xMax) <= 0; i = i.add(xStep)) {
            values.add(i);
        }

        // Contains the calculated values
        final BlockingList list = new BlockingList(values);

        for (int i = 0; i < 4; i++) {
            new Thread() {
                public void run() {
                    BigDecimal x;

                    // Keep looping until there are no more values
                    while ((x = values.poll()) != null) {
                        PointPair pair = new PointPair();
                        pair.realX = x;

                        try {
                            list.put(pair);
                        } catch (Exception ex) {
                            ex.printStackTrace();
                        }
                    }
                }
            }.start();
        }
    }

    private static class PointPair {
        public BigDecimal realX;
    }

    private static class BlockingList {
        private final ConcurrentLinkedQueue<BigDecimal> _values;
        private final ConcurrentLinkedQueue<PointPair> _list = new ConcurrentLinkedQueue<PointPair>();

        public BlockingList(ConcurrentLinkedQueue<BigDecimal> expectedValues) throws InterruptedException {
            // Copy the values into a new queue
            BigDecimal[] arr = expectedValues.toArray(new BigDecimal[0]);

            _values = new ConcurrentLinkedQueue<BigDecimal>();
            for (BigDecimal dec : arr) {
                _values.add(dec);
            }
        }

        public void put(PointPair item) throws InterruptedException {
            while (item.realX.compareTo(_values.peek()) != 0) {
                synchronized (this) {
                    // Block until someone enters the next desired value
                    wait();
                }
            }

            _list.add(item);
            _values.poll();

            synchronized (this) {
                notifyAll();
            }
        }
    }
}

My question is can anybody help me find the threading error?

Thanks!

A: 

Does it work if you put synchronized in front of the entire BlockingList.put method?

What if thread A executes

while (item.realX.compareTo(_values.peek()) != 0) {

Thread B goes on and does

_list.add(item);
_values.poll();

synchronized (this) {
    notifyAll();
}

Thread A does

synchronized (this) {
    // Block until someone enters the next desired value
    wait();
}
aioobe
No, unfortunately, but see the comments on the question, I used tzaman's solution for this problem, which is much better than my convoluted one.
nasufara
updated the answer. What do you think? (Even if you updated to another solution, the race-condition issue is still a relevant question)
aioobe
Yeah, it does seem that would be where the race condition is, Thread B calls `notifyAll()` before thread A can call `wait()`. I suppose making it `wait(10)` or some other arbitrary number would work better, so that it checks after a bit.
nasufara
+3  A: 

You're adding the threading to improve performance, but in doing so have introduced a performance bottleneck.

You write:

I figured that ordering each point as its calculated would work best

Let's evaluate the performance cost of doing this.

Adding a single point to an already sorted list can be done very quickly.

With a BubbleSort algorithm, this will take O(N) time - linear time proportional to the size of the list already present. Repeating this for every point you're adding means that you repeat this O(N) operation N times, giving O(N^2) performance. Running this over many cores might reduce the wall clock time, but the killer is the N^2 performance.

Unfortunately, Java doesn't use the BubbleSort algorithm internally - it uses the QuickSort, which is generally better behaved. For adding a single number to an already sorted list, QuickSort takes O(N lg N) time; repeating this N times gives O(N^2 lg N) performance. Double ouch.

Resorting a list many times is inefficient - if you instead just gathered all the items together and sorted them once at the end, sorting will cost you just O(N lg N) time, giving you a much better end result.

Note that having multiple cores will just scale the results by a constant factor, it doesn't change the overall analysis. Going multithreaded can improve performance markedly, so you're going the right way, but always measure the results.

In examining the performance results from this, I found that the HashMap is a performance bottleneck, since I do that synchronously on one thread

Instead of using a Hashmap, try using a simple list to gather the points together - adding a point to a list is near-constant, near-zero time, as long as you set the list capacity high enough in advance.

Bevan
Indeed, using an array worked much better, since I already know how many points I will have, and what order they're in. I'll mark this as the answer unless tzaman wants to put his into an answer. Thanks!
nasufara
+2  A: 

You have to realise why Java requires wait() and notify() to be synchronized.

Imagine you are a thread, and have just completed this.

while (item.realX.compareTo(_values.peek()) != 0) {
            synchronized (this) {
                // Block until someone enters the next desired value
                wait();
            }
        }

You had the lock, released it for the wait, then get it back when the wait finished. Then - you gave it up again!

        _list.add(item);
        _values.poll();

During this time, your other threads have access to the lock. Then you go on and re-obtain a lock in order to poll the list and perform the actual 'put'.

        synchronized (this) {
            notifyAll();
        }

The point of the 'synchronized' is for you to keep the lock while you make the changes - you should put the 'wait', 'add' and 'poll' statements within the same 'synchronized' statement.

Sanjay Manohar
A: 

The best way to understand race condtion in threads and how to resolve it, you should read this book:

Blockquote

Java.Concurrency.in.Practice.(2006)

Shashank T