Code Zen

LogBook 1.2.1 patch release is now available on googlecode now. The release addresses quite a few bugs and feature requests. The list of patches can be found here.
We are also discussing features for LogBook 1.3 at the Google group for the project. If you have a feature you would like to see added or contribute, join the conversation here. Like Mat said in a previous thread there, the importance of a feature is only as much as the conversation around it.

There are some interesting discussions I have had around LogBook with a couple of folk within Comcast Interactive Media where I work. For example, one of the things we have had to deal with is QA bugs that we cannot reproduce. Running LogBook in the background on QA machines and looking at the logs generated would be an interesting idea to implement, thus making LogBook a part of the QA cycle. The biggest thing that needs is a notion of a session in LogBook which is something we dont have right now. I guess that could be a part of 1.3 or 1.4. Other ideas are welcome.

If you have any code around debugging Flex applications that you would like to contribute to LogBook, do let me know as well.

Links:
Download LogBook 1.2.1
LogBook on Googlecode

One of the bugs reported on our recently released LogBook application was around the “Clear Logs” button. Within LogBook, we keep all the LogMessage messages in an ArrayCollection. When Clear Logs was clicked, we called a removeAll() function to clear the ArrayCollection. However when the size of the ArrayCollection grew more than a hundred or so, the clear command took a while and the whole application would freeze for a few seconds. A quick change of that to ArrayCollection.source = new Array() fixed the performance immediately. Interesting.
Looking deeper into the removeAll function and following the hierarchy to the ArrayList class, I saw that the removeAll loops through the entire source array, looking at each member to see if its an event dispatcher and if so calling a function to remove the propertyChange listener. Confused a bit, I posted a message on flexcoders to see why this was the case. Alex Harui from Adobe resonded with this:

Our code has to look like that otherwise if someone kept a reference to one of the objects that got removed, it would keep the ArrayList from being GC’d. We added listeners, so we should remove them. If the data objects are going away too, then you can probably reset the source, but if you see a memory leak later, I’d definitely check in that area.

Makes sense, but only kind of since the way the eventlistener was added was with the weak keys flag turned on, which means that I did reset the array, even if the old LogMessage objects were in memory, the garbage collector could still get rid of them whenever it ran. So I responded to Alex so:

Hmm, maybe I am missing something here? The only place I see addEventListener being called is

protected function startTrackUpdates(item:Object):void
{
if (item && (item is IEventDispatcher))
{
IEventDispatcher(item).addEventListener(
PropertyChangeEvent.PROPERTY_CHANGE,
itemUpdateHandler, false, 0, true);
}
}

which has the weak-keys boolean flag turned on. Shouldnt that allow garbage collection to run without explicitly calling removeEventListener ?

And Alex’s response:

Because of the way GC works, I still think it is best for us to remove the listeners. GC is only on allocation so again, if someone had a handle to an object that got removed and twiddled a value, the notification code would run needlessly until GC kicked in.

However, I think you’ve come up with a great optimization for folks who know they are destroying everything all at once.

So the final verdict: If you are destroying all the objects within the Arraycollection, use ArrayCollection.source = new Array() instead of ArrayCollection.removeAll(). Calling removeAll() on large ArrayCollections seems to be a really heavy operation and could kill responsiveness of the app.

I do feel that this is a bug and not a simple optimization. Here is the final email I sent to flexcoders a couple of minutes back:

Hi Alex,

Thanks for responding. Should this be tracked as a bug? I think there may be a better way of handling this:

On removeAll() you can set a flag like removeEventListenersPending = true, copy the old elements to a temporary new array and run through the it in smaller sized increments (removing event listeners from say 40 objects every enterframe using something like calllater). In the meanwhile if a property change event is fired, you can see if the flag is set and if so make sure the item is in the current source array and not the temp array. When all objects in the temp have had their event listeners removed, set the removeEventListenersPending = false.

I think the current implementation may be causing a lot of performace issues since a lot of people may be using ArrayCollections for large data sets (thats what Flex is great for 🙂 ) and may not know of the penalty of the removeAll().

If anyone has any opinions, please feel free to comment. Okay, back to real work :).