Closed Bug 990608 Opened 6 years ago Closed 6 years ago

Make tile size configurable

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: cwiiis, Assigned: cwiiis)

Details

Attachments

(1 file)

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).
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.
Attachment #8400032 - Flags: review?(tnikkel)
Attachment #8400032 - Flags: review?(nical.bugzilla)
Attachment #8400032 - Flags: review?(bgirard)
Attachment #8400032 - Flags: review?(tnikkel) → review+
Attachment #8400032 - Flags: review?(nical.bugzilla) → review+
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.
Attachment #8400032 - Flags: review?(bgirard) → review+
(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.
(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
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
(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.
ugh, it is a divide by zero, because I'm an idiot and this is an integer division. Will fix.
(which is only triggerred by low precision rendering)
https://hg.mozilla.org/mozilla-central/rev/efd2b6b51237
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.