Friday, March 21, 2008

Object Leaks & The Dangers of ArrayCollection.removeItemAt



I have been using the new Profiler in Flex Builder 3 to fix some performance issues in my flex apps and discovered a bit of an anti-pattern in several places in my code. I like to use object pools to re-use my custom renderer objects. To do this I first have my renderers implement the IFactory interface. Which is basically just adding this function:
public function newInstance():*
In the class that I will be using the renderers, I create some code to manage the pool of renderers, which is basically just two methods:
protected function getRenderer():MyRenderer
protected function freeRenderers():void
Calling getRenderer() will check the pool of "free" renderers and return one if available, or if not, create a new one and add it tool the pool of renderers currently in use. The freeRenderers() method just takes all of the in-use renderers and moves them to the free pool. The discovered my anti-pattern here in the freeRenderers() method:
for ( var I:int i = 0; i < usedRenderers.length; i++ )
{
freeRenderers.addItem( usedRenderers.removeItemAt(i) );
}
This code will fail to copy all of the used renderers from the in-use pool to the free pool. The reason is that as we remove items, we are changing the length of the collection that we are indexing across. The proper way to do this is:
while ( usedRenderers.length > 0 )
{
freeRenderers.addItem( usedRenderers.removeItemAt( 0 ) );
}
There is another variation of this issue which needs to be handled differently. If you are only wanting to transfer some of the items out of the collection, you might do something like this:
var key:String = "foo";
for ( var i:int i = 0; i < collection.length; i++ )
{
if ( MyObject(collection.getItemAt( i )).id == key )
collection.removeItemAt( i );
}
Doing this will cause problems similar to the first example. This is basically like a ConcurrentModificationException that you might get in Java. The problem is that in Flex's ArrayCollection you don't get any exception when you do this, so it is real easy to think you are doing the right thing when you are not. Here is one possible fix for this:
var key:String = "foo";
var removes:ArrayCollection = new ArrayCollection();
for ( var i:int = 0; i < collection.length; i++ )
{
if ( MyObject(collection.getItemAt( i )).id == key )
removes.addItem( collection.getItemAt( i ) );
}

for ( i = 0; i < removes.length; i++ )
{
collection.removeItemAt( collection.getItemIndex( removes.getItemAt( i ) ) );
}
This is pretty simple but it is definitely something worth remembering

2 comments:

Scott said...

Actually, re: remove certain items - i try not to use increment / length vars with Collections, and go for simplicity, such as:

var key:String = "foo";
for each ( var item:MyObject in collection )
{
if ( item.id == key ) {
var i:int = collection.getIndexOf( item ); //or equivalent..
collection.removeItemAt( i );

//take it to the break
break;
//unless you're storing dupes (?)
}
}

Would be nice if they just had a removeItem() method..., but I usually end up wrapping Collections within my own data types, where I can do that sort of thing, myself.

Anonymous said...

Much simpler solution:

for ( var i:int i = 0; i < usedRenderers.length; i++ )
{
freeRenderers.addItem( usedRenderers.removeItemAt(i--) );
}