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

RESOLVED FIXED in Firefox 33

Status

()

Firefox for Android
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Margaret, Assigned: Shashank VRSN Sabniveesu, Mentored)

Tracking

Trunk
Firefox 33
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=java][good first bug])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8438842 [details] [diff] [review]
BUG 1024120 - Create new member to hold Rect object r=lucasr
Assignee: nobody → shashank
Attachment #8438842 - Flags: review?(lucasr.at.mozilla)
Flags: needinfo?(lucasr.at.mozilla)
(Assignee)

Comment 2

4 years ago
(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?
Flags: needinfo?(margaret.leibovic)
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.
Attachment #8438842 - Flags: review?(lucasr.at.mozilla) → feedback+

Updated

4 years ago
Flags: needinfo?(lucasr.at.mozilla)
(Reporter)

Comment 4

4 years ago
(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 :)
Flags: needinfo?(margaret.leibovic)
(Assignee)

Comment 5

4 years ago
(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 :)
(Assignee)

Comment 6

4 years ago
Created attachment 8439491 [details] [diff] [review]
BUG 1024120 - Create new member to hold Rect object r=lucasr
Attachment #8438842 - Attachment is obsolete: true
Attachment #8439491 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #8439491 - Flags: review?(lucasr.at.mozilla) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Thanks for the patch! Can we get a Try run to confirm that this doesn't break any existing tests?
Keywords: checkin-needed
Mentor: lucasr.at.mozilla@gmail.com
Whiteboard: [lang=java][mentor=lucasr][good first bug] → [lang=java][good first bug]
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
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
Keywords: checkin-needed
Whiteboard: [lang=java][good first bug] → [lang=java][good first bug][fixed-in-fx-team]
(Assignee)

Comment 11

4 years ago
(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!
https://hg.mozilla.org/mozilla-central/rev/f0b6c8f60ecb
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [lang=java][good first bug][fixed-in-fx-team] → [lang=java][good first bug]
Target Milestone: --- → Firefox 33
You need to log in before you can comment on or make changes to this bug.