[Solved] ConcurrentModificationException - How to avoid it

Newbie & Debugging Questions
#1

Hi

So I have a class called PopUps that renders all PopUp objects, or if they have been marked disposed, they are removed from the list in PopUps. Anyways, the list, once empty, throws a ConcurrentModificationException. I don’t know what to do to fix this, so I brought the question here.


   /**
	 * Render all popups
	 */
	public void render(){
		
		for(PopUp popup : popups){ // This line is where the exception is thrown.
			if(popup.isDisposed()){
				popups.remove(popup);
				continue;
			}
			popup.render();
		}
		
	}

Any thoughts? If you need to see more code just ask :slight_smile:

CopyableCougar4

#2

You can’t use a for each loop if you add or remove items. You have to instead loop through backwards with a regular for loop.


for(int i = popups.size() - 1; i >= 0; i--)
{
     Popup popup = popups.get(i);
     if(popup.isDisposed()){
            popups.remove(popup);
            continue;
     }
     popup.render();
}
#3

Hi

Thanks Longarmx :slight_smile:

CopyableCougar4

#4

I prefer this slightly more compact format using the faster overloaded index-based version of the remove() method: ::slight_smile:

for (int i = popups.size(); i-- != 0;) {
  PopUp p = popups.get(i);

  if (p.isDisposed())  popups.remove(i);
  else                 p.render();
}

And in the case the index order isn’t important, this approach averts any left-shifting after remove(): ;D

for (int len = popups.size(), i = len; i-- != 0;) {
  PopUp p = popups.get(i);

  if (!p.isDisposed())  p.render();

  else {
    p = popups.remove(--len);
    if (i != len)  popups.set(i, p);
  }
}
#5

For removing you can use this as well:


for (int i = 0; i < popups.size(); i++) {
       Popup p = popups.get (i);

       if (p.isDisposed ()) {
              popups.remove (i);
              i--;
       }
       p.render();
}
#6

???
I’m wondering why nobody has mentioned the ‘best’ method (most commonly used, fast and reliable): Use iterators.
With iterator’s you can remove objects from a collection while you’re iterating over them.
Sample:


public void render(){
	Iterator<PopUp> iterator = popups.iterator();
	while(iterator.hasNext()){
		PopUp popup = iterator.next();
		if(popup.isDisposed()){
			iterator.remove();
			continue;
		}
		popup.render();
	}
}

You should loop until [icode]iterator.hasNext()[/icode], you can receive the current element with [icode]iterator.next()[/icode] and delete the current element from the collection using [icode]iterator.remove()[/icode]. :wink:

#7

I daresay that a backwards loop would still be a bit faster than an Iterator version.
B/c the condition is against a literal 0 rather than a method call like hasNext()! :persecutioncomplex:
And the 2nd example using a remove()/set() combo is unbeatable! Averting left-shifting collateral is hard to ignore IMO! ::slight_smile:
On the other hand, if I was dealing w/ a structure like a LinkedList or similar, going w/ an Iterator or a ListIterator is a no-brainer! :stuck_out_tongue:

#8

Wrong. Java’s built-in for-each loop essentially uses iterators so the performance is exactly the same, while the c-style for loop what you’re talking about will generally be slower. You can read a pretty good explanation on that here. :slight_smile:

#9

I see no evidence there that the c-style is slower for iterating over a random access structure such as ArrayList. Sure it will be night and day with a LinkedList, but that is a separate issue.

#10

C-style is about 10% faster to iterate and doesn’t create an Iterator instance that the GC has to cleanup.

#11

Indeed. Iterators in tight nested loops can actually put substantial pressure on the GC if you’re not careful.
How recent is that 10% figure?

#12

Java 7u42 or so.

#13

Don’t get me wrong, I love using the “enhanced” for-“each” loop version!
Even though it can be 10% slower for non-array structures than a c-style for loop! ;D
However, we can’t use remove() on such loops. Either c-style or Iterator/ListIterator style for such occasions! ::slight_smile:

#14

Just written a minibenchmark for this and the result I got is kind of surprising.
No matter how many samples I use, or what array size, I always end up with iterators and c-style loops being almost exactly as fast, and for-each loops being quite a bit slower.
The results I’ve got from the gist above on an i5 3570K@4GHz, Win7, Java 7u60:


Array size: 150000
Total time took: 2549ms or 2.549 seconds
Enhanced for average: 0.672
C for average: 0.53
Iterator average: 0.532

So as I see it, there is really no reason to avoid iterators if you’re afraid of performance loss. If you prefer C-style for loops over iterators because that’s your programming style though, that’s a valid reason. :slight_smile:
If there’s anything wrong with my minibenchmark (can easily happen since I wrote it around 1AM) let me know please. :slight_smile:

#15

I made the loops in your bench actually perform work (minimal, incrementing a counter by the string length) and made it run the whole test over and over (to make sure the JIT is warmed, makes a difference):
It’s still pretty impromptu though.

Results which are consistent after a few rounds: (upped samples because 500 is not much)

Still odd that the for-each and iterators are different…

EDIT: this is on Win7, 7u51

8u5 results, in case it matters:

Tidy speed up there it seems.