How simple animation can be a big problem

Recently in our team project we decided to fix OOM issues. Yes, we had them. And we had a lot of OutOfMemoryError-s. Most of them was because of android annotations library, version 3.3.2. It didn’t provide cleaning up views that were defined by @ViewById annotation. And since we had “one activity many fragments” architecture, in some period of time we had a great amount of views, stored in memory. And thus, we had OOM. We changed version to 4.2.0 (see changes for 4.0.0 version where clean up views in fragments was introduced) and eventually almost all cases where fixed. Almost. Still we had some small issues. They were disgusting. Why? Because, for example we stored RecyclerView adapter as a fragment field and we didn’t clean up it. And since recycler adapter store link to RecyclerView (maybe I will write about my amazement of it in the next article), we didn’t clean up it also. And thus we didn’t clean up whole view hierarchy of whole fragment (since view store link to it children and parent). Our custom progress animation was such a ‘small’ issue that prevented views from being garbage collected.

How it was looked at the beginning

To be terse, here is the code of our progress view:

See the problem?

The first problem is poor readability of code. It’s because of decision to use Handler instead of Animations or Animators. The second problem stems from the first one. This code is hard to maintain. Mostly because of using Handler. What happens in Runnable? How long it takes to play the animation? It’s unclear. And moreover if tomorrow designer come to you and tell that you need to stretch the animation a little bit, or change it interpolation, you will be very difficult to apply his wishes. But the third problem is the most important one. It’s a memory leak. Do you see it?

The leak consists of two parts. The first one is anonymous Runnable instance, that holds reference to outter class (our progress view). The second is repeatedly calling postDelayed(Runnable r, long delayMillis) method. So in the one side we keep reference of view in Runnable, that will somehow be handled by instance of Handler.class through postDelayed method, but in the same time in the end of invoking this runnable we call postDelayed once again. And so on. Thus, we don’t allow to finish work with Runnable and thus forbid to garbage collect the view.

Here what we have when we run heap profiler:

Result of switching between search page and profile page in app with old animation

Suspicious 20 instances of animation view

Now we see who keep reference to ProgressAnimationView and thus to view hierarchy

Here we clearly see the problem. I ran device and started to switching between search page and profile page (it’s our root pages with bottom navigation). And we see that we have much less instances of ProfileFragment(profile page) compare to SearchResultFragment. It because search page has progress animation and profile page not. And in second screenshot we see that indeed we have many instances of ProgressAnimationView that don’t allow fragments to be destroyed. From the third screenshot we see, that, as I said before, MessagesQueue holds our anonymous Runnable, that has reference to outer ProgressAnimationView.

Naive fixes

There are two hotfixes to resolve this problem (actually they not so hot and fast, they more naive from my point of view).

• We need to rewrite runnable to static nested class that will hold WeakReference to view that creates it:

This will fix our third problem, but remain unreadable code that will hard to update.

• Since we use this animation in fragments, we can call setShow(false) in fragment’s onDestroyView. If we will use animation in more complex view, then it will harder to provide such functionality, but still possible. This will fix the memory leak, but remain first two described problems, and, plus, you will need to always think and remember to stop animation when closing application page. Last problem can be solved by placing setShow(false) in the onDetachedFromWindow method, that has every view.

Better solution

It’s not ideal, but for me it’s will be better since it will be more clear. Here is my solution:

Here we changed handlers and inner classes to simple ValueAnimator, that animates time and we controls view changing in standard update listener. Also we could easily update our animation.

Before (left), after (right)

Also we optimized our view. Earlier it was LinearLayout with 3 ImageView, now it is a view with painting in canvas, which is faster. Also we use now automatically start and end animation when you show/hide view through setVisibility method and stop animation when we detaches from window. Still there is some shortcomings that we can fix and micro-optimizations that we can make. For example, we can encapsulate state and position of each circle in static nested class Bulb. Or, for example, we can invalidate not whole view, when alpha updated only for one circle, but call invalidate just for this piece of view. So let’s try to remove duplicates and apply our micro-optimization suggestions:

Conclusion

When you work with UI and implement custom view, animations, pages, e.t.c, you always need to understand what classes or approaches you need to use and also often remember to clean up views to prevent memory leaking.