990608 - Make tile size configurable

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Chris Lord [:cwiiis]]

We waste a lot of memory using tiles on some devices because our default tile size (256x256) is not that well-aligned to screen sizes (320x480, 480x854, 540x960).

We've traditionally picked powers of two due to driver limitations and mobile chipset behaviour, but this doesn't appear to be such an issue anymore and it'd be good to test this on a case-by-case basis. We may also determine that the memory wastage is a bigger issue than the possible impact on performance, (assuming the driver doesn't end up wasting memory with NPOT textures).

Comment 1

[Chris Lord [:cwiiis]]

Attachment: Make tile size configurable

nical for Texture-related changes, BenWa for tile-related changes, tn for the small layout related stuff.

Note, this patch allows for rectangular tiles (which works fine), and also fixes a bug where setting the tile alignment to 1 was still causing alignment code to run.

Comment 2

[Benoit Girard (:BenWa)]

Comment on attachment 8400032 [details] [diff] [review]
Make tile size configurable

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

Nice work!

::: gfx/layers/TiledLayerBuffer.h
@@ +117,3 @@
>  
>  #ifdef MOZ_WIDGET_ANDROID
> +  MOZ_NEVER_INLINE // bug 881018 causes wrong results when GetScaledTileSize is inlined

I wonder if this could go away now?

::: gfx/thebes/gfxPrefs.h
@@ +159,5 @@
>    DECL_GFX_PREF(Once, "layers.dump",                           LayersDump, bool, false);
>    DECL_GFX_PREF(Once, "layers.enable-tiles",                   LayersTilesEnabled, bool, false);
>    DECL_GFX_PREF(Once, "layers.simple-tiles",                   LayersUseSimpleTiles, bool, false);
>    DECL_GFX_PREF(Once, "layers.force-per-tile-drawing",         PerTileDrawing, bool, false);
> +  DECL_GFX_PREF(Once, "layers.tile-width",                     LayersTileWidth, int32_t, 256);

You're allowing non square tile so that we can adjust it per device and avoid having the edges allocate a tile and only consume say 10% on the average/common case. I believe this should be document in the source code. Either here or somewhere else where you see best fit.

I really wished we would of created a tile namespace in the prefs like layers.tile.XXX.

::: layout/base/nsLayoutUtils.cpp
@@ +718,5 @@
>  
>          nsIScrollableFrame* scrollableFrame = frame->GetScrollTargetFrame();
>          nsPoint scrollPos(
>            scrollableFrame ? scrollableFrame->GetScrollPosition() : nsPoint(0,0));
> +        if (marginsData->mAlignmentX > 1 || marginsData->mAlignmentY > 1) {

Kats believe that this should remain > 0. Can you explain why this changed to > 1?

::: widget/xpwidgets/APZCCallbackHelper.cpp
@@ +45,5 @@
>    displayPortInLayerSpace.Inflate(1);
>  
>    // Now nudge the rectangle to the nearest equal or larger tile boundary.
> +  gfxFloat left = gfxPrefs::LayersTileWidth()
> +    * floor(displayPortInLayerSpace.x / gfxPrefs::LayersTileWidth());

Where going to lose some nice compiler optimizations here :( (assuming it even did them). But the memory/bandwidth saving must be worth these extra cycles.

Comment 3

[Chris Lord [:cwiiis]]

(In reply to Benoit Girard (:BenWa) from comment #2)
> ::: gfx/layers/TiledLayerBuffer.h
> @@ +117,3 @@
> >  
> >  #ifdef MOZ_WIDGET_ANDROID
> > +  MOZ_NEVER_INLINE // bug 881018 causes wrong results when GetScaledTileSize is inlined
>
> I wonder if this could go away now?

I believe so. I'll remove it, I guess let's just keep an eye out on the crash stats - We've probably bumped the compiler version a few times since then, so maybe it was already fixed anyway.

> ::: gfx/thebes/gfxPrefs.h
> @@ +159,5 @@
> >    DECL_GFX_PREF(Once, "layers.dump",                           LayersDump, bool, false);
> >    DECL_GFX_PREF(Once, "layers.enable-tiles",                   LayersTilesEnabled, bool, false);
> >    DECL_GFX_PREF(Once, "layers.simple-tiles",                   LayersUseSimpleTiles, bool, false);
> >    DECL_GFX_PREF(Once, "layers.force-per-tile-drawing",         PerTileDrawing, bool, false);
> > +  DECL_GFX_PREF(Once, "layers.tile-width",                     LayersTileWidth, int32_t, 256);
> 
> You're allowing non square tile so that we can adjust it per device and
> avoid having the edges allocate a tile and only consume say 10% on the
> average/common case. I believe this should be document in the source code.
> Either here or somewhere else where you see best fit.

I'll document it here, good catch. Bad me for not documenting it :(

> I really wished we would of created a tile namespace in the prefs like
> layers.tile.XXX.

We should file a follow-up, I agree.

> ::: layout/base/nsLayoutUtils.cpp
> @@ +718,5 @@
> >  
> >          nsIScrollableFrame* scrollableFrame = frame->GetScrollTargetFrame();
> >          nsPoint scrollPos(
> >            scrollableFrame ? scrollableFrame->GetScrollPosition() : nsPoint(0,0));
> > +        if (marginsData->mAlignmentX > 1 || marginsData->mAlignmentY > 1) {
> 
> Kats believe that this should remain > 0. Can you explain why this changed
> to > 1?

Actually kats is right, I misread this code. Will change back.

> ::: widget/xpwidgets/APZCCallbackHelper.cpp
> @@ +45,5 @@
> >    displayPortInLayerSpace.Inflate(1);
> >  
> >    // Now nudge the rectangle to the nearest equal or larger tile boundary.
> > +  gfxFloat left = gfxPrefs::LayersTileWidth()
> > +    * floor(displayPortInLayerSpace.x / gfxPrefs::LayersTileWidth());
> 
> Where going to lose some nice compiler optimizations here :( (assuming it
> even did them). But the memory/bandwidth saving must be worth these extra
> cycles.

I could read the width/height into local variables and group the width/height divisions to increase the chance that we might get some kind of optimisation? It wouldn't hurt anyway.

Comment 4

[Chris Lord [:cwiiis]]

Changes made and pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/bf184a7a1d63

Comment 5

[Benoit Girard (:BenWa)]

(In reply to Chris Lord [:cwiiis] from comment #3)
> I could read the width/height into local variables and group the
> width/height divisions to increase the chance that we might get some kind of
> optimisation? It wouldn't hurt anyway.

This was mostly be me complaining uselessly. If you can't find anything obvious it's probably ok as-is. Memory bandwidth > a few cpu cycles

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbb81640ea16

https://tbpl.mozilla.org/php/getParsedLog.php?id=37291539&tree=Mozilla-Inbound et al.

Comment 7

[Chris Lord [:cwiiis]]

This may have been due to the removal of the MOZ_NEVER_INLINE... (it appears the crash is happening in GetScaledTileSize) Pushed to try to find out:

https://tbpl.mozilla.org/?tree=Try&rev=6f6eb712cce1

Comment 8

[Chris Lord [:cwiiis]]

(In reply to Chris Lord [:cwiiis] from comment #7)
> This may have been due to the removal of the MOZ_NEVER_INLINE... (it appears
> the crash is happening in GetScaledTileSize) Pushed to try to find out:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=6f6eb712cce1

ok, nope, obviously not the issue - I'll look into it properly then.

Comment 9

[Chris Lord [:cwiiis]]

ugh, it is a divide by zero, because I'm an idiot and this is an integer division. Will fix.

Comment 10

[Chris Lord [:cwiiis]]

(which is only triggerred by low precision rendering)

Comment 11

[Chris Lord [:cwiiis]]

Fixed, pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/efd2b6b51237

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/efd2b6b51237