1024120 - Avoid allocating a new Rect on each touch event on the main layout

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

Right now, in HideTabsTouchListener, we allocate a new Rect on each touch event:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#1993

Instead of doing this, we should create an an mTempRect member.

Comment 1

[Shashank VRSN Sabniveesu]

Attachment: BUG 1024120 - Create new member to hold Rect object r=lucasr

Comment 2

[Shashank VRSN Sabniveesu]

(In reply to :Margaret Leibovic from comment #0)
> Right now, in HideTabsTouchListener, we allocate a new Rect on each touch
> event:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.
> java#1993
> 
> Instead of doing this, we should create an an mTempRect member.

BUG1024120.patch - Changed all references to 'rect' to 'mTempRect'.

I am curious to know how big this change effects memory allocation.
Also, what lead you to identify this now?

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 8438842 [details] [diff] [review]
BUG 1024120 - Create new member to hold Rect object r=lucasr

Review of attachment 8438842 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch! Just needs some small fixes/tweaks.

::: mobile/android/base/BrowserApp.java
@@ +1975,5 @@
>      }
>  
>      private class HideTabsTouchListener implements TouchEventInterceptor {
>          private boolean mIsHidingTabs = false;
> +        private final Rect mTempRect;

Initialize mTempRect on declaration:

private final Rect mTempRect = new Rect();

nit: add empty line after mTempRect declaration.

Comment 4

[:Margaret Leibovic]

(In reply to Shashank VRSN Sabniveesu from comment #2)
> (In reply to :Margaret Leibovic from comment #0)
> > Right now, in HideTabsTouchListener, we allocate a new Rect on each touch
> > event:
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.
> > java#1993
> > 
> > Instead of doing this, we should create an an mTempRect member.
> 
> BUG1024120.patch - Changed all references to 'rect' to 'mTempRect'.
> 
> I am curious to know how big this change effects memory allocation.

I'm not certain how big a change this will make, but we hit this code path every time the user touches the app, so this is a good place for an optimization. You could try using some of Android's memory-reporting tools to measure memory before/after your patch, it would be interesting to see the difference.

> Also, what lead you to identify this now?

This is just a bug that lucasr noticed while reviewing my patch for bug 1019735. Big projects are always full of unreported bugs waiting to be discovered :)

Comment 5

[Shashank VRSN Sabniveesu]

(In reply to :Margaret Leibovic from comment #4)
> (In reply to Shashank VRSN Sabniveesu from comment #2)
> > (In reply to :Margaret Leibovic from comment #0)
> > > Right now, in HideTabsTouchListener, we allocate a new Rect on each touch
> > > event:
> > > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.
> > > java#1993
> > > 
> > > Instead of doing this, we should create an an mTempRect member.
> > 
> > BUG1024120.patch - Changed all references to 'rect' to 'mTempRect'.
> > 
> > I am curious to know how big this change effects memory allocation.
> 
> I'm not certain how big a change this will make, but we hit this code path
> every time the user touches the app, so this is a good place for an
> optimization. You could try using some of Android's memory-reporting tools
> to measure memory before/after your patch, it would be interesting to see
> the difference.
> 
> > Also, what lead you to identify this now?
> 
> This is just a bug that lucasr noticed while reviewing my patch for bug
> 1019735. Big projects are always full of unreported bugs waiting to be
> discovered :)
Thanks for the response. I noted down a few more bugs from the discussion there :)

Comment 6

[Shashank VRSN Sabniveesu]

Attachment: BUG 1024120 - Create new member to hold Rect object r=lucasr

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 8439491 [details] [diff] [review]
BUG 1024120 - Create new member to hold Rect object r=lucasr

Review of attachment 8439491 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks! Please add the checkin-needed keyword.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Thanks for the patch! Can we get a Try run to confirm that this doesn't break any existing tests?

Comment 9

[Shashank VRSN Sabniveesu]

Try
https://tbpl.mozilla.org/?tree=Try&rev=f4bc98ecb809

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/f0b6c8f60ecb

Thanks for the Try push! One more request - please try to be conscientious about what builds and tests you run when you do push there. In this case, B2G builds and tests aren't necessary for a code change that only affects Android. Thanks!
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Comment 11

[Shashank VRSN Sabniveesu]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4][PTO June 19-22] from comment #10)
> One more request - please try to be conscientious
> about what builds and tests you run when you do push there. In this case,
> B2G builds and tests aren't necessary for a code change that only affects
> Android. Thanks!

Yes, I learned from your previous similar comment and avoided all desktop builds & tests this time. I still wondered if B2G being a mobile product, that too might require testing. Thanks for the comment. I'll avoid B2G in future unless I'm working on FirefoxOS

Thanks!

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f0b6c8f60ecb