Last Comment Bug 779511 - Freeing buffers should be moved out of finalize functions
: Freeing buffers should be moved out of finalize functions
Status: RESOLVED FIXED
[good first bug][lang=java][mentor=kats]
:
Product: Firefox for Android
Classification: Client Software
Component: Graphics, Panning and Zooming (show other bugs)
: unspecified
: All Android
: -- normal (vote)
: Firefox 19
Assigned To: pushkar.singh93
: general
:
Mentors:
Depends on: 802491 802495 803613
Blocks: 803700
  Show dependency treegraph
 
Reported: 2012-08-01 08:20 PDT by Kartikaya Gupta (email:kats@mozilla.com)
Modified: 2012-10-19 13:59 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Version1 : Introducing a destroy method in LayerRenderer class for freeing its buffer (1.20 KB, text/plain)
2012-10-11 14:57 PDT, pushkar.singh93
no flags Details
Changes including introduction of destroy() method in ScreenshotLayer for clearing its buffer. method called from destroy method in LayerRenderer (18.40 KB, patch)
2012-10-12 18:19 PDT, pushkar.singh93
bugmail: feedback+
Details | Diff | Splinter Review
Patch with required changes (9.61 KB, patch)
2012-10-17 13:38 PDT, pushkar.singh93
bugmail: review+
Details | Diff | Splinter Review
Patch with required cosmetic changes (9.69 KB, patch)
2012-10-17 16:04 PDT, pushkar.singh93
bugmail: review+
Details | Diff | Splinter Review

Description Kartikaya Gupta (email:kats@mozilla.com) 2012-08-01 08:20:36 PDT
Various classes (such as BufferedCairoImage, CheckerboardImage, etc) free their direct-allocation buffers in finalize() functions. As of bug 769269, these classes can be destroyed and re-created many times during Fennec's lifetime and so it's a bad idea to do the freeing in finalizers. It would be better to deterministically free the buffers when the activity is destroyed and we tear down the java gfx classes.
Comment 1 Kartikaya Gupta (email:kats@mozilla.com) 2012-08-15 15:09:59 PDT
This should be fairly straightforward for a new contributor. It's mostly just code cleanup.
Comment 2 Mark Capella [:capella] 2012-08-15 17:37:54 PDT
I can find 5 classes in mobile/android with finalize member functions, and paired DirectBufferAllocator.allocate() and DirectBufferAllocator.free() calls.

  gfx/BufferedCairoImage.java
  gfx/CheckerboardImage.java
  gfx/LayerRenderer.java
  gfx/ScreenshotLayer.java
  Tab.java

There are 3 other classes with DirectBufferAllocator.allocate() calls, but no matching .free(). Is the thought here to simply remove the existing 5 buffer .free() function calls?

ie: Is this what you mean by "deterministically freeing the buffers" when the activity is destroyed ?
Comment 3 Kartikaya Gupta (email:kats@mozilla.com) 2012-08-15 20:51:34 PDT
Not quite. The thing with finalize() functions is that they are called by the VM at some random time in the future after we are done with the object. See http://developer.android.com/reference/java/lang/Object.html#finalize%28%29 for background on this. The problem is that we could be creating and throwing away multiple instances of a CheckerboardImage (for example) before the finalizer runs and cleans up our memory. So we get this unnecessary spike in memory usage, with a pattern that looks like this:

- create CheckerboardImage #1, which allocates X amount of memory
- throw away CheckerboardImage #1
- create CheckerboardImage #2, which allocates another X amount of memory, so total memory requirement is now 2X
- throw away CheckerboardImage #2
- create CheckerboardImage #3, which allocates yet another X amount of memory, so total memory requirement is now 3X
- GC/finalizers run, freeing the memory from CheckerboardImage #1 and CheckerboardImage #2, bringing memory requirement back down to X

Instead what I would like is a pattern that looks like this:
- create CheckerboardImage #1, which allocates X amount of memory
- throw away CheckerboardImage #1, which frees the X memory
- create CheckerboardImage #2, which allocates X amount of memory
- throw away CheckerboardImage #2, which frees the X memory
- create CheckerboardImage #3, which allocates X amount of memory
- and so on...

In order to do this, those free() calls need to be moved out of the finalize() function and into a different function that is called explicitly when we "throw away" the object. The hard part is figuring out where the object is thrown away. In the case of a Tab, that happens in Tabs.removeTab, so we would need to do that buffer freeing there. (And in fact, it seems like there is already a call to tab.freeBuffer there, so in this instance you can just take out the finalizer).

Take a look at the other four classes that have the finalizers and see if you can determine who holds on to those objects and when they get thrown away, so that you can do the same thing there.
Comment 4 Kartikaya Gupta (email:kats@mozilla.com) 2012-08-27 15:32:09 PDT
Jonathan, are you going to be working on this bug? If so, let me know if you need any help!
Comment 5 Jonathan [:elefont] 2012-08-28 06:03:36 PDT
hey Kartikaya,

Yeah, I want to tackle this bug, but I'm a new contributor & I actually decided to take on two bugs at once. So I'll be kinda slow since this is apparently the more involved/difficult of the two. Plus I'm still trying to set up my mozilla/Android environment. 

Are you familiar at all with setting up a mozilla/Android environment on Macs? If not, do you know someone who does? I feel like I'm wasting time with this initial step, and in the end it won't work because no APK will have been deployed to my phone.
Comment 6 Kartikaya Gupta (email:kats@mozilla.com) 2012-08-28 06:53:19 PDT
If you're just starting out with contributing, I would recommend only tackling one bug at a time; if you want to work on bug 746983 first then it would be better to unassign yourself from this one.

And yes, I can help you with setting up an Android build on Macs - there are instructions at https://wiki.mozilla.org/Mobile/Fennec/Android with some Mac-specific modifications at https://wiki.mozilla.org/Mobile/Fennec/Android_OtherBuildEnvs

If you're having trouble getting it set up it would be best to jump on IRC and message me and I can walk you through it. You can also post in #mobile if I'm not around. It's much easier to debug environment problems on IRC because it generally requires a lot of back-and-forth interaction to figure out what your environment is and what needs to be done.
Comment 7 pushkar.singh93 2012-09-07 15:04:30 PDT
Hi, I am a new contributor and I'm interested in working on this bug. So can I work on this even though it has been assigned to someone? I am kinda excited to try my hands on this with some guidance:)
Comment 8 Kartikaya Gupta (email:kats@mozilla.com) 2012-09-08 00:19:14 PDT
Hi pushkar, I've assigned the bug to you since I don't believe Jonathan has looked at this bug at all since it was assigned to him. I'm happy to help you get set up and working on this. To start you should get Fennec building on your local machine and make sure that it runs fine - there are instructions on how to build at https://wiki.mozilla.org/Mobile/Fennec/Android

Once you have a working build take a look at the code in mobile/android/base/gfx and the problem as I described in comment #3 and see if you can figure out a good way to fix it. Feel free to comment in this bug or ping me on IRC (my handle is kats) if you run into problems or have any questions.
Comment 9 pushkar.singh93 2012-09-08 13:11:33 PDT
Thanks, I am on it!
Comment 10 pushkar.singh93 2012-09-10 15:28:11 PDT
I built the code and its working fine, I am now going to tackle the main bug.
Comment 11 pushkar.singh93 2012-09-15 13:00:01 PDT
Did u received my messages on IRC?
Comment 12 Kartikaya Gupta (email:kats@mozilla.com) 2012-10-11 10:43:59 PDT
Just an update here: pushkarsingh has been working with me on IRC to learn how the relevant pieces of the gfx code work. We have identified a good potential solution for when to free the gfx buffers but implementation has not yet started.
Comment 13 pushkar.singh93 2012-10-11 14:57:33 PDT
Created attachment 670554 [details]
Version1 : Introducing a destroy method in LayerRenderer class for freeing its buffer

To be done: Freeing buffers of relevant Layers
Comment 14 pushkar.singh93 2012-10-12 18:19:23 PDT
Created attachment 671007 [details] [diff] [review]
Changes including introduction of destroy() method in ScreenshotLayer for clearing its buffer. method called from destroy method in LayerRenderer

To Do : Buffer cleanup in other left over layers
Comment 15 Kartikaya Gupta (email:kats@mozilla.com) 2012-10-14 21:42:15 PDT
Comment on attachment 671007 [details] [diff] [review]
Changes including introduction of destroy() method in ScreenshotLayer for clearing its buffer. method called from destroy method in LayerRenderer

As discussed on IRC, you've got the right idea here. Adding explicit destroy functions to the layer renderer and checkerboard layer is a good start. It will probably be better to add the function to the TileLayer base class so that it can be reused across all of the layer implementations that hold an image, and likewise a destroy function should be added to the CairoImage class and implemented in all of its subclasses.

Also the patch you uploaded here is not formatted properly; it seems to contains two different patches mashed into one, along with a changes_version3 file which itself contains a patch. IIRC you are using git, so you should be able to do a "git diff origin/master" to generate a single patch with everything you've changed, and you can get rid of the changes_version3 file (or move it somewhere else if you feel the need to keep it).
Comment 16 pushkar.singh93 2012-10-17 13:38:16 PDT
Created attachment 672502 [details] [diff] [review]
Patch with required changes

This patch clears all the buffers associated with LayerRender when activity is destroyed.
Comment 17 Kartikaya Gupta (email:kats@mozilla.com) 2012-10-17 14:01:39 PDT
Comment on attachment 672502 [details] [diff] [review]
Patch with required changes

This looks great! I've tested it locally as well and it works fine. I just have a few nits on code style and logging, once those are addressed we can land the patch! :)

>diff --git a/mobile/android/base/gfx/BufferedCairoImage.java b/mobile/android/base/gfx/BufferedCairoImage.java
> 
> import java.nio.ByteBuffer;
> 
>+import android.util.Log;
>+

Please move this import statement up to just under the Bitmap import statement. I forgot to send you this link before (totally my fault, I apologize): https://wiki.mozilla.org/Fennec/NativeUI/CodingStyle
For import statements all the android.* statements should be grouped together and sorted in alphabetical order.

>     private int mFormat;
>     private boolean mNeedToFreeBuffer;
>+    
>+    private static String LOGTAG = "BufferedCairoImage";

Make this "GeckoBufferedCairoImage". All of our logtags start with "Gecko" so that they're easy to identify in the logcat.

>     }
>+    
>+    @Override
>+    public void destroy() { 
>+        try {
>+            freeBuffer();
>+            Log.d(LOGTAG, "BufferedCairoImage FREED USING destroy");

Take out this Log.d call - this is useful for debugging but in the final version of the patch we should leave it out so it doesn't clutter up logcat output. This is particularly true here because this is the "normal" code path now.

>+        } catch(Exception ex) {

Add a space between "catch" and "(".

>diff --git a/mobile/android/base/gfx/CheckerboardImage.java b/mobile/android/base/gfx/CheckerboardImage.java
> import org.mozilla.gecko.mozglue.DirectBufferAllocator;
> 
> import android.graphics.Color;
> 
> import java.nio.ByteBuffer;
> import java.nio.ShortBuffer;
> import java.util.Arrays;
>+import android.util.Log;

Same here, move up the import statement to just under the Color import statement.

>     private static final float TINT_OPACITY = 0.4f;
>+    
>+    private static String LOGTAG = "CheckerboardImage";

Make this GeckoCheckerboardImage.

>     }
>+    
>+    @Override
>+    public void destroy() { 
>+        try {
>+            DirectBufferAllocator.free(mBuffer);
>+            mBuffer = null;
>+            Log.d(LOGTAG, "CheckerboardImage FREED USING destroy");

Take out this Log.d statement.

>+        } catch(Exception ex) {

Space between "catch" and "(".

>diff --git a/mobile/android/base/gfx/LayerRenderer.java b/mobile/android/base/gfx/LayerRenderer.java
>         }
>     }
>+    
>+    public void destroy() {
>+        DirectBufferAllocator.free(mCoordByteBuffer);
>+        mCoordByteBuffer = null;
>+        mCoordBuffer = null;
>+        mScreenshotLayer.destroy();  // destroy function of TileLayer

You can take out the comment on the above line, it shouldn't be needed.

>+        mBackgroundLayer.destroy();
>+        mShadowLayer.destroy();
>+        mHorizScrollLayer.destroy();
>+        mVertScrollLayer.destroy();
>+        if(mFrameRateLayer != null)
>+            mFrameRateLayer.destroy();

Add a space between the "if" and "(", and wrap the body of the statement in braces.

>+        Log.d(LOGTAG, "CLEARING BUFFERS! ");
>+    }

Take out this log line.

>+        
>+        @Override
>+        public void destroy() {
>+            try {
>+                DirectBufferAllocator.free(mBuffer);
>+                mBuffer = null;
>+                Log.d(LOGTAG, "CLEARING BUFFERS! by DESTROY in screenshotImage");
>+            } catch(Exception ex) {

Same things here: take out the Log.d, and add a space between "catch" and "(".

>diff --git a/mobile/android/base/gfx/TileLayer.java b/mobile/android/base/gfx/TileLayer.java
> 
> import android.graphics.Rect;
> import android.opengl.GLES20;
>+import android.util.Log;
> 
> import java.nio.ByteBuffer;

This import is in the right place, nice! :)

>     }
>+    
>+    public void destroy() {
>+        try {
>+            if (mImage != null) {
>+            Log.d(LOGTAG, "REACHED TileLayer DESTROY METHOD");
>+            mImage.destroy();
>+            }   
>+        } catch(Exception ex) {

Same here - remove log and add a space. Also the indentation of the mImage.destroy() line is wrong, it needs to be indented one more.

Finally, there's a bunch of trailing whitespace in your patch - these are empty spaces at the end of the line. They're harmless but we like to keep them out of the tree as they can make diffs harder to read later. To see all the trailing whitespace, you can look at the splinter view for your patch (click on the 'Splinter Review' link for the attachment, or just go to https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=779511&attachment=672502). Anything that shows up in pink is a trailing space, and should be removed.

Thanks again for taking this bug and seeing it through! This code is really really hard to understand and I'm glad you didn't get discouraged but pushed through and figured out how to fix it. :)
Comment 18 pushkar.singh93 2012-10-17 16:04:28 PDT
Created attachment 672571 [details] [diff] [review]
Patch with required cosmetic changes

This patch includes all the required cosmetic changes.
Comment 19 Kartikaya Gupta (email:kats@mozilla.com) 2012-10-17 20:52:24 PDT
Comment on attachment 672571 [details] [diff] [review]
Patch with required cosmetic changes

Perfect, thanks!
Comment 20 Kartikaya Gupta (email:kats@mozilla.com) 2012-10-17 20:56:14 PDT
Landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/0883b53129b9

Also for future reference I did push the earlier version of the patch to try and it was as green as could be expected: https://tbpl.mozilla.org/?tree=Try&rev=5ab4e83b9e6d
Comment 21 Ed Morley [:emorley] 2012-10-18 10:35:17 PDT
https://hg.mozilla.org/mozilla-central/rev/0883b53129b9

Note You need to log in before you can comment on or make changes to this bug.