1174365 - [Linter: DrawAllocation] Don't allocate in draw/layout functions

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

via lint:

 ../../src/main/java/org/mozilla/gecko/home/TopSitesGridView.java:114: Avoid object allocations during draw/layout operations (preallocate and reuse instead)

 111         final int columnWidth = getColumnWidth();
 112 
 113         // Get the first child from the adapter.
 114         final TopSitesGridItemView child = new TopSitesGridItemView(getContext());
 115 
 116         // Set a default LayoutParams on the child, if it doesn't have one on its own.

../../src/main/java/org/mozilla/gecko/home/TopSitesThumbnailView.java:90: Avoid object allocations during draw/layout operations (preallocate and reuse instead)

  87         super.onLayout(changed, left, top, right, bottom);
  88         if (HardwareUtils.isTablet() && mResize) {
  89             setScaleType(ScaleType.MATRIX);
  90             RectF rect = new RectF(0, 0, mWidth, mHeight);
  91             Matrix matrix = new Matrix();
  92             matrix.setRectToRect(rect, rect, Matrix.ScaleToFit.CENTER);

../../src/main/java/org/mozilla/gecko/home/TopSitesThumbnailView.java:91: Avoid object allocations during draw/layout operations (preallocate and reuse instead)

  88         if (HardwareUtils.isTablet() && mResize) {
  89             setScaleType(ScaleType.MATRIX);
  90             RectF rect = new RectF(0, 0, mWidth, mHeight);
  91             Matrix matrix = new Matrix();
  92             matrix.setRectToRect(rect, rect, Matrix.ScaleToFit.CENTER);
  93             setImageMatrix(matrix);

Priority: 9 / 10
Category: Performance
Severity: Warning
Explanation: Memory allocations within drawing code.
You should avoid allocating objects during a drawing or layout operation. These are called frequently, so a smooth UI can be interrupted by garbage collection pauses caused by the object allocations.

The way this is generally handled is to allocate the needed objects up front and to reuse them for each drawing operation.

Some methods allocate memory on your behalf (such as Bitmap.create), and these should be handled in the same way.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

The little bit more I looked into the these, I realized the solutions weren't always super simple.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Michael Comella (:mcomella) from comment #1)
> The little bit more I looked into the these, I realized the solutions
> weren't always super simple.

Agreed.

I don't have any suggestions for TopSitesGridView.java:114 yet.

In TopSitesThumbnailView.java, we get mWidth and mHeight in onMeasure, which is still a bad place to new objects. Of course, the objects are only new'd if mResize is true, which happens when we set a new bitmap. Could we make the matrix then?

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1174365 - Remove TopSitesGridView.onMeasure allocation call. r=mhaigh

Bug 1174365 - Remove TopSitesGridView.onMeasure allocation call. r=mhaigh

We pre-allocate and re-use the item now.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1177166 - Remove allocation in TopSitesThumbnailView.onLayout. r=mhaigh

Bug 1177166 - Remove allocation in TopSitesThumbnailView.onLayout. r=mhaigh

Comment 5

[Martyn Haigh (:mhaigh)]

Comment on attachment 8625874 [details]
MozReview Request: Bug 1177166 - Remove allocation in TopSitesThumbnailView.onLayout. r=mhaigh

https://reviewboard.mozilla.org/r/11941/#review10547

Ship It!

::: mobile/android/base/home/TopSitesThumbnailView.java:88
(Diff revision 1)
> +        if (!HardwareUtils.isTablet() || !mResize) {

Is that better than?
if (!(HardwareUtils.isTablet() && mResize))

Up to you.

Comment 6

[Martyn Haigh (:mhaigh)]

Comment on attachment 8625873 [details]
MozReview Request: Bug 1174365 - Remove TopSitesGridView.onMeasure allocation call. r=mhaigh

https://reviewboard.mozilla.org/r/11939/#review10549

Ship It!

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 8

[Carsten Book [:Tomcat]]

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