Using variable number of threads in Java

sstows 0
Viewed 52

I'm trying to write some code in java that uses a variable number 'p' of threads to calculate the average of elements in an array.

I created this class to represent each thread:

class Worker extends Thread {
    int[] a;
    int low;
    int upp;
    double avg = 0;
    public Worker(int[] a, int low, int upp){
        this.a = a;
        this.low = low;
        this.upp = upp;
    }
    public void run(){
        int sum = 0;
        for (int i = low; i < upp; i++){
            sum+=a[i];
        }
        avg = (double)sum/a.length;
    }
}

And this is the code to calculate the average:

static double parallelaverage(int a[], int p) {
    int avg = 0;
    int num = a.length/p;
    ArrayList<Worker> threads = new ArrayList<Worker>();
    for (int i = 0; i<a.length; i+=num){
        if (i+num > a.length){
            Worker x = new Worker(a, i, a.length);
            x.start();
            threads.add(x);
        }
        else{
            Worker x = new Worker(a, i, i+num);
            x.start();
            threads.add(x);
            
        }
    }
    try{for (Worker thread: threads){
        thread.join();}
    } catch (Exception e){}
    for (Worker thread: threads){
        avg += thread.avg;
    }
    return avg;
}

This seems to make sense to me, but the actual result I get is always way off what would be the actual average. What's wrong with my code?

multithreading java concurrency
3 Answers

Your problem is the actual way types require to do arithmetic in the language. All numbers used in an equation must be the same type, However, this is critical for divisions of the outcome may be required to be a float or double!

// avg = (double)sum/a.length;
// Should be done as
double avg = (new Integer(sum).doubleValue()/new Integer(a.length).doubleValue());

You will often get compiler warnings about conversion of "loss of precision" when using multiple types in an equation when the language will allow it.

Samuel Marchant0

The 2 Ideas On How We Can Improve the quality of your Existing Code.

Personal Note: There were a few mistakes in your code, I got the idea of what you were trying to achieve and have done it a bit better way.

1. Rely on ExecutorService.

Why don't you take advantage of executor service, you don't have to manually control the threads.

class Scratch {
    public static void main(String[] args) throws ExecutionException, InterruptedException {
        System.out.println(parallelAverage(new int[]{1, 2, 3, 4, 5, 6, 7, 8, 9, 10,11}, 2));

    }

    static double parallelAverage(int a[], final int perSlice) throws ExecutionException, InterruptedException {
        ExecutorService executorService = Executors.newFixedThreadPool(a.length / perSlice);
        List<Future<Double>> futureList = new ArrayList<>();

        int start, end;
        start = end = 0;
        for (; ; ) {
            if (a.length <= end) {
                break;
            }
            start = end;
            end = Math.min((int) (end + perSlice), a.length);
            futureList.add(executorService.submit(new Worker(a, start, end)));
        }


        double avg = 0;
        for (Future<Double> doubleFuture : futureList) {
            Double aDouble = doubleFuture.get();
            avg += aDouble;
        }
        executorService.shutdown();
        return avg;
    }
}

2. Use Callable Rather Than Runnable For Returning Values.

And also in your Worker class, rather than using a Runnable interface a better approach would be to use a Callable interface. This allows you to return a value(Future).

final class Worker implements Callable<Double> {
    int[] a;
    int low;
    int upp;

    public Worker(int[] array, final int startInclusive, final int endExclusive) {
        this.a = array;
        this.low = startInclusive;
        this.upp = endExclusive;
    }

    @Override
    public Double call() throws Exception {
        int sum = 0;
        for (int i = low; i < upp; i++) {
            sum += a[i];
        }
        return (double) sum / a.length;
    }
Zahid Khan0

The problem is how you initialize the avg variable:

int avg = 0;

It's an int, every time you add Worker.avg to it, which is double the decimal part is being lost, 0 + 0.3 = 0, 1 + 2.7 = 3, and so on. You can easily observe the behaviour with a debugger.

You need to initialize it as double:

double avg = 0;

Additional note, this is code duplication and somewhat unclear:

if (i+num > a.length){
     Worker x = new Worker(a, i, a.length);
     x.start();
     threads.add(x);
}
else{
     Worker x = new Worker(a, i, i+num);
     x.start();
     threads.add(x);
}

A better way to write it would be using Math.min():

int upp = Math.min(i + num, a.length);
Worker x = new Worker(a, i, upp);
x.start();
threads.add(x);
Chaosfire0
Related