Closed Bug 634788 Opened 11 years ago Closed 11 years ago

Displayport is way too big

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: dougt, Assigned: dougt)

References

Details

(Keywords: perf)

Attachments

(2 files)

Attached patch patch v.1Splinter Review
changing the displayport reallocs the layers which can cause slowness during panning, fragmentation, and death.
Attachment #512982 - Flags: review?(ben)
Comment on attachment 512982 [details] [diff] [review]
patch v.1

Add a comment about what you're doing and take out dumps.
Attachment #512982 - Flags: review?(ben) → review+
A comment in this bug might be useful too. What heck is the purpose of this patch?
Blocks: 624444
tracking-fennec: --- → ?
Keywords: perf
OS: Linux → All
Hardware: x86_64 → All
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?
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.
(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.)
Matt is right on both counts.
Attachment #512982 - Flags: review+ → review-
Attachment #513184 - Flags: review+
Bug was originally put in bug 613954 with r+ from mbrubeck, but we are using that bug for other purposes.
This bug stays open because we don't know if this fixes the small changes in displayport width/height.
Approved for beta 5
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.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Summary: we change the displayport way too often → Displayport is way too big
how does one test this?
(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.
Flags: in-testsuite?
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
Status: RESOLVED → VERIFIED
tracking-fennec: ? → ---
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.