I'm struggling with some exercise code that should calculate sum of squares using threads. For some reason I get inconsistent results despite the use of locks, also made sure I am locking on objects and not local items / variables etc.
When I add logging to understand the behavior, the code runs much slower and executes just fine. same goes when reducing to one thread. But - once I run multiple threads it act unpredictably. The result should be
My code:
package com.yaniv.concurrency;
import java.sql.Timestamp;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.locks.ReentrantLock;
public class SumSquaresSync implements Runnable {
ReentrantLock nextLock = new ReentrantLock();
ReentrantLock sumLock = new ReentrantLock();
Long sum = new Long(0);
int min, max;
Integer next = new Integer(0);
int threadBatchSize = 10;
static final boolean LOG = false;
@Override
public void run() {
long localSum = 0;
int[] batch = new int[threadBatchSize];
while (next <= max) {
nextLock.lock();
if (this.next <= max) {
{
for (int i = 0; i < threadBatchSize; i++) {
batch[i] = ((next + i <= max) ? (next + i) : 0);
}
next += threadBatchSize;
}
}
nextLock.unlock();
if (LOG) {
synchronized (System.out) {
System.out.print(Thread.currentThread().getName() + " got batch " + batch.toString() + ": ");
for (int i = 0; i < threadBatchSize; i++) {
System.out.print(batch[i] + ", ");
}
System.out.println();
}
}
for (int i : batch) {
localSum += Math.pow(i, 2);
}
}
sumLock.lock();
sum += localSum;
sumLock.unlock();
if (LOG) {
safePrintln(Thread.currentThread().getName() + " terminated, localSum = " + localSum);
}
}
private long executeSumSquares(int min, int max, int numberOfThreads, int threadBatchSize) throws Exception {
this.min = min;
this.max = max;
this.next = min;
this.threadBatchSize = threadBatchSize;
this.sum = 0L;
ExecutorService executorService = Executors.newFixedThreadPool(numberOfThreads);
for (int i = 0; i < numberOfThreads; i++) {
if (LOG) {
System.out.format("Adding thread %d%n", i);
}
executorService.execute(new SumSquaresSyncThread(this));
}
executorService.shutdown();
executorService.awaitTermination(5_000L, TimeUnit.MILLISECONDS);
return sum;
}
public static void main(String[] args) throws Exception {
SumSquaresSync SumSquaresSync = new SumSquaresSync();
long total;
int iteration = 0;
Timestamp startTime, endTime;
do {
iteration++;
startTime = new Timestamp(System.currentTimeMillis());
total = SumSquaresSync.executeSumSquares(1, 10000, 1, 5);
endTime = new Timestamp(System.currentTimeMillis());
System.out.println("==========================================");
System.out.format("Total sum: %,8d, elapsed time %d, iteration %d%n", total, (endTime.getTime() - startTime.getTime()), iteration);
System.out.println("==========================================");
} while (iteration < 10); //(total == 333383335000L);
}
public void safePrintln(String s) {
synchronized (System.out) {
System.out.println(s);
}
}
public void safePrint(String s) {
synchronized (System.out) {
System.out.print(s);
}
}
}
The results:
Total sum: 347,938,671,335, elapsed time 16, iteration 1
Total sum: 342,283,818,850, elapsed time 10, iteration 2
Total sum: 336,257,779,565, elapsed time 10, iteration 3
Total sum: 336,233,345,285, elapsed time 9, iteration 4
Total sum: 337,663,242,000, elapsed time 8, iteration 5
Total sum: 336,779,784,290, elapsed time 10, iteration 6
Total sum: 335,474,886,225, elapsed time 10, iteration 7
Total sum: 338,825,524,135, elapsed time 8, iteration 8
Total sum: 335,820,751,880, elapsed time 10, iteration 9
Total sum: 335,083,150,300, elapsed time 8, iteration 10
The correct result should be 333,383,335,000. Can anyone tell what I am missing?
(Copied from comment)
Your while (next <= max) check is not protected by a lock.
You check it again inside the lock probably for this reason, but the for (int i : batch) { localSum += Math.pow(i, 2); } part is not inside the inner condition so the localSum may get a batch added twice sometimes.
Move the for (int i : batch) loop inside the if (this.next <= max).