608983 - [OGL] Floating ThebesSurface rendering is very slow

Status: RESOLVED WONTFIX

(Core :: Graphics, defect)

Description

[Oleg Romashin (:romaxa)]

I was playing with GL backend and found that we do "create/resize/destroy" TextureImage (CreateBuffer) calls for each "awesome/sidepanel" move event.


This looks very bad, because slow sliding of urlbar and side panels causing zillions ~70 TextureImage create/destroy calls.

Not sure if it is known problem.

Comment 1

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

ISTR Ben Stover noticing this before.  If the memory tradeoff isn't too bad, we should definitely permanently retain the URL bar and sidebars.

Comment 2

[Benjamin Stover (:stechz)]

Is there an underlying problem here though? Is this because layout will create a layer when scrolling begins and then merge the newly created layer back into its parent too quickly?

Comment 3

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

Churning textures is much more expensive than churning the heap.  We don't want to do that for the fennec UI.

Comment 4

[Benjamin Stover (:stechz)]

For instance, this could really hurt animation performance too for content.

Since creating these buffers are expensive (I think more so for mobile than for desktop since this is shared memory, right?), in general we probably want to be much more conservative about creating and destroying layers. Either that, or we need to keep some memory around for these temporary layers, just in case.

Comment 5

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

Content is less susceptible to this problem because we lie about what's visible.

Comment 6

[Benjamin Stover (:stechz)]

Yes, but won't content animations have the same tendency?

Comment 7

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

What tendency?

Note that avoiding thebeslayer churn in the UI will also be a win for SW only.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think the problem here is that any time we resize a ThebesLayerOGL, if the new size is larger than the old size, we allocate a new texture of exactly the new size. So we need to do a texture allocation at every frame if the ThebesLayer is growing steadily.

The only way to avoid this is to use a different allocation strategy, for example by allocating a texture that's larger than needed whenever we have to create a new one.

Comment 9

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

We do the same for BasicThebesLayer.  To move away from exact-sized textures for GL thebes layers, we would need to either write a new shader program or disable buffer rotation.

Agree that we need a smarter allocation strategy (although I think both for SW and GL).  There are a few simple things we can do.  This is going to happen in the fennec4 timeline, should help desktop too.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #9)
> We do the same for BasicThebesLayer.

Yes, but as you said in comment #3, texture churn has a bigger impact.

> To move away from exact-sized textures
> for GL thebes layers, we would need to either write a new shader program or
> disable buffer rotation.

OK.

> Agree that we need a smarter allocation strategy (although I think both for SW
> and GL).  There are a few simple things we can do.  This is going to happen in
> the fennec4 timeline, should help desktop too.

What are those simple things?

Comment 11

[Oleg Romashin (:romaxa)]

> example by allocating a texture that's larger than needed whenever we have to
> create a new one.

Would it be possible to just allocate full "URL-BAR" size layer as soon as URL-BAR start appearing on the screen?  and drop it only when it fully invisible.

Comment 12

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

(In reply to comment #10)
> > Agree that we need a smarter allocation strategy (although I think both for SW
> > and GL).  There are a few simple things we can do.  This is going to happen in
> > the fennec4 timeline, should help desktop too.
> 
> What are those simple things?

Same kinds of tricks folks use to avoid array resizes: allocate extra space initially, grow by factors or chunks, allocate maximum size initially, some combination thereof.  That code is easy, the hard part is getting the memory tradeoff right.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #11)
> Would it be possible to just allocate full "URL-BAR" size layer as soon as
> URL-BAR start appearing on the screen?  and drop it only when it fully
> invisible.

If you use CSS transitions to move the URL bar into position, then we could add stuff to layout to correctly size the layer and pre-render its contents so the animation is smooth. This would also be necessary for when we use the compositor process to drive animation.

Comment 14

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

(In reply to comment #9)
> To move away from exact-sized textures
> for GL thebes layers, we would need to either write a new shader program or
> disable buffer rotation.

Actually, now that clipping works in the GL backend, I don't believe that's true anymore.  Disregard.

Comment 15

[Oleg Romashin (:romaxa)]

> Same kinds of tricks folks use to avoid array resizes: allocate extra space
> initially, grow by factors or chunks, allocate maximum size initially, some

no, I'm not talking about heuristics...  I'm talking about well-known url-bar size on XUL (UI/fennec/layout) side and give that info to ThebesBuffer and cache it properly

Comment 16

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

I wasn't talking about fennec's XUL elements, so we're even.

Comment 17

[Oleg Romashin (:romaxa)]

Even with Lock/Unlock extension (Fast upload) we are really slow on awesome-bar view...

With Remote ThebesLayer (Shadow) we upload textures data once and scroll it with maximum speed... but for Local thebes layer we repaint/upload texture data on each scroll event.

Comment 18

[Oleg Romashin (:romaxa)]

Attachment: SVG graph for awesomebar scrolling

Sounds like we are spending lot of time in repainting text url entries

Comment 19

[Oleg Romashin (:romaxa)]

we do repaint thebes layers entries content every scroll event (scrolled-into-view), and that is causing slow scrolling (~24-30 FPS) comparing to web content remote frame scrolling  which is 60 FPS.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So for this, we probably want something like the displayport stuff for normal chrome scrollframes. We can pre-render content into the scrolled ThebesLayer that's outside the visible region of the ThebesLayer.

Comment 21

[Oleg Romashin (:romaxa)]

> If you use CSS transitions to move the URL bar into position, then we could add

is it really possible? I though transition it is some programmed transition from point A to point B, and between those points we can create special layer and animate it smoothly, but in this case each movement not predictable,

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You're right, I was wrong.

Comment 23

[Oleg Romashin (:romaxa)]

Ok, solution I see here is some XUL CSS pref which help to freeze ThebesLayer size.

Comment 24

[Benjamin Stover (:stechz)]

This is not just a problem for the URL bar though! Any layer that is getting scrolled off the screen (or the displayport) is going to churn like this. This is especially bad for Fennec in the content process since this memory is shared and double buffered.

I think we need to be smarter about this in general. Any thoughts?

Comment 25

[Benjamin Stover (:stechz)]

A rather straightforward solution: always allocate X pixels more in a dimension if the buffer is not big enough. After a timeout we could shrink this to the exact size, but it would protect us during these patterns when the width or height of a visible region is consistently growing.

Comment 26

[Benjamin Stover (:stechz)]

> Same kinds of tricks folks use to avoid array resizes: allocate extra space
> initially, grow by factors or chunks, allocate maximum size initially, some
> combination thereof.  That code is easy, the hard part is getting the memory
> tradeoff right.

Err, it seems I've just arrived to the discussion, sorry. Do carry on.

Comment 27

[Florian Hänel [:heeen]]

Maybe some kind of cache-policy attribute?
Are there cases where we actually don't want to create memory for the whole possibly visible layer?

Comment 28

[Florian Hänel [:heeen]]

Attachment: pre-allocate space for thebes layers

Here's a patch that tries to find the appropriate size for buffers before clipping. On egl we destroy and recreate buffers for every pixel panned in. These buffers then have to be repainted from scratch. By creating buffers of the right size right away, we re-use layer buffers as much as possible and can only paint areas newly exposed. Another idea might be to disable clipping for those layers somehow so we can paint them in their entirity and don't do any thebes drawing while panning.

Comment 29

[Florian Hänel [:heeen]]

Attachment: weird paint issue

There's an issue with the awesomebar, if I don't clear the paint area before exiting BasicBufferOGL::BeginPaint, it looks weird - see the screenshot. 
This only happens in the awesomebar, but not the side bars. I find it confusing that only the background is clipped to the most recent paint event, but the text and some seperator lines are appearing across the whole buffer.

Comment 30

[Oleg Romashin (:romaxa)]

it might be possible to use layer="true" xul attribute or something like that and pre-allocate space only for some layers (not for all)

Comment 31

[Oleg Romashin (:romaxa)]

also it worse to  try animate fennec bars with CSS transform, similar to https://bug623485.bugzilla.mozilla.org/attachment.cgi?id=506918

Comment 32

[Florian Hänel [:heeen]]

Attachment: pre-allocate space for thebes layers: rebased, artifact-free

I rebased my patch and did some intensive debugging. The artifact above stems from a concave regeion to draw. While image backgrounds seem to be limited to the actual region to draw, text and lines seem to draw outside the region, which leads to text without proper background.
This is fixed by doing an explicit thebes clip on the paint region.
The question is: why do we have a concave draw region in the first place? I found that the awesomebar gets invalidated by a narrow area of 6 pixel width while panning it into view on about:blank even though that content doesn't even have scrollbars; also there is no layer of that size at that point.
I traced this back to this call: http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#810
Apparently this invalid region seems to stick around even though the layer that caused the invalidation is no longer active, Maybe :roc or someone else can shed some light into this.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Currently our invalidation code doesn't track exactly which ThebesLayer contains invalid content. So sometimes we invalidate layers that don't really need to be invalidated.

Also, due to bug 648483 we sometimes invalidate the wrong area.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So with your patch, if we have a very large element that is clipped to the window in a layer, we'll actually create a buffer for the layer that is the size of the element? That could be bad.

I thought maybe the right approach here would be to allocate buffers whose dimensions come from some fixed list, e.g. powers of 2, or maybe rounded powers of 1.5 or something like that. Then small changes in the layer size are likely to get the same buffer size so we don't have to reallocate.

Comment 35

[Florian Hänel [:heeen]]

The downside of your suggestion is waste of memory.
I propose a future addition to this patch that lets the backend decide on how big it really creates the buffer - e.g. at most the main window or the screen or the max texture size.
This patch also creates the possibility of pre-painting the floating bars once in their entire size, so we don't do any painting while panning.
Plus, at the moment as the layers get out of view they get recycled completely and get recreated on demand. This is bad as it adds noticable lag to the user experience. We should either keep the layers from being recycled or don't free their backing images so when a new layer is requested it could try to find one with the right size already existing or actually recycle/create one.
Image sizes do repeat apart from the exact same layer being revived, as icons and thumbnails often have the same size.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #35)
> The downside of your suggestion is waste of memory.

Sometimes yes; anything we do here will waste memory. We really need some data on how much memory is used/wasted over a representative set of pages.

> I propose a future addition to this patch that lets the backend decide on how
> big it really creates the buffer - e.g. at most the main window or the screen
> or the max texture size.

We would need something like that as part of this patch because at minimum, it shouldn't be possible for a page with a single large transformed element to consume a max-texture-size chunk of VRAM.

> This patch also creates the possibility of pre-painting the floating bars once
> in their entire size, so we don't do any painting while panning.
> Plus, at the moment as the layers get out of view they get recycled completely
> and get recreated on demand. This is bad as it adds noticable lag to the user
> experience.

I don't think it does on desktop generally. So we may need different policies for mobile vs desktop.

> We should either keep the layers from being recycled or don't free
> their backing images so when a new layer is requested it could try to find one
> with the right size already existing or actually recycle/create one.

That sounds good.

A good way to proceed might be to break the patch into two pieces: one which passes down more information to the layer backends and creates hooks for an allocation policy, and another piece that implements different policies and lets us switch between them via a pref or something like that.

Comment 37

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: rebased

This is just the same patch rebased against current mozilla-central

Comment 38

[Florian Hänel [:heeen]]

Attachment: added size restriction

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 530610 [details] [diff] [review]
added size restriction

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

Please split the patch up into a patch which defines the Layers.h API and calls it from FrameLayerBuilder, and a separate patch to add GL support.

::: gfx/layers/Layers.h
@@ +579,5 @@
>      mVisibleRegion = aRegion;
>      Mutated();
>    }
>  
> +  virtual void SizeHint(const nsIntRect& aSizeHint) { };

Call this "SetUnclippedSize" and carefully describe in a comment what it means.

@@ +580,5 @@
>      Mutated();
>    }
>  
> +  virtual void SizeHint(const nsIntRect& aSizeHint) { };
> +  virtual const nsIntRect GetSizeHint(){ return nsIntRect(0,0,0,0);}

I don't think we need GetSizeHint to be defined here.

::: layout/base/FrameLayerBuilder.cpp
@@ +1136,5 @@
>      mVisibleRegion.Or(mVisibleRegion, aVisibleRect);
>      mVisibleRegion.SimplifyOutward(4);
>      mDrawRegion.Or(mDrawRegion, aDrawRect);
>      mDrawRegion.SimplifyOutward(4);
> +    if(!aVisibleRect.IsEmpty()) {

space after "if"

@@ +1322,5 @@
>        itemContent.IntersectRect(aClip.mClipRect, itemContent);
>      }
>      mBounds.UnionRect(mBounds, itemContent);
>      nsIntRect itemDrawRect = itemContent.ToOutsidePixels(appUnitsPerDevPixel);
> +    nsIntRect unclippedItemDrawRect = unclippedItemContent.ToOutsidePixels(appUnitsPerDevPixel);

The setup of unclippedItemDrawRect can be moved down to just before you call FindThebesLayerFor.

Comment 40

[Florian Hänel [:heeen]]

Attachment: fixed nits

this patch lives as part of a patchset here:
https://hg.mozilla.org/users/florian.haenel_heeen.de/GLES_layers

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #39)
> Please split the patch up into a patch which defines the Layers.h API and
> calls it from FrameLayerBuilder, and a separate patch to add GL support.

Comment 42

[Florian Hänel [:heeen]]

Attachment: Part 1: layers API change

Comment 43

[Florian Hänel [:heeen]]

Attachment: Part 2: thebesbufferOGL implementation

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 533226 [details] [diff] [review]
Part 1: layers API change

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

::: gfx/layers/Layers.h
@@ +588,5 @@
> +  * We use a rectangle instead of just the size so content that is scrolled in from either left and right
> +  * or up and down can preallocate surfaces on the opposing side respectively.
> +  */
> +  virtual void SetUnclippedSize(const nsIntRect& aSizeHint) { };
> +  virtual const nsIntRect GetUnclippedSize();

Should not return const; there's no point in returning const values that aren't references.

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 533227 [details] [diff] [review]
Part 2: thebesbufferOGL implementation

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

This patch scares me because it makes it easy for pages to consume unlimited quantities of VRAM.

Maybe we should restrict use of the size hint to elements in chrome documents only?

::: gfx/layers/opengl/ThebesLayerOGL.cpp
@@ +483,5 @@
>          // The buffer's big enough but doesn't contain everything that's
>          // going to be visible. We'll move it.
>          destBufferRect = nsIntRect(neededRegion.GetBounds().TopLeft(), mBufferRect.Size());
>        }
> +    } else if(!sizeHint.IsEmpty()) {

Space between "if" and "("

@@ +484,5 @@
>          // going to be visible. We'll move it.
>          destBufferRect = nsIntRect(neededRegion.GetBounds().TopLeft(), mBufferRect.Size());
>        }
> +    } else if(!sizeHint.IsEmpty()) {
> +      destBufferRect = sizeHint;

This isn't right. destBufferDims needs to be the scaled size of destBufferRect, but you're breaking that if neededRegion.GetBounds() doesn't fit into sizeHint.Size().

This gets simpler when the resolution code is removed from here, which I have patches to do (which I will attach in bug 637852).

@@ +757,5 @@
> +  // clip and draw regions.
> +  result.mContext->NewPath();
> +  for(nsIntRegionRectIterator iter = result.mRegionToDraw; const nsIntRect *R = iter.Next();)
> +    result.mContext->Rectangle(gfxRect(R->x, R->y, R->width, R->height));
> +  result.mContext->Clip();

Use gfxUtils::ClipToRegion(Snapped).

Hmm, is there a bug on trunk without this code? If so, why haven't we seen it?

Comment 46

[Florian Hänel [:heeen]]

You don't see it because on resize it reallocates the whole image, which in turn repaints the whole visible area.

Comment 47

[Florian Hänel [:heeen]]

Attachment: Part 2 - thebesbuffer implementation

Rebased and added a check to only apply sizehint if in ui process. Can you elaborate on the scaling issue? I'm not sure if I completely grok it.

Comment 48

[Oleg Romashin (:romaxa)]

Comment on attachment 533671 [details] [diff] [review]
Part 2 - thebesbuffer implementation


>   float curXRes = mLayer->GetXResolution();
>   float curYRes = mLayer->GetYResolution();
>+  nsIntRect sizeHint;
>+  if (XRE_GetProcessType() == GeckoProcessType_Default) {
You don't need this check, because ThebesLayerOGL and any OGL layers are always in Chrome process...


>+    // we only use the size hint in chrome to prevent 
>+    // pages from hogging huge amounts of vram
>+    sizeHint = mLayer->GetUnclippedSize();
>+  }
>+

Comment 49

[Oleg Romashin (:romaxa)]

Comment on attachment 533671 [details] [diff] [review]
Part 2 - thebesbuffer implementation


>   /** Layer implementation */
>   void SetVisibleRegion(const nsIntRegion& aRegion);
>+  void SetUnclippedSize(const nsIntRect& aUnclippedSize){ mUnclippedSize=aUnclippedSize; };
>+  nsIntRect GetUnclippedSize() { return mUnclippedSize; };
this should be const

Comment 50

[Florian Hänel [:heeen]]

romaxa: see #c44

Comment 51

[Oleg Romashin (:romaxa)]

Comment on attachment 533671 [details] [diff] [review]
Part 2 - thebesbuffer implementation

I guess this patch need to be updated...
if (XRE_GetProcessType() == GeckoProcessType_Default) {, + problems with const functions

Comment 52

[Chris Lord [:cwiiis]]

Attachment: Part 1: layers API change (rebased)

This is the first patch, rebased on today's mozilla-central, in case it's useful to anyone.

Comment 53

[Chris Lord [:cwiiis]]

Attachment: Part 2 - thebesbuffer implementation (rebased)

And the second patch, rebased on today's mozilla-central.

Comment 54

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 533671 [details] [diff] [review]
Part 2 - thebesbuffer implementation

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

Sorry about the super-long delay. You should have nagged me!

::: gfx/layers/opengl/ThebesLayerOGL.cpp
@@ +434,5 @@
> +  nsIntRect sizeHint;
> +  if (XRE_GetProcessType() == GeckoProcessType_Default) {
> +    // we only use the size hint in chrome to prevent 
> +    // pages from hogging huge amounts of vram
> +    sizeHint = mLayer->GetUnclippedSize();

This is going to return true for desktop Mac and Linux Firefox. We don't want that.

We probably need a new layer content flag to indicate whether we should use these size hints. Layout would set that flag for content derived from chrome docshells (e.g. by checking nsPresContext::IsChrome).

Comment 55

[Florian Hänel [:heeen]]

The additional flag works out nicely. I ave been trying to get it to render the content unclipped as well, unsuccessful so far. Can someone give me any pointers on how to aproach this? I dug into the DisplayList generation stuff trying to suppress the nsDisplayClip items from being generated, but I didn't find the right place yet.

Comment 56

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why do you want to not clip content? I'm not sure what you're trying to do there.

Comment 57

[Florian Hänel [:heeen]]

When we already allocate buffers of the right size, we should also preemptivly paint them for common UI elements like the top and side bars even if they are not fully visible.
By the way, :cwiiis found that we paint the scrollbars even if they only change their position, not their shape and that preventing that gives us a performance boost, but that's for another bug.

Comment 58

[Robert O'Callahan (:roc) (email my personal email if necessary)]

For that, we'll need to modify some ComputeVisibility methods in nsDisplayList.cpp to compute a larger-than-normal visible rect.

Comment 59

[Gregory Pappas [:gregp]]

OpenGL Layers are gone