Closed Bug 1036654 Opened 6 years ago Closed 6 years ago

Report the tile fragmentation overhead

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34
tracking-b2g backlog

People

(Reporter: BenWa, Assigned: BenWa)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
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.
Attachment #8453377 - Flags: review?(n.nethercote)
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.
Attachment #8453377 - Flags: review?(n.nethercote) → review+
Attachment #8453377 - Flags: review?(bugmail.mozilla)
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?
Attachment #8453377 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8453377 [details] [diff] [review]
patch

I'll bounce it to Cwiiis too then.
Attachment #8453377 - Flags: review?(chrislord.net)
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.
Attachment #8453377 - Flags: review?(chrislord.net) → review+
: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?
Flags: needinfo?(milan)
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.
blocking-b2g: --- → backlog
Flags: needinfo?(milan)
Attached patch patch v2Splinter Review
Assignee: nobody → bgirard
Attachment #8453377 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8457443 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/e62aa8c88533
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Flags: qe-verify-
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.