3
\$\begingroup\$

This bit of code initializes an array of any given size where each index corresponds to a thread and a value. That is, index 0 has 0, index 1 has 1, and so on. Then, the values, 0 to n, in the indexes are summed.

This code as part of an assignment though I'm wondering if there are any ways it could be improved? Namely, is there an alternative to using 2 for loops and calling thread.join() on them?

I'd be open to any advice and would gladly take it!

public class Processor {

    static volatile int numberOfValues = 17;
    static double processedArray[];
    static Thread threadsArray[];
    static volatile int sum;
    static Object lock1 = new Object();
    static Object lock2 = new Object();

Adding values to the array, each index in the array gets a thread to initialize that value:

    private static void initializeArray() {
        threadsArray = new Thread[numberOfValues];
        processedArray = new double[numberOfValues];

        for (int i = 0; i < threadsArray.length; i++) {
            threadsArray[i] = new Thread(new Runnable() {
                public void run() {
                    synchronize(lock1) {
                        processedArray[numberOfValues - 1] = numberOfValues;
                        numberOfValues--;
                    }

                }   
            });
            threadsArray[i].start();
        }

Here is the first for loop joining all threads after the initialization of the array:

        for (int i = 0; i < threadsArray.length; i++) {
            try {
                threadsArray[i].join();
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }

Summing the values in the array:

        for (int i = 0; i < threadsArray.length; i++) {
            threadsArray[i] = new Thread(new Runnable() {
                public void run() {
                    synchronize(lock2) {
                        sum += processedArray[numberOfValues];
                        numberOfValues++;
                    }       

                }   
            });
            threadsArray[i].start();
        }

Here is the second for loop joining all the threads after summing:

        for (int i = 0; i < threadsArray.length; i++) {
            try {
                threadsArray[i].join();
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }

    }

Main:

    public static void main(String args[]) {
        initializeArray();
        for (int i = 0; i < threadsArray.length; i++) {
            System.out.println(processedArray[i]);
        }

        System.out.println("Sum: " + sum);
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! Please tell us what the goal or assignment is. What is the expected result? Why do you want to use multithreading to achieve it? Are you using Java 8? \$\endgroup\$ – 200_success Mar 19 '16 at 15:28
  • \$\begingroup\$ @200_success Thank you for the direction, I added some more information about the goal and which part I'm curious about. \$\endgroup\$ – 1101 Mar 19 '16 at 17:01
  • 2
    \$\begingroup\$ @1101 Could you tell us what the assignment was? It's much easier to review the code when we know what it is supposed to do. \$\endgroup\$ – ThatOneGuy Mar 19 '16 at 17:39
  • \$\begingroup\$ Your added text actually isn't more informative. It just states the mechanics of what the code does (which we could get by reading the code itself). We want to know the motivation for the code. \$\endgroup\$ – 200_success Mar 19 '16 at 18:30
  • \$\begingroup\$ Sorry that my comment wasn't more informative. The assignment was to initialize an array where each index corresponds to a thread and then using those threads sum the array. My motivation was to do the assignment, it's my first multithreaded piece of code. \$\endgroup\$ – 1101 Mar 19 '16 at 19:23
3
\$\begingroup\$

In this answer I want to take a look at what your code is doing, and why it doesn't really make sense to use threads like this.

One of the main point of using threads is to do long-running things at the same time (more details here). Some examples:

  • keeping a GUI responsive while doing other work in the background.
  • Spreading long calculations over multiple cores.
  • Doing long-running tasks like waiting for incoming connections or collecting statistics.

If you do them on the same thread, they have to wait for each other. Keep each one on a separate thread, and they'll happily run side-by-side.

In your code, this doesn't really happen.

     for (int i = 0; i < threadsArray.length; i++) {
        threadsArray[i] = new Thread(new Runnable() {
            public void run() {
                synchronize(lock1) {
                    processedArray[numberOfValues - 1] = numberOfValues;
                    numberOfValues--;
                }

            }   
        });
        threadsArray[i].start();
    }

Here you're creating your first set of threads, to set the values of the array. However, all of the code inside the thread is synchronized over one object.

static Object lock1 = new Object();

Only one thread can hold a lock at a time. So while one thread is doing its thing, the others are just waiting. This means that all the threads you've created are waiting for each other. They're not running at the same time at all!

But think about why threads are useful: they allows you to do long running tasks at the same time. Except these threads aren't doing anything long. They're just setting a value in an array. That's super fast. It probably takes longer to make the thread than to set the value.

So the same code could have been written like this, without threads:

for (int i = 0; i < processedArray.length; i++) {
    processedArray[i] = i;
}

And it would have been more readable, shorter and faster.

Addendum

As I've said before, this problem should not be solved using threads. However as an exersize let's look at why you need a lock, and how to get rid of it.

public void run() {
    synchronize(lock1) {
        processedArray[numberOfValues - 1] = numberOfValues;
        numberOfValues--;
    }
}

You're sharing the variable numberOfValues between threads. This means that without lock:

  • One thread could excecute the first line.
  • Another thread could execute the same line.

Because numberOfValues hasn't been decremented, both proccessArray[] values are now set to the same value.

Instead of using a normal integer as a counter you could use an AtomicInteger.

static AtomicInteger numberOfValues = new AtomicInteger(17);

Your code will look like this:

public void run() {
    int index = numberOfValues.decrementAndGet();
    processedArray[index] = index;
}

You won't need a lock now, since all access to the AtomicInteger is atomic. There's no way to set a proccessArray value without also decrementing the counter.

But we can do better. Instead of using a counter, why don't we just use the index of the loop you're using to create the threads? The code will look like this:

for (int i = 0; i < threadsArray.length; i++) {
    final int index = i;
    threadsArray[i] = new Thread(new Runnable() {
        @Override
        public void run() {
            processedArray[index] = index;
        }
    });
}

Now you don't need a shared counter, and you wont't need a lock.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the detailed post. I originally had no locks in the run() sections and the problem was of course that, sometimes, the values wouldn't be added in the array (0 instead of 16 for example) and the same thing with sum. I agree that this is probably not the best use for threads, but the assignment was to to it this way to work more with threads. \$\endgroup\$ – 1101 Mar 20 '16 at 7:54
  • \$\begingroup\$ @1101 I've added a bit of extra explanation about why you need a lock, and how to get rid of it. \$\endgroup\$ – ThatOneGuy Mar 20 '16 at 10:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.