Closed Bug 1037220 Opened 10 years ago Closed 10 years ago

Reorder rendering to avoid render target switches

Categories

(Core :: Graphics, defect, P2)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: jrmuizel, Assigned: jrmuizel)

References

Details

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

Attachments

(2 files, 4 obsolete files)

Right now we will for example draw the background, switch to a container layers's render target, draw to that, switch back the render target draw the container layer, move on to the next container layer etc. This render target switching is especially bad for mobile gpus (like adreno that needs to move things in and out of gmem). This patch switches to a two pass approach that renders all of the container layers first and then composites everything all together.
This patch nearly doubles the framerate during the app -> homescreen transition, but will cause our peak memory usage to increase as we now retain all of the child temporary buffers until they are composited.
Attachment #8454099 - Attachment is patch: true
Attachment #8454099 - Attachment mime type: text/x-patch → text/plain
Attachment #8454099 - Attachment is obsolete: true
This breaks out the original ContainerRender into three functions. ContainerPrepare, ContainerRender, and RenderLayers ContainerPrepare produces a PreparedData that contains all the information that it computed and is used by ContainerRender.
Attachment #8455451 - Attachment is obsolete: true
Attachment #8455631 - Flags: review?(matt.woodrow)
Attachment #8455631 - Flags: review?(bgirard)
Have we investigated why the app -> homescreen transition needs an intermediate surface? It seems like avoiding that would be an even bigger win. It would also be nice to add tools for gaia developers to use to diagnose this particular performance problem.
(In reply to Matt Woodrow (:mattwoodrow) from comment #4) > Have we investigated why the app -> homescreen transition needs an > intermediate surface? It seems like avoiding that would be an even bigger > win. It would also be nice to add tools for gaia developers to use to > diagnose this particular performance problem. Jeff and I discussed avoiding this. We would have to avoid using opacity. I think if we replace the transforms with a fancier animation we could remove the opacity and still have a nice feel. Otherwise we would have to make sure the layer tree only has one child. I think the only way to guarantee that is to use a screenshot layer which isn't ideal. bug 864829 covers adding performance warnings for things that should be avoided and include this as an example.
Attachment #8455631 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8455631 [details] [diff] [review] Patch with logging and trimed reftests for reproducing Review of attachment 8455631 [details] [diff] [review]: ----------------------------------------------------------------- r+ with style fix + removing redundant SetRenderTarget switches for nested container layers. ::: gfx/layers/composite/ContainerLayerComposite.cpp @@ +283,5 @@ > + layer(aLayer), clipRect(aClipRect), restoreVisibleRegion(aRestoreVisibleRegion), savedVisibleRegion(aVisibleRegion) {} > + LayerComposite* layer; > + nsIntRect clipRect; > + bool restoreVisibleRegion; > + nsIntRegion savedVisibleRegion; These member should have the m prefix. Even if it's a struct. @@ +346,2 @@ > > compositor->SetRenderTarget(surface); I think this needs to be closer to the render call otherwise we might do needless switch. It might not paint but at least we wont bet on the driver optimizing this for us. ::: gfx/layers/composite/LayerManagerComposite.h @@ +322,5 @@ > > virtual Layer* GetLayer() = 0; > > virtual void RenderLayer(const nsIntRect& aClipRect) = 0; > + virtual void Prepare(const nsIntRect& aClipRect) {} Again I'm a big fan of method documentation. /** * Perform a first pass over the layer tree to prepare intermediate surfaces. * This allows us on to avoid framebuffer switches in the middle of our render * which is inefficient. */
Attachment #8455631 - Flags: review?(bgirard) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #4) > Have we investigated why the app -> homescreen transition needs an > intermediate surface? It seems like avoiding that would be an even bigger > win. It would also be nice to add tools for gaia developers to use to > diagnose this particular performance problem. I filed bug 1037092 on this issue.
Attached patch With review comments applied (obsolete) (deleted) — Splinter Review
Attachment #8455631 - Attachment is obsolete: true
Attached patch Rebased (deleted) — Splinter Review
Attachment #8456412 - Attachment is obsolete: true
The original patch broke the copying of the background that we do because we don't have a background to copy. This is the interdiff of a patch that only renders to an intermediate early when we don't need to copy the background.
Attachment #8458248 - Flags: review?(matt.woodrow)
Attachment #8458248 - Flags: review?(matt.woodrow) → review+
Are you using an ancient version of hg that's still affected by the long-ago-fixed (like 2.5 or 2.6 timeframe) rebase bug? Because your push touched a ton of Places test files that I don't think were included in your patch. If so, please update hg ASAP. There's nothing that can be done at this point to un-do the history munging this push caused, but we can at least try to avoid it happening again. Thanks!
Assignee: nobody → jmuizelaar
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Keywords: perf
Priority: -- → P2
Whiteboard: [c=fps p= s= u=]
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #11) > Are you using an ancient version of hg that's still affected by the > long-ago-fixed (like 2.5 or 2.6 timeframe) rebase bug? Because your push > touched a ton of Places test files that I don't think were included in your > patch. If so, please update hg ASAP. There's nothing that can be done at > this point to un-do the history munging this push caused, but we can at > least try to avoid it happening again. > > Thanks! Yeah. I was using 2.3. I've fixed that now.
Blocks: 1087530
Blocks: 1092360
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: