1036654 - Report the tile fragmentation overhead

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Benoit Girard (:BenWa)]

Attachment: patch

We've been guessing a lot about how much memory we're losing to tile fragmentation. We should track it properly so what we can make better decissions if the performance justifies the memory overhead.

Comment 1

[Nicholas Nethercote [inactive]]

Comment on attachment 8453377 [details] [diff] [review]
patch

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

I never particularly like memory reporters that involve analytical size computations -- too easy to get wrong, too hard to verify -- but I guess there's no other way in this case?

::: gfx/gl/GfxTexturesReporter.cpp
@@ +13,4 @@
>  NS_IMPL_ISUPPORTS(GfxTexturesReporter, nsIMemoryReporter)
>  
>  int64_t GfxTexturesReporter::sAmount = 0;
> +int64_t GfxTexturesReporter::sTileFragmentationAmount = 0;

Should these be Atomic<>? Can they be updated on different threads?

::: gfx/gl/GfxTexturesReporter.h
@@ +51,5 @@
>                                nsISupports* aData, bool aAnonymize)
>      {
> +        MOZ_COLLECT_REPORT("gfx-tiles-fragmentation", KIND_OTHER, UNITS_BYTES,
> +            sTileFragmentationAmount,
> +            "Memory lost due to rounding tiles to 256.");

"256 bytes"?

@@ +60,4 @@
>  
>  private:
>      static int64_t sAmount;
> +    // Count the ammount of memory lost to internal tile fragmentation

s/ammount/amount/

::: gfx/layers/client/TiledContentClient.cpp
@@ +948,5 @@
> +    nsIntRect(aTileOrigin.x, aTileOrigin.y,
> +              GetScaledTileSize().width, GetScaledTileSize().height);
> +  // Intersect this area with the portion that's invalid.
> +  tileRegion = tileRegion.Sub(tileRegion, GetValidRegion());
> +  tileRegion = tileRegion.Sub(tileRegion, aDirtyRegion); // Has now been validated

I can't meaningfully review this part. You might want to get a gfx person to check it.

@@ +952,5 @@
> +  tileRegion = tileRegion.Sub(tileRegion, aDirtyRegion); // Has now been validated
> +
> +  int fragmentationArea = 0;
> +  nsIntRegionRectIterator it(tileRegion);
> +  for (const nsIntRect* rect = it.Next(); rect != nullptr; rect = it.Next()) {

Nit: I'd just use |rect| for the condition.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8453377 [details] [diff] [review]
patch

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

This seems ok to me, but I'm not that confident in my knowledge of this part of the code.

::: gfx/gl/GfxTexturesReporter.h
@@ +51,5 @@
>                                nsISupports* aData, bool aAnonymize)
>      {
> +        MOZ_COLLECT_REPORT("gfx-tiles-fragmentation", KIND_OTHER, UNITS_BYTES,
> +            sTileFragmentationAmount,
> +            "Memory lost due to rounding tiles to 256.");

"Memory lost due to tiles extending past content boundaries"

::: gfx/layers/client/TiledContentClient.cpp
@@ +955,5 @@
> +  nsIntRegionRectIterator it(tileRegion);
> +  for (const nsIntRect* rect = it.Next(); rect != nullptr; rect = it.Next()) {
> +    fragmentationArea += rect->Width() * rect->Height();
> +  }
> +  fragmentationArea = fragmentationArea * mResolution * mResolution;

Can you just use tileRegion.Area() * mResolution * mResolution?

Comment 3

[Benoit Girard (:BenWa)]

Comment on attachment 8453377 [details] [diff] [review]
patch

I'll bounce it to Cwiiis too then.

Comment 4

[Chris Lord [:cwiiis]]

Comment on attachment 8453377 [details] [diff] [review]
patch

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

I'd much rather we call this 'tile waste' rather than 'tile fragmentation' - I don't think fragmentation really describes what this is and has other connotations, plus I've never heard it called fragmentation before (not to say that it isn't, but I've only ever previously heard it referred to as waste/wastage).

LGTM modulo the comments. If you have good reason to call it fragmentation, I wouldn't argue, but I think it'd be clearer and more concise to call it waste.

::: gfx/gl/GfxTexturesReporter.h
@@ +51,5 @@
>                                nsISupports* aData, bool aAnonymize)
>      {
> +        MOZ_COLLECT_REPORT("gfx-tiles-fragmentation", KIND_OTHER, UNITS_BYTES,
> +            sTileFragmentationAmount,
> +            "Memory lost due to rounding tiles to 256.");

I agree with kats's change suggestion.

::: gfx/layers/client/TextureClient.h
@@ +331,5 @@
>     */
>    virtual void WaitForBufferOwnership() {}
>  
> +  /**
> +   * Track how much of this texture is used by fragmentation.

'how much of this texture is used by fragmentation'?

How about just 'How much of this texture is unused'?

::: gfx/layers/client/TiledContentClient.cpp
@@ +944,4 @@
>    ctxt = nullptr;
>    drawTarget = nullptr;
>  
> +  nsIntRegion tileRegion =

Let's not call this tileRegion and leave this segment of code uncommented, it's confusing - Add a comment as to what this is doing (calculating tile waste) and rename this to something like tileWasteRegion.

Comment 5

[bhavana bajaj [:bajaj]]

:milan, so this should be be blocking 2.0+ then as it helps https://bugzilla.mozilla.org/show_bug.cgi?id=1036654. Can you please confirm?

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

I don't think we need to block on this for 2.0. This helps us identify the issues we may have, but most of those will show up on the trunk and we can just uplift any fixes to problems we may find.

Comment 7

[Benoit Girard (:BenWa)]

Attachment: patch v2

Comment 8

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5607eeaa106

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/4829189e7999 for Windows build bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=43961195&tree=Mozilla-Inbound

Comment 10

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5607eeaa106

Comment 11

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=1caacf56303c

Comment 12

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e62aa8c88533

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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