Synchronized block over concurrent collections

Question

Here, this vector was used at many APIs in the code with sync block on it. And all the places they want to search another collection based on each itr.next().

synchronised(vector)
{
    Iterator itr = vector.iterator();
    while (itr.hasNext) 
    {
       //Perform some search on another collection using the value from itr.next() :: THIS IS COSTLY JOB WITH SYNCH BLOCK
       //If that collection doesn't have any entry for itr.next() then perform itr.remove();
     }
}

This is like a vector of conference IDs. Using each conference ID, it gets a conference from list of conferences, then searches again for a user within that conference having user ID from list of users available in that conference.

As stated above: now the sync block is doing a time-consuming search job on another objects. And other threads keep waiting for this vector lock. And the total number of threads in the blocked state increases much while load testing.

I changed the code from Vector to Collections.newSetFromMap(new ConcurrentHashMap<psConference,Boolean>()). I assume that, since this is concurrent collection, no ConcurrentModificationException while performing iteration. Add/remove operations while iteration is allowed here, so I have removed all sync block in my code.

But I come to know from ConcurrentHashMap Java doc:

iterators are designed to be used by only one thread at a time.

From the above, I think I have moved the sync block problem from my code to the Java API. There is no advantage over this change.

I finally found the best return copy of original vector collection each time any APIs requires that vector of conference IDs. Let the iteration happen on this new vector with that costly searching and other jobs and then finally perform the remove separately.

Vector newVectorObject = mgr.getCopyOfVector();
List toBedeleted = new ArryList();

Iterator itr = newVectorObject .iterator();
    while (itr.hasNext) 
    {
       //Perform some search on another collection using itr.next() 
       //If that collection doesn't have any entry for itr.next() then perform toBedeleted.add(itr.next())
     }
mgr.removeVector(toBedeleted);


public Vector getCopyOfVector(){
      return new Vector(vector);
}

public Vector removeVector(List toBedeleted){
      return vector.removeAll(toBedeleted);
}

I think in this case, no lock is required on the vector. No threads will be blocked.

Answer 1

Just some thoughts:

• For your list use a Collections.newSynchronizedList(new ArrayList<ConferenceID>()), only perform add(), remove() or addAll(), don't use Iterators. That way the synchronized blocks will be short.
• When you start processing, insert(copy) all the ConferenceID objects into a BlockingQueue. This BlockingQueue is consumed by some thread(s) that run the (time consuming) Job.
• Use some ExecutorService with Futures (eg. Futures<Tuple<ConferenceId, Boolean>>) for the jobs to indicate if the processed ConferenceID has to be removed.
• Remove the ConferenceID from your original list if necessary once the job has finished.

Keep your synchronized blocks as short as possible. Have you many objects in the list? (Because you copy them every time you start your processing).

I also like idea with the CopyOnWriteArrayList, as @toto2 mentioned. But you have to be careful, since writes are expensive and the Iterator does not support modifying operations.

That being said, Java is fast with object creation/deletion. So you have to try out. It is more likely that you end up starving because some resource is blocked(synchronized), so creating copies should be a good idea.

Answer 2

It looks like you are trying to re-invent the CopyOnWriteArrayList.

By the way, Vector is very old fashioned, but I guess you are working with legacy code. The code also does not use generics. CopyOnWriteArrayList started with Java 5, so might not be able to use it if you are stuck with an older Java version.