634788 - Displayport is way too big

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Doug Turner (:dougt)]

Attachment: patch v.1

changing the displayport reallocs the layers which can cause slowness during panning, fragmentation, and death.

Comment 1

[Benjamin Stover (:stechz)]

Comment on attachment 512982 [details] [diff] [review]
patch v.1

Add a comment about what you're doing and take out dumps.

Comment 2

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

A comment in this bug might be useful too. What heck is the purpose of this patch?

Comment 3

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

Comment on attachment 512982 [details] [diff] [review]
patch v.1

So it seems like you are ignoring small changes in the displayport. Instead of setting displayport equal to the old cached value and then making the API call. Why make the API call at all? If the displayport is being set to exactly what it already was, why not just bail?

Comment 4

[Doug Turner (:dougt)]

Hi Mark.  Thank you for your suggestions both about the comments in this bug being terse and the comments in the bug being non-existent.  Sorry it wasn't clear when you first read the bug report.  I am happy that you were able to piece the picture together.

You are correct in comment #3 about the purpose of this patch.  And your suggestion about bailing might be fine.  I do not know this code as well as Ben or you, so I defer to your advice.

Comment 5

[Matt Brubeck (:mbrubeck)]

(In reply to comment #3)
> Why make the API call at all? If the displayport is being set to exactly what
> it already was, why not just bail?

The patch only changes the width and height of the passed viewport to be the same.  The location will generally still be different.

>+        if (Math.abs(json.w - cachedDisplayPort.width) < delta ||
>+            Math.abs(json.h - cachedDisplayPort.height) < delta ) {

Drive-by review:  This should be && instead of ||.  (This is important when the size of the window changes in just one dimension, e.g. to show the keyboard.)

Comment 6

[Benjamin Stover (:stechz)]

Matt is right on both counts.

Comment 7

[Benjamin Stover (:stechz)]

Attachment: we change the displayport way too often (OOM fix)

Comment 8

[Benjamin Stover (:stechz)]

Bug was originally put in bug 613954 with r+ from mbrubeck, but we are using that bug for other purposes.

Comment 9

[Benjamin Stover (:stechz)]

Pushed: http://hg.mozilla.org/mobile-browser/rev/6da76224f125

Comment 10

[Benjamin Stover (:stechz)]

This bug stays open because we don't know if this fixes the small changes in displayport width/height.

Comment 11

[Stuart Parmenter]

Approved for beta 5

Comment 12

[Benjamin Stover (:stechz)]

Doug and I have drilled the problem down into how the visible region is set for thebes layers. Frontend's displayport numbers that are being passed in are quite stable. A new bug has been filed for the buffer reallocation issue: bug 635035.

Comment 13

[Tony Chung [:tchung]]

how does one test this?

Comment 14

[Matt Brubeck (:mbrubeck)]

(In reply to comment #13)
> how does one test this?

This should reduce content process (plugin-container) memory use when zoomed out all the way on very tall pages like planet.mozilla.org.  It might also fix OOM crashes that are reproducible in that same situation.

Adding in-testsuite? because we should add unit tests for this.

Comment 15

[Tony Chung [:tchung]]

Verified fixed on Nexus S, on Mozilla/5.0 (Android; Linux armv71; rv:2.0b13pre) Gecko/20110223 Firefox/4.0b13pre Fennec/4.0b6pre	

running ps against plugin-container, with 2 tabs open and panning around planet.mozilla.org.    Memory seems manageable and have not crashed.

http://pastebin.mozilla.org/1093276