Last Comment Bug 805689 - Hitting omgslow fallback path and allocating large amounts of memory in LayerManagerOGL when unlocking gaia lock screen
: Hitting omgslow fallback path and allocating large amounts of memory in Layer...
Status: RESOLVED FIXED
[MemShrink:P1][soft]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: P2 normal (vote)
: mozilla19
Assigned To: Jeff Muizelaar [:jrmuizel]
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks: slim-fast
  Show dependency treegraph
 
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)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
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

Description 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,

  <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 2012-10-25 18:07:43 PDT
This may be related to bug 788409.
Comment 2 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.
Comment 3 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.
Comment 4 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.
Comment 5 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 ;).
Comment 6 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?
Comment 7 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
Comment 8 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.
Comment 9 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.
Comment 10 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.
Comment 11 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?
Comment 12 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
Comment 13 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.)
Comment 14 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.
Comment 15 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.
Comment 16 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.
Comment 17 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.
Comment 18 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.
Comment 19 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 ;).
Comment 20 Joe Drew (not getting mail) 2012-10-30 19:24:44 PDT
So can we land this? :)
Comment 21 Jeff Muizelaar [:jrmuizel] 2012-10-31 19:15:10 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/189d6461e441
Comment 22 Ed Morley [:emorley] 2012-11-01 06:49:20 PDT
https://hg.mozilla.org/mozilla-central/rev/189d6461e441
Comment 23 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
Comment 24 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+.
Comment 25 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.
Comment 26 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.
Comment 27 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.