The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 18

Status

()

Core
Graphics
P2
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: cjones, Assigned: jrmuizel)

Tracking

(Blocks: 1 bug)

unspecified
mozilla19
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

(Whiteboard: [MemShrink:P1][soft])

Attachments

(1 attachment, 2 obsolete attachments)

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.
Blocks: 788409
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.
No longer blocks: 788409
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 ;).
Whiteboard: [MemShrink]
(Assignee)

Comment 6

5 years ago
Created attachment 675632 [details] [diff] [review]
Don't copy when we don't need subpixel AA

How about something like this?
Attachment #675632 - Flags: review?(matt.woodrow)
(Assignee)

Comment 7

5 years ago
Created attachment 675635 [details] [diff] [review]
Don't copy when we don't need subpixel AA

And something that builds
Attachment #675632 - Attachment is obsolete: true
Attachment #675632 - Flags: review?(matt.woodrow)
Attachment #675635 - Flags: review?(matt.woodrow)
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.
(Assignee)

Comment 9

5 years ago
(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.
(Assignee)

Comment 11

5 years ago
(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?
(Assignee)

Comment 12

5 years ago
Created attachment 676305 [details] [diff] [review]
Don't copy when we don't need subpixel AA fixed
Attachment #675635 - Attachment is obsolete: true
Attachment #675635 - Flags: review?(matt.woodrow)
Attachment #676305 - Flags: review?(matt.woodrow)
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.)
Attachment #676305 - Flags: review?(matt.woodrow) → review+
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.
Whiteboard: [MemShrink] → [MemShrink:P1]
Comment 15 is idle argument about ivory towers.  It can safely be ignored ;).
So can we land this? :)
(Assignee)

Comment 21

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/189d6461e441
QA Contact: jmuizelaar
Assignee: nobody → jmuizelaar
QA Contact: jmuizelaar
https://hg.mozilla.org/mozilla-central/rev/189d6461e441
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
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
Attachment #676305 - Flags: approval-mozilla-beta?
Attachment #676305 - Flags: approval-mozilla-aurora?
As a memshrink P1, we might also consider a bb+.
blocking-basecamp: --- → ?
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.
Attachment #676305 - Flags: approval-mozilla-beta?
Attachment #676305 - Flags: approval-mozilla-aurora?
blocking-basecamp: ? → +
Priority: -- → P2
Target Milestone: mozilla19 → mozilla18
Whiteboard: [MemShrink:P1] → [MemShrink:P1][soft]
https://hg.mozilla.org/releases/mozilla-beta/rev/4e486ec47f47
status-firefox18: --- → fixed
status-firefox19: --- → fixed

Updated

4 years ago
Target Milestone: mozilla18 → mozilla19
You need to log in before you can comment on or make changes to this bug.