HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Parallel for loop in Java 8

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
loopparallelforjava

Problem

This is my first attempt to provide some syntactic sugar for doing mutually independent loop iterations in parallel. Thanks to Java 8 lambdas, I can write the parallel loops in pretty elegant fashion compared to pre-lambda Java's.

Also, I have declared the actual forp as synchronized, which leads to the question will it be sufficient to make sure that two or more threads trying to use forp will get to it in some non-overlapping order?

My code is as follows:

ParallelFor.java:

```
package net.coderodde.util;

import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.Callable;
import java.util.concurrent.ForkJoinPool;
import static net.coderodde.util.Utilities.checkNotNull;
import static net.coderodde.util.Utilities.checkRange;

/**
* This class implements the parallel for loop. It is assumed that two distinct
* tasks in the loop are independent, i.e., one task needs no output data from
* another task.
*
* @author Rodion Efremov
* @version I
*/
public class ParallelFor {

/**
* The thread pool doing the actual work.
*/
private static final ForkJoinPool pool;

static {
pool = new ForkJoinPool();
}

/**
* Implements the actual parallel for loop.
*
* @param the type of input data.
* @param the type of output data.
* @param inputList the list of individual arguments to the routine to
* parallelize.
* @param outputList the list of output data, may be null
* whenever no output is needed.
* @param body the actual code. May be specified in the form of a
* lambda expression.
*/
public static synchronized final
void forp(final List inputList,
final List outputList,
final ParallelLoopBody body) {
// Create the tasks.
final List> tasks = new ArrayList<>(inputList.size());

if (outputL

Solution

There are a number of concurrency issues in this code,. I can point out what I see, but correcting them will require some significant rethinking... and determining what the rewrite will need will take more than there's time for in this answer.

Concurrency issues:

-
outputList is not controlled with any synchronization. Even though you pre-size the list to have a slot 'available' for each task, there's no reason to believe that on the completion of the task, when you run:

@Override
    public Object call() throws Exception {
        final O output = body.execute(input);

        if (outputList != null) {
            outputList.set(outputIndex, output);
        }

        return null;
    }


that the copy of the outputlist in your thread is going to be the same copy as the outputList on some other thread. The value you set in that outputList may not be visible to the controlling code.

Admittedly, pre-sizing the List makes a difference on the entry side, but the exit conditions are not controlled.

Additionally, note that a user calling your code with a Non-ArrayList (perhaps a LinkedList?) may get different results to one calling with an ArrayList.

-
Code other than your code can do synchronized (ParallelFor.class) {...} and suddenly your code becomes unrunnable because you cannot get a synchronization lock on it.... Never use a publicly accessible member to control the concurrency of a class unless you need to make it a 'free for all'. Do something like:

private static final Object PARALLEL_LOCK = new Object();


and then, in your static methods, instead of making them static synchronized, do:

public static void forp(...) {
    synchronized(PARALLEL_LOCK) {
        ....
    }
}


-
The static synchronized method means that only one thread in the whole JVM can be scheduling jobs at any one time. Is there even a need to synchronize it at all? The only external fields are the pool and the input parameters. The pool is already safe, and the input parameters are presumed to be unique for each caller... right?

-
If the input parameters (outputList especially) are used for two separate calls to forp you have a problem, because one set of tasks will be overwriting the data in another set, and you may end up with concurrent modifications, and even ArrayIndexOutOfBounds exceptions

Code Snippets

@Override
    public Object call() throws Exception {
        final O output = body.execute(input);

        if (outputList != null) {
            outputList.set(outputIndex, output);
        }

        return null;
    }
private static final Object PARALLEL_LOCK = new Object();
public static void forp(...) {
    synchronized(PARALLEL_LOCK) {
        ....
    }
}

Context

StackExchange Code Review Q#73930, answer score: 5

Revisions (0)

No revisions yet.