0
\$\begingroup\$

So I am trying to delete a list having n number of objects and thereby I divided the list in 4 parts and let it to be deleted in parallel. The intention here is to do it in parallel so as to make it fast rather than sequential. deleteObject(x) can be assumed as an API which finally deletes an object has the code to take care of concurrency. It returns true on successful delete and false otherwise

I have few question here

  1. So as I have 4 parallel method which I am executing though invokeAll and there are 4 threads declared by Executors.newFixedThreadPool(4) would there always be 1 thread be assigned to 1 method?
  2. Do I need to synchronize and use volatile for iterator 'i' in the for loop of parallelDeleteOperation() method. The reason I am asking this is supposing if 1st thread has not completed its task (deleting listDir1 and the for loop has not completed) and supposing in midway it got context switch and 2nd thread starts executing the same task(deleting listDir1). Just wondering if 2nd thread can get IndexOutOfBound exception in this case.
  3. Is there any advantage of dividing list in 4 parts and executing this rather than having multiple threads executing delete operation on a very big list.

  4. If one of the operation of ExecutorService returns false then on the whole deleteMain() API would return false 

    public boolean deleteMain(List<Integer> listDir) {
if(listDir.size()<4){
    return parallelDeleteOperation(listDir);
}
final List<Integer> listDir1 = listDir.subList(0, listDir.size() / 4);
final List<Integer> listDir2 = listDir.subList(listDir.size() / 4, listDir.size() / 2);
final List<Integer> listDir3 = listDir.subList(listDir.size() / 2, (listDir.size()/ 4) *3);
final List<Integer> listDir4 = listDir.subList((listDir.size()/ 4)*3, listDir.size());`
Set<Callable<Boolean>> callables = new HashSet<Callable<Boolean>>();
callables.add(new Callable<Boolean>() {
    @Override
    public Boolean call() throws Exception {
        return parallelDeleteOperation(listDir1);
    }
});
callables.add(new Callable<Boolean>() {
    @Override
    public Boolean call() throws Exception {
        return parallelDeleteOperation(listDir2);
    }
});
callables.add(new Callable<Boolean>() {
    @Override
    public Boolean call() throws Exception {
        return parallelDeleteOperation(listDir3);
    }
});
callables.add(new Callable<Boolean>() {
    @Override
    public Boolean call() throws Exception {
        return parallelDeleteOperation(listDir4);
    }
});


ExecutorService service = Executors.newFixedThreadPool(4);
try {
    List<Future<Boolean>> futures = service.invokeAll(callables);

for(Future<Boolean> future : futures){
    if( future.get() != true)
        return future.get();
}
} catch (InterruptedException e) {
    e.printStackTrace();
} catch (ExecutionException e) {
    e.printStackTrace();
}finally{
service.shutdown();
}
return true;
}
public boolean parallelDeleteOperation(List<Integer> listDir) {
for (int i = 0; i < listDir.size(); i++) {
    int x = listDir.get(i);
    deleteObject(x);
    if(deleteObject(x)!=true)
        return false;
     }
return true;
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Your code will not work properly as written. Objects will be deleted twice, which should immediately fail. \$\endgroup\$ Commented Aug 23, 2018 at 3:38
  • 1
    \$\begingroup\$ Note: I rolled back your last edit, as modifying the original code after an answer is not allowed on the codereview site. Please ask a followup-question (or in case of problems, take the problem to stackoverflow) but leave the original code untouched. \$\endgroup\$ Commented Aug 24, 2018 at 5:02

1 Answer 1

Reset to default
2
\$\begingroup\$

First of all, regarding your questions:

  1. Yes. The 4 Futures will be mapped to 4 executor threads for all practical considerations. There might be edge-cases, where the first future is finished before the last is submitted and a thread gets reused, but this does not lead to any practical implications.
  2. No, the i is a local variable. There is a different instance in each thread.
  3. As you go index-based, you might as well use start- and end-indexes for your Future executions instead of sublists. On the other hand, if the list is an ArrayList this is exactly what happens under the hood of the sublist call.
  4. Yes, I cannot see the question there.

However, your code has some other points worth addressing:

  • as AJNeufeld already mentioned, deleteObject() gets called twice per object
  • if a single object deletion fails, the following objects in that sublist will not even be attempted - is that the expected behaviour?
  • Callable is a functional interface, so instead of creating an anonymous subclass for each callable, simply use a lambda expression: () -> parallelDeleteOperation(listDir1)
  • index-based list traversal only makes sense if you actually need the index variable. In this case, you should use for(int x : listDir)

Regarding the general principle: you try to do many things manually and via a design decision, which the system already can do for you. Especially the idea to use exactly 4 CPUs is odd: if the target machine only has 2 CPUs you will wast time through unnecessary thread-switches, on an 8-core half of the machine is idle.

Additionally, as I already mentioned above, you don't attempt to delete every object, but only up to the first failure.

To rectify this, I suggest that you simply use a parallelStream which will be split to a number of cores suited for the given machine automatically, and collect the total success value via a reducer:

boolean allDeletionsSuccessful
= listDir.parallelStream()             // split to number of cores
    .map(x -> deleteObject(x))         // attempt each deletion, record success value
    .reduce(true, (x, y) -> x & y);    // merge with logical AND
\$\endgroup\$
3
  • \$\begingroup\$ Using a parallel Stream is indeed much simpler and doesn't bury the intent under a pile of thread management details. If the boolean result isn't needed, you could use filter() instead of map() (that would also be safer than calling deleteObject(x) which, I suppose, holds a reference to the list being processed and modifies it at the same time). \$\endgroup\$ Commented Aug 23, 2018 at 7:46
  • 1
    \$\begingroup\$ @Kolargol00 I don't see any evidence that deleteObject(int) references or modifies the list, but this detail is hidden from us. Nevertheless, filtering for x -> !deleteObject(x) (i.e. collecting all the failures) is definitely a good alternative approach if you want to do better error handling. \$\endgroup\$ Commented Aug 23, 2018 at 9:32
  • \$\begingroup\$ Thanks a lot for your answers. I updated my code and trying to get it work with parallel streams.... I am getting errors here. \$\endgroup\$ Commented Aug 24, 2018 at 3:04

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.