Multithreaded for loop

Question

I am trying to multithread a for loop, and this works so far. How can it be improved? Have I done anything incorrectly?

The work is broken up into blocks and the block variable is the amount of objects to process per thread.

Test.class

public class Test extends MultiThreaded {

    public static void main(String args[]) {

        List<TestObject> values = new ArrayList<TestObject>();

        for(int i = 1;i < 7;i++)
            values.add(new TestObject(String.valueOf(i)));

        int block = 2;
        int cursor = 0;

        List<TestObject> objectList = new ArrayList<TestObject>();

        for(TestObject v : values) {
            cursor++;
            objectList.add(v);

            if(cursor < block)
                continue;

            final List<TestObject> objectListCopy = new ArrayList<TestObject>();
            objectListCopy.addAll(objectList);

            Task task = new Task() {
                @Override
                public void run() {
                    for(TestObject o : objectListCopy)
                        o.print();
                }
            };

            startTask(task);
            objectList.clear();

            cursor = 0; 
        }

        while(MultiThreaded.processing()) { continue; }

        System.out.println("move on");
    }

    public static class TestObject {
        String line;

        public TestObject(String string) {
            this.line = string;
        }

        public void print() {
            System.out.println(line);
        }
    }

}

MultiThreaded.class

public class MultiThreaded {

    public static HashSet<Task> tasks = new HashSet<Task>();

    public static class Task extends Thread {
        public boolean running;
        public void run(){  
            return;
        }  
    }

    public static void startTask(Task task) {
        task.start();
        tasks.add(task);
    }

    public static Task getTask(int id) {

        for(Task task : tasks){
            if(task.getId() == id)
                return task;
        }

        return null;
    }

    public static boolean processing() {

        boolean processing = false;

        for(Task task: tasks){
            if(task.isAlive())
                processing = true;
        }

        return processing;
    }

}

OUTPUT

1
2
5
6
3
4
move on

Answer 1

Multi-threaded concepts are complicated at times. If you really want to learn multi-threading, it is important to get it right. Some things can be extremely difficult to get right. I can highly recommend reading Java Concurrency in Practice.

Overall

It is not that often I see "multi-threaded code" without a single synchronized keyword, or a single use of an atomic variable or concurrent data structure. I suggest you read up on all three of those to better understand what you will have to deal with sooner or later.

I think that you are adding a lot of overhead with the way you are constructing your tasks, that it will remove any benefits you get by running it multi-threaded. Creating all the lists that you use simply takes more time than actually running the code.

It is also important to consider that you are using System.out.println, this is a synchronized call, which means that only a single thread can only run it a time. This tends to be a big concurrency bottle-neck in a lot of multi-threaded code. As your task(s) seems to only be to use System.out.println, I think your code would be better of as single-threaded.

Oops, missed some!

In your current code, it does not run the print method for the last x items in the list. There already, it is broken.

If you have 1024 items and use a "block" value of 100, 24 items will be missed. (Hint: What is the value of your cursor variable after your for-loop to start the tasks is finished?)

Singleton mindset

This code:

public static HashSet<Task> tasks = new HashSet<Task>();

By making it public static it seems like you are in the "singleton mindset". Avoid this. This has no business being either public or static. Imagine if some code somewhere at some point would call tasks.clear(); - That could break a lot.

This field is also a big memory leak. You are only adding tasks to it, you are not removing any tasks.

Extending for no reason

You are extending the MultiThreaded class, but all methods and fields in that class are static. Extending it has no real effect besides giving you quick-access to the methods and variables, by not having to write MultiThreaded. each time. This is a bad reason for extending a class.

getTask

Your public static Task getTask(int id) method, although it is not used, suggests that you are using the wrong data-structure. You could use a Map<Integer, Task> which will make the getTask lookup much more efficient (from \$O(n)\$ to \$O(1)\$). As you are dealing with multi-threading, better make it a ConcurrentMap as well. You should however ask yourself if you really need this in the first place.

Are we working? Are we working? Are we, are we, are we?

This code:

while(MultiThreaded.processing()) { continue; }

Is a busy-loop. This code will run as quickly as possible and waste CPU power for you. Running this will essentially remove any benefits you get by using multi-threading in the first place. You should check into the wait-notify construct. This can be compared to three kids in the backseat of your car constantly asking you "Are we there yet? Are we there yet? Are we, are we, are we?", I bet you can drive better without such a distraction. (No offense intended against kids)

This busy-loop does not get better by the fact that your processing() method is an inefficient way to check if any task is still working. Use Atomic variables to keep track of the number of running tasks.

The wheel...

Java 8 provides the concept of parallel streams which significantly makes this easier.

Your main method can be replaced with this:

List<TestObject> values = new ArrayList<TestObject>();
for (int i = 1; i < COUNT; i++)
    values.add(new TestObject(String.valueOf(i)));
values.parallelStream().forEach(o -> o.print());
System.out.println("move on");