766647 - Multiple screenshots get queued up and processed unnecessarily

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

I'm seeing often upwards of 4 or 5 full-page screenshot slices get queued and then processed, which is basically useless because they only get processed on idle (i.e. when the page has stopped changing, we re-render the page 5 times for screenshot purposes). These events should be coalesced somehow.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Drop pending events for the tab if doing a whole-page screenshot

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 634991 [details] [diff] [review]
Drop pending events for the tab if doing a whole-page screenshot

I realized this patch has a problem - it doesn't clear out sAcumulatedRect. This could result in the wrong rect being sent to setCheckerboardBitmap if some pending events are dropped. I'll try to figure out a fix for that.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Also I see sometimes that the "partial update" rect (i.e. the union of all the addRectToRepaint calls) is the whole page, and we end up re-screenshotting everything just because it goes through a different code path. The fact that this is on a 5-second delay with no coalescing again results in a lot of extra screenshotting work that should be unnecessary.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Do better coalescing of screenshots

Here's a more significant overhaul of the code that tries to minimize spurious screenshotting activity. I modified the PendingScreenshot class to encapsulate a full screenshot and keep a list of the events corresponding to the slices, and then discard these PendingScreenshot objects if we start doing a new full-page screenshot. There's also some robustification so that if we switch tabs we don't keep processing and updating screenshots from the previous tabs, and it also accounts for rounding errors in the slicing code. Still needs more testing, but feedback is welcome.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 635506 [details] [diff] [review]
Do better coalescing of screenshots

Review of attachment 635506 [details] [diff] [review]:
-----------------------------------------------------------------

if you're going to move this code out of GeckoAppShell, preserve the history by doing hg cp GeckoAppShell.java ScreenshotHandler.java and then deleting stuff from the two files

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Just another data point: loading planet.mozilla.org on a Galaxy Nexus triggers 7 full-page screenshots, for a total of 56697 slices of screenshot. That backlog takes 98 seconds to clear after the page is done loading.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Do better coalescing of screenshots

Rebased to tip and put ScreenshotLayer back in GeckoAppShell for now. I can do the "hg cp" in a follow up.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae3955b96bcd

Comment 11

[Ed Morley [:emorley]]

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

Comment 12

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

Request approval for uplift?

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 642973 [details] [diff] [review]
Do better coalescing of screenshots

[Approval Request Comment]
Bug caused by (feature/regressing bug #): low-res screenshot
User impact if declined: a lot of time is wasted taking redundant screenshots, specially just after page load. this *should* happen only when gecko is idle, but still burns up a lot of CPU time
Testing completed (on m-c, etc.): on m-c, but should probably bake for another couple of days
Risk to taking this patch (and alternatives if risky): mobile-only, but is a fairly significant change and has a risk of regressions. for example bug 775976 is a crasher that was introduced by this patch
String or UUID changes made by this patch: none

Comment 14

[Alex Keybl [:akeybl]]

Comment on attachment 642973 [details] [diff] [review]
Do better coalescing of screenshots

[Triage Comment]
I don't think we can take a patch with possibility of regression unless we've got data of the improvements we'd be taking. Please re-nominate if/when we get that info. For now, let's maintain current state in 16.

Comment 15

[Scoobidiver (away)]

(In reply to Alex Keybl [:akeybl] from comment #14)
> I don't think we can take a patch with possibility of regression unless
> we've got data of the improvements we'd be taking.
This bug blocks bug 765952 that causes hangs and crashes in 16.0a2.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

This may also have fixed bug 774616

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 642973 [details] [diff] [review]
Do better coalescing of screenshots

Re-requesting aurora approval. This has been baking on m-c for almost a week with no word of any regressions (other than the NPE in bug 775976). This patch definitely fixes bug 765952 and possibly also fixes 774616, so I think it's worth uplifting to aurora.

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

You're right to re-nom, definitely want fixes to Aurora mobile crash rate right now.

Comment 19

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3bec50c0a33f