Closed Bug 969591 Opened 6 years ago Closed 6 years ago

Cleanup Thebes Layer if mRegionToDraw is Empty and we still have a buffer allocated

Categories

(Core :: Graphics, defect, P3)

x86
macOS
defect

Tracking

()

RESOLVED WONTFIX
1.4 S1 (14feb)

People

(Reporter: mchang, Assigned: mchang)

References

Details

(Keywords: perf, Whiteboard: [c=memory p= s=2014.08.15 u=])

Attachments

(1 file, 2 obsolete files)

No description provided.
Attached patch clearBuffers.patch (obsolete) — Splinter Review
Hey Matt, just a question. Why would we have a case where the visible region is not empty, but the region to draw is empty? Is that a separate bug that we should be investigating instead? Thanks! Otherwise, with this patch, we clean up the memory leak original reported in bug 958727.
Attachment #8372654 - Flags: review?(matt.woodrow)
Comment on attachment 8372654 [details] [diff] [review]
clearBuffers.patch

Review of attachment 8372654 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/RotatedBuffer.cpp
@@ +516,5 @@
> +  // a clean buffer for those cases. If everything is empty, then clear out
> +  // the buffers.
> +  if (result.mRegionToDraw.IsEmpty() && !canReuseBuffer &&
> +      validRegion.IsEmpty()) {
> +    Clear();

If think this should be a nested if() only testing !canReuseBuffer. We always want to return here if mRegionToDraw is empty, and sometimes clear the buffers as well.
(In reply to Mason Chang [:mchang] from comment #1)
> 
> Hey Matt, just a question. Why would we have a case where the visible region
> is not empty, but the region to draw is empty? Is that a separate bug that
> we should be investigating instead? Thanks! Otherwise, with this patch, we
> clean up the memory leak original reported in bug 958727.

This happens when there has been no invalidation since the last paint. It should be a fairly common, and valid case.
Attached patch clearBuffers.patch (obsolete) — Splinter Review
Thanks for the pointer Matt, that did it. It also shrinks the buffer the targeted buffer:

ThebesLayerComposite (0x43e55000) [shadow-visible=< (x=0, y=0, w=320, h=20); >] [visible=< (x=0, y=0, w=320, h=20); >] [opaqueContent] [valid=< (x=0, y=0, w=320, h=20); >]
  DeprecatedContentHostDoubleBuffered (0x4656c2b0) [buffer-rect=(x=0, y=0, w=320, h=32)] [buffer-rotation=(x=0, y=0)]
    GrallocDeprecatedTextureHostOGL (0x4530b400) [size=(width=320, height=32)] [format=SurfaceFormat::B8G8R8X8] [flags=TEXTURE_USE_NEAREST_FILTER]
    GrallocDeprecatedTextureHostOGL (0x45308f00) [size=(width=0, height=0)] [format=SurfaceFormat::UNKNOWN] [flags=TEXTURE_USE_NEAREST_FILTER]


Last question, any idea why the last buffer is empty while the other buffer has [320x32]? This seems like a new issue with this patch. Is that a separate bug or that makes sense? I've never seen double buffered thebes layers have different sizes. Thanks!
Attachment #8372654 - Attachment is obsolete: true
Attachment #8372654 - Flags: review?(matt.woodrow)
Attachment #8373117 - Flags: review?(matt.woodrow)
Attachment #8373117 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8373117 [details] [diff] [review]
clearBuffers.patch

Review of attachment 8373117 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/RotatedBuffer.cpp
@@ +515,5 @@
> +  // but the valid Region is not empty, so we still need to allocate
> +  // a clean buffer for those cases. If everything is empty, then clear out
> +  // the buffers.
> +  if (result.mRegionToDraw.IsEmpty() && canReuseBuffer) {
> +    if (validRegion.IsEmpty()) {

actually, this should be 'neededRegion'.
(In reply to Mason Chang [:mchang] from comment #4)
> Created attachment 8373117 [details] [diff] [review]
> clearBuffers.patch
> 
> Thanks for the pointer Matt, that did it. It also shrinks the buffer the
> targeted buffer:
> 
> ThebesLayerComposite (0x43e55000) [shadow-visible=< (x=0, y=0, w=320, h=20);
> >] [visible=< (x=0, y=0, w=320, h=20); >] [opaqueContent] [valid=< (x=0,
> y=0, w=320, h=20); >]
>   DeprecatedContentHostDoubleBuffered (0x4656c2b0) [buffer-rect=(x=0, y=0,
> w=320, h=32)] [buffer-rotation=(x=0, y=0)]
>     GrallocDeprecatedTextureHostOGL (0x4530b400) [size=(width=320,
> height=32)] [format=SurfaceFormat::B8G8R8X8]
> [flags=TEXTURE_USE_NEAREST_FILTER]
>     GrallocDeprecatedTextureHostOGL (0x45308f00) [size=(width=0, height=0)]
> [format=SurfaceFormat::UNKNOWN] [flags=TEXTURE_USE_NEAREST_FILTER]
> 
> 
> Last question, any idea why the last buffer is empty while the other buffer
> has [320x32]? This seems like a new issue with this patch. Is that a
> separate bug or that makes sense? I've never seen double buffered thebes
> layers have different sizes. Thanks!

Does it stay this way? I think we only modify the back-buffer, since the compositor 'owns' the front buffer. We won't make the same changes to the front buffer until we swap them.
For some layers, it stays this way, while in other layers it settles. For example, in the settings app, the last layer that is the scrollable layer has an [0x0] layer buffer until we stop scrolling. The settings Header that says 'Settings', never settles and always has an empty buffer:

ThebesLayerComposite (0x4735bc00) [shadow-visible=< (x=0, y=0, w=320, h=50); >] [visible=< (x=0, y=0, w=320, h=50); >] [opaqueContent] [valid=< (x=0, y=0, w=320, h=50); >]
  DeprecatedContentHostDoubleBuffered (0x47393080) [buffer-rect=(x=0, y=0, w=320, h=50)] [buffer-rotation=(x=0, y=0)]
    GrallocDeprecatedTextureHostOGL (0x458028c0) [size=(width=320, height=50)] [format=SurfaceFormat::B8G8R8X8] [flags=NoFlags]
    GrallocDeprecatedTextureHostOGL (0x458026c0) [size=(width=0, height=0)] [format=SurfaceFormat::UNKNOWN] [flags=NoFlags]

Visually, I don't see any issues.
Flags: needinfo?(matt.woodrow)
I think this is ok.

If we're not drawing into the Settings header, then this makes sense. We draw one, allocate the sized buffer, and give it to the compositor.

If we never do any invalidation, then we'll never need to allocate the back buffer.
Flags: needinfo?(matt.woodrow)
Carrying r+ from mattwoodrow.
Attachment #8373117 - Attachment is obsolete: true
Attachment #8373143 - Flags: review+
Keywords: checkin-needed
Priority: P1 → P3
From backout, still have to investigate if still an issue. Reopen if still an issue.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Whiteboard: [c=memory p=2 s= u=] → [c=memory p= s=2014.08.15 u=]
You need to log in before you can comment on or make changes to this bug.