Bug 805689 - Hitting omgslow fallback path and allocating large amounts of memory in LayerManagerOGL when unlocking gaia lock screen
 Summary: Hitting omgslow fallback path and allocating large amounts of memory in Layer...
 Status: RESOLVED FIXED [MemShrink:P1][soft] Core Components Graphics (show other bugs) unspecified ARM Gonk (Firefox OS) P2 normal (vote) mozilla19 Jeff Muizelaar [:jrmuizel] Milan Sreckovic [:milan] slim-fast Show dependency tree / graph

Reported: 2012-10-25 18:02 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-12-11 07:47 PST (History)
12 users (show)
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
 blocking-basecamp: +
 status-firefox18: fixed status-firefox19: fixed

Attachments
Don't copy when we don't need subpixel AA (1.39 KB, patch)
2012-10-26 12:01 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
Don't copy when we don't need subpixel AA (1.42 KB, patch)
2012-10-26 12:06 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
Don't copy when we don't need subpixel AA fixed (1.47 KB, patch)
2012-10-29 14:17 PDT, Jeff Muizelaar [:jrmuizel]
matt.woodrow: review+
Details | Diff | Splinter Review

 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-25 18:02:03 PDT We end up here http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/LayerManagerOGL.cpp#1296 We're getting here through if (needsFramebuffer) { LayerManagerOGL::InitMode mode = LayerManagerOGL::InitModeClear; nsIntRect framebufferRect = visibleRect; if (aContainer->GetEffectiveVisibleRegion().GetNumRects() == 1 && (aContainer->GetContentFlags() & Layer::CONTENT_OPAQUE)) { [snip] } else { const gfx3DMatrix& transform3D = aContainer->GetEffectiveTransform(); gfxMatrix transform; // If we have an opaque ancestor layer, then we can be sure that // all the pixels we draw into are either opaque already or will be // covered by something opaque. Otherwise copying up the background is // not safe. if (HasOpaqueAncestorLayer(aContainer) && transform3D.Is2D(&transform) && !transform.HasNonIntegerTranslation()) { mode = LayerManagerOGL::InitModeCopy; I think we're outsmarting ourselves by trying to copy up from the framebuffer. This is causing some huge allocations in the b2g process. In the case I caught in gdb, > = { x = -145, y = -189, width = 611, height = 879 which if I count correctly would be 3.2 MB :(. Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-25 18:07:43 PDT This may be related to bug 788409. Matt Woodrow (:mattwoodrow) 2012-10-25 18:16:38 PDT We can definitely just not do this. We are indeed attempting to be smart, copying up the framebuffer contents into the new temporary surface so that we can support component alpha text. But we don't ever attempt to use this on mobile. MOZ_GFX_OPTIMIZE_MOBILE ftw. We are we getting a temporary surface required here btw? Fixing that might be a nice additional win. Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-25 19:13:17 PDT If by that you mean UseComponentAlpha() or something like that, absolutely! ;) (In reply to Matt Woodrow (:mattwoodrow) from comment #2) > We are we getting a temporary surface required here btw? Fixing that might > be a nice additional win. Not sure yet, but even by fixing this we'll still be allocating and rendering a ginormous temp fb, so we may still need to chase that down. Matt Woodrow (:mattwoodrow) 2012-10-25 19:43:38 PDT There's fairly strong precedent for using MOZ_GFX_OPTIMIZE_MOBILE in OGL layers :) But yes, I totally agree. Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-25 20:11:00 PDT Yep, and we've been working to kill all those uses ;). Jeff Muizelaar [:jrmuizel] 2012-10-26 12:01:22 PDT Created attachment 675632 [details] [diff] [review] Don't copy when we don't need subpixel AA How about something like this? Jeff Muizelaar [:jrmuizel] 2012-10-26 12:06:26 PDT Created attachment 675635 [details] [diff] [review] Don't copy when we don't need subpixel AA And something that builds Matt Woodrow (:mattwoodrow) 2012-10-26 16:11:12 PDT Comment on attachment 675635 [details] [diff] [review] Don't copy when we don't need subpixel AA Review of attachment 675635 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxPlatform.h @@ +311,5 @@ > virtual bool FontHintingEnabled() { return true; } > > + bool SubpixelAA() { > +#ifdef MOZ_GFX_OPTIMIZE_MOBILE > + return true; Isn't this backwards? Also, since removing MOZ_GFX_OPTIMIZE_MOBILE is a goal, can we make this virtual instead. 'UsesSubpixelAATextRendering' or similar would be nice too. Jeff Muizelaar [:jrmuizel] 2012-10-26 21:14:56 PDT (In reply to Matt Woodrow (:mattwoodrow) from comment #8) > Comment on attachment 675635 [details] [diff] [review] > Don't copy when we don't need subpixel AA > > Review of attachment 675635 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/thebes/gfxPlatform.h > @@ +311,5 @@ > > virtual bool FontHintingEnabled() { return true; } > > > > + bool SubpixelAA() { > > +#ifdef MOZ_GFX_OPTIMIZE_MOBILE > > + return true; > > Isn't this backwards? Yes it is. That's what happens when the patch you test is not the patch you post :) > Also, since removing MOZ_GFX_OPTIMIZE_MOBILE is a goal, can we make this > virtual instead. Is this a goal? What is the point of that goal? In this case I don't see any reason to not make the decision at compile time. > 'UsesSubpixelAATextRendering' or similar would be nice too. I chose to keep it short to make it less likely to need to be line wrapped when written as a condition. I can understand wanting the longer name. Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-26 21:20:59 PDT (In reply to Jeff Muizelaar [:jrmuizel] from comment #9) > (In reply to Matt Woodrow (:mattwoodrow) from comment #8) > > Comment on attachment 675635 [details] [diff] [review] > > Don't copy when we don't need subpixel AA > > > > Also, since removing MOZ_GFX_OPTIMIZE_MOBILE is a goal, can we make this > > virtual instead. > > Is this a goal? What is the point of that goal? In this case I don't see any > reason to not make the decision at compile time. > Yes. That macro has been used as lazy shorthand in many places where features should have been tested instead, and the laziness keeps coming back to bite us. Fwiw I agree with mattwoodrow about making this a gfxPlatform feature test, but this isn't my front yard. Jeff Muizelaar [:jrmuizel] 2012-10-29 14:15:30 PDT (In reply to Chris Jones [:cjones] [:warhammer] from comment #10) > Fwiw I agree with mattwoodrow about making this a gfxPlatform feature test, > but this isn't my front yard. I'm not sure what you mean by a feature test. Isn't this just a policy decision that gfxPlatform should decide at compile time? Jeff Muizelaar [:jrmuizel] 2012-10-29 14:17:59 PDT Created attachment 676305 [details] [diff] [review] Don't copy when we don't need subpixel AA fixed Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-29 14:56:23 PDT No, I don't think so. We should decide whether to use subpixel AA based on screen DPI and performance of the machine we're running on. ("should" in the ivory-tower sense.) Matt Woodrow (:mattwoodrow) 2012-10-29 15:00:28 PDT I do agree with comment 13 fwiw, but it's not really in the scope of this bug. Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-29 16:02:48 PDT This will result in allocations of 3MB for readback buffer, another 3MB for temp fbo, and possibly another 3MB for upload buffer, which could end up being 6-9% of memory available for apps. That's enough to get background things killed when they shouldn't be. Matt Woodrow (:mattwoodrow) 2012-10-29 16:14:37 PDT Chris: Can you grab the paint list output for this page (MOZ_DUMP_PAINT_LIST=1 in your env). We can hopefully figure out why we're getting a temporary surface from that. Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-30 15:11:52 PDT Anything left to do here? (In reply to Matt Woodrow (:mattwoodrow) from comment #16) > Chris: Can you grab the paint list output for this page > (MOZ_DUMP_PAINT_LIST=1 in your env). > > We can hopefully figure out why we're getting a temporary surface from that. Yep, but this doesn't appear to be causing perceivable perf issues, so let's do it in a different bug. Nicholas Nethercote [:njn] 2012-10-30 16:32:06 PDT Can someone clarify the status of this? We have an r+ patch that hasn't landed, but comment 15 is confusing me. Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-30 16:59:21 PDT Comment 15 is idle argument about ivory towers. It can safely be ignored ;). Joe Drew (not getting mail) 2012-10-30 19:24:44 PDT So can we land this? :) Jeff Muizelaar [:jrmuizel] 2012-10-31 19:15:10 PDT https://hg.mozilla.org/integration/mozilla-inbound/rev/189d6461e441 Ed Morley [:emorley] 2012-11-01 06:49:20 PDT https://hg.mozilla.org/mozilla-central/rev/189d6461e441 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-12-07 23:49:14 PST Comment on attachment 676305 [details] [diff] [review] Don't copy when we don't need subpixel AA fixed [Approval Request Comment] Bug caused by (feature/regressing bug #): NA User impact if declined: Spikes of memory usage of up to 9MB or so that will kill background processes unnecessarily, and possibly increase heap fragmentation. Testing completed (on m-c, etc.): Over a month, none reported. Risk to taking this patch (and alternatives if risky): \epsilon (as close to 0 as is possible) String or UUID changes made by this patch: None Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-12-07 23:49:38 PST As a memshrink P1, we might also consider a bb+. Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-12-07 23:52:06 PST Also, we should do a query for MemShrink winners for b2g that might have been overlooked for uplift. Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-12-08 00:42:55 PST Comment on attachment 676305 [details] [diff] [review] Don't copy when we don't need subpixel AA fixed Because of time pressure, I'm making a decision to make this a slim-fast soft blocker with \epsilon risk and high reward, and land. Please back out if you disagree. Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-12-08 00:52:39 PST https://hg.mozilla.org/releases/mozilla-beta/rev/4e486ec47f47

 Note You need to log in before you can comment on or make changes to this bug.