805689 - Hitting omgslow fallback path and allocating large amounts of memory in LayerManagerOGL when unlocking gaia lock screen

Status: RESOLVED FIXED

(Core :: Graphics, defect, P2)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This may be related to bug 788409.

Comment 2

[Matt Woodrow (:mattwoodrow)]

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.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 4

[Matt Woodrow (:mattwoodrow)]

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

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Attachment: Don't copy when we don't need subpixel AA

How about something like this?

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Attachment: Don't copy when we don't need subpixel AA

And something that builds

Comment 8

[Matt Woodrow (:mattwoodrow)]

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.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

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

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 11

[Jeff Muizelaar [:jrmuizel]]

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

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Attachment: Don't copy when we don't need subpixel AA fixed

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 14

[Matt Woodrow (:mattwoodrow)]

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

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 16

[Matt Woodrow (:mattwoodrow)]

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.

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 18

[Nicholas Nethercote [inactive]]

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

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 20

[Joe Drew (not getting mail)]

So can we land this? :)

Comment 21

[Jeff Muizelaar [:jrmuizel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/189d6461e441

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/189d6461e441

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 24

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 25

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 26

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 27

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/releases/mozilla-beta/rev/4e486ec47f47