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, <mozilla::gfx::BaseRect<int, nsIntRect, nsIntPoint, nsIntSize, nsIntMargin>> = { x = -145, y = -189, width = 611, height = 879 which if I count correctly would be 3.2 MB :(.

This may be related to bug 788409.

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.

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.

There's fairly strong precedent for using MOZ_GFX_OPTIMIZE_MOBILE in OGL layers :) But yes, I totally agree.

Yep, and we've been working to kill all those uses ;).

How about something like this?

And something that builds

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.

(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.

(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.

(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?

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.)

I do agree with comment 13 fwiw, but it's not really in the scope of this bug.

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.

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.

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.

Can someone clarify the status of this? We have an r+ patch that hasn't landed, but comment 15 is confusing me.

Comment 15 is idle argument about ivory towers. It can safely be ignored ;).

So can we land this? :)

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

As a memshrink P1, we might also consider a bb+.

Also, we should do a query for MemShrink winners for b2g that might have been overlooked for uplift.

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.