This code works (or appears to anyway), but before I build it into my larger application I want to check that this can't be done better/faster etc. It has the potential to create a major bottleneck in the application so I want to make sure it is done in the most efficient way possible. I'm not very experienced with the concurrent package. Any feedback would be greatly appreciated.

Here is the concept code that I have written to test with. System outs will be replaced with logging when it is built into the main application.

import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.Executor;
import java.util.concurrent.Executors;
import java.util.concurrent.Semaphore;

{

private final Semaphore semaphore = new Semaphore(3);

private final Executor executor = Executors.newCachedThreadPool();

public static void main(final String[] args)
{
for (int i = 0; i < 20; i++)
{
{
@Override
public void run()
{
try
{
}
catch (final InterruptedException e)
{
throw new RuntimeException(e); // unreachable
}
}
}).start();
}
}

{
public void setResult() {}
public void getResult() {}
}

{
{
}
}

@Override
public void run()
{
try
{
while (true)
{
this.semaphore.acquire(); // blocks until we can get a permit

// get all the other tasks in the queue
{
}

}
}
catch (final InterruptedException e)
{
throw new RuntimeException(e); // unreachable
}
}

{
final Runnable runnable = new Runnable()
{
@Override
public void run()
{
try
{
{
task.setResult(); // once fully implemented will set the result to be the result of exeuteTasks()
{
}
}
}
finally
{
}
}
};
this.executor.execute(runnable);
}

{
try
{
}
catch (final InterruptedException e)
{

e.printStackTrace();
}
}
}


I am not a Java expert, but

• If you are going to log "waiting for task" then you should also log "waiting for semaphore"
• Related to that, I have a suspicion that your semaphore approach does not work, you should try to run more than 3 tasks.
• I would allow the caller to pass 3 and 100 as parameters or read this from a config file or environment variables
• Thanks, to respond to your bullet points: 1. I removed all references to 'jobs', you're quite right that was confusing. Hopefully the updated first paragraph makes the problem clearer. 2. Added extra logging as suggested. 3. This code runs and appears to work, so I think the semaphore is working, could you elaborate on that point? 4. Good point, when this code is integrated into the larger application I will make use of the existing config system. – jimjim Apr 16 '14 at 15:38
• Now your code will be even more confusing, because you have tasks running tasks ? I would have suggested to re-introduce jobs. As for point 3, if you see the metaphor work and never have more than 3 main tasks, then I was probably wrong. – konijn Apr 16 '14 at 15:47
• Well, the code as it stands uses a TaskProcessor class to process Tasks or Task groups in anonymous threads, so I think it was the description that was confusing the issue. Also as Marc-Andre pointed out I can't modify the code posted, so hopefully changing the description helps people understand what I'm trying to do here. – jimjim Apr 16 '14 at 15:54

I can have a maximum of three concurrent threads processing tasks. Each of these threads can process 1 to 100 tasks simultaneously (by connecting to an external system).

Q: How many threads should you have? A: 4 - the management thread, and 3 worker threads.

How many threads are in your code? At least 24 .... potentially hundreds .... let's count them...

2. 20 threads created in the main method
3. The CachedThreadPool has no upperbound on the number of threads, and will create a new thread each time you call this.executor.execute(runnable); but, that is gated by the Semaphore

About that Semaphore.... Your code is asymmetrical in the sense that the semaphore is acquired in one class, and released in another (the Runnable)., This makes it hard to follow.

Fundamentally, though, your system is a poor example of using the java.util.concurrent.* package, and there are more 'idiomatic' ways to do this:

// no interruptedException handling, not validated... just to give you the idea.

}

// submit all torun tasks to the remote system
// get the results.... and populate the return value:
.....
}
}

private final class TaskManager implements Runnable {
public void run () {

boolean done = false;
while (!done) {
queue.drainTo(tosubmit, CHUNKSIZE - 1);
Task last = tosubmit.get(tosubmit.size() - 1);
if (last == null) {
done = true;
tosubmit.remove(tosubmit.size() -1);
}
if (!tosubmit.isEmpty()) {
}
}
service.shutDown();
service.awaitTermination();
}
}

private static final int THREADS = 3;
private static final int CHUNKSIZE = 100;

private final ExecutorService service;