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

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mcomella, Assigned: mcomella, Mentored)

Tracking

(Blocks 1 bug, {perf})

unspecified
Firefox 41
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

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

Attachments

(2 attachments)

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.
Mentor: michael.l.comella
Component: General → Graphics, Panning and Zooming
Keywords: perf
Whiteboard: [good first bug][lang=java]
Component: Graphics, Panning and Zooming → Awesomescreen
The little bit more I looked into the these, I realized the solutions weren't always super simple.
Whiteboard: [good first bug][lang=java] → [good next bug][lang=java]
(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?
Bug 1174365 - Remove TopSitesGridView.onMeasure allocation call. r=mhaigh

We pre-allocate and re-use the item now.
Attachment #8625873 - Flags: review?(mhaigh)
Bug 1177166 - Remove allocation in TopSitesThumbnailView.onLayout. r=mhaigh
Attachment #8625874 - Flags: review?(mhaigh)
Assignee: nobody → michael.l.comella
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.
Attachment #8625874 - Flags: review?(mhaigh) → review+
Attachment #8625873 - Flags: review?(mhaigh) → review+
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!
https://hg.mozilla.org/mozilla-central/rev/a5bcca1a2396
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
You need to log in before you can comment on or make changes to this bug.