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.
Created attachment 634991 [details] [diff] [review] Drop pending events for the tab if doing a whole-page screenshot
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.
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.
Created attachment 635506 [details] [diff] [review] 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 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
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.
Created attachment 642973 [details] [diff] [review] 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.
Request approval for uplift?
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 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.
(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.
This may also have fixed bug 774616
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.
You're right to re-nom, definitely want fixes to Aurora mobile crash rate right now.