818060 - Add a memory reporter for graphics textures

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

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

:jrmuizel tells me that the graphics code doesn't have proper memory reporters and that (on Android) most of the memory is just lumped under the pvrsrvkm smaps entry. We should add some memory reporters to break that down so we can see in more detail where memory is being used and where we can trim stuff.

In particular I'd be interested in seeing how much memory is spent on the new progressive/low-res tiling textures because that is something that should be easily reduceable on low-memory devices.

Comment 1

[Benoit Girard (:BenWa)]

Most things in our side on mobile will show up by counting gfx[Shared]ImageSurface. There's still the GL memory that considerable that I'm not sure how to count.

Comment 2

[Nicholas Nethercote [inactive]]

We definitely have some coverage.  E.g. see all the functions with |SizeOf| in their title in image/src/RasterImage.cpp and image/src/imgFrame.cpp.  So this bug needs to get more specific about what's missing in order to make progress.

BTW, memory reporters for graphics stuff are often a pain, because often graphics stuff doesn't use heap memory.  Reporters for heap memory can be written reliably -- using moz_malloc_usable_size to measure each allocated block -- and DMD is capable of verifying their correctness.  Reporters for non-heap memory, in contrast, must rely on analytically computing sizes of allocated blocks, which is error-prone, and DMD cannot check them.

Comment 3

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

Jeff said he would do this. We may not be able to get exact byte counts for how much memory the textures use up but we should be able to estimate. Also updating title to be more specific about the memory I care about for this bug.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Attachment: A sketch

Something like this.

Comment 5

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

Attachment: A working sketch

Here is an updated version of jeff's patch that compiles and works.

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 690743 [details] [diff] [review]
A working sketch

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

::: gfx/layers/opengl/TiledThebesLayerOGL.cpp
@@ +38,5 @@
>    if (aTile == GetPlaceholderTile())
>      return;
>    mContext->fDeleteTextures(1, &aTile.mTextureHandle);
> +  GLenum format = aTile.mFormat;
> +  sTiledThebesLayerTextureUsage -= GetTileLength()*GetTileLength()*(mozilla::gl::GetBitsPerTexel(format, format == LOCAL_GL_RGB ? LOCAL_GL_UNSIGNED_SHORT_5_6_5 : LOCAL_GL_UNSIGNED_BYTE)/8);

Yowza!  What an ugly statement.  Please break up the expression.  Spaces around '*' and '/' are also appreciated.

@@ +85,5 @@
> +    return sTiledThebesLayerTextureUsage;
> +}
> +
> +NS_MEMORY_REPORTER_IMPLEMENT(
> +                             TILEDTHEBESLAYER,

Why the shouting?  |TiledThebesLayer| is fine.

Also, why put a newline after the '(' and then indent the same amount?  Most of the other reporters do this:

NS_MEMORY_REPORT_IMPLEMENT(TiledThebesLayer,
  "gfx-tiled-thebes-layer-textures",
   ...

@@ +90,5 @@
> +                             "gfx-tiled-thebes-layer-textures",
> +                             KIND_OTHER,
> +                             UNITS_BYTES,
> +                             GetTiledThebesLayerTextureUsage,
> +                             "Texture memory used by TiledThebesLayer");

Is it worth mentioning pvrsrvkm here?

@@ +121,5 @@
>    mContext->fTexImage2D(LOCAL_GL_TEXTURE_2D, 0, format,
>                         GetTileLength(), GetTileLength(), 0,
>                         format, type, buf);
>  
> +  sTiledThebesLayerTextureUsage += GetTileLength()*GetTileLength()*(mozilla::gl::GetBitsPerTexel(format, type)/8);

Spaces around operators, plz.  Also, this expression isn't quite the same as the "yowza" one above -- that's surprising (speaking as someone who knows nothing about this code in general...)

@@ +145,5 @@
>    mImplData = static_cast<LayerOGL*>(this);
> +  if (sTiledThebesLayerMemoryReporter == nullptr) {
> +    sTiledThebesLayerMemoryReporter = new NS_MEMORY_REPORTER_NAME(TILEDTHEBESLAYER);
> +    NS_RegisterMemoryReporter(sTiledThebesLayerMemoryReporter);
> +  }

This reporter is never unregistered.  Is that appropriate?  I don't know what the lifetime of this class is.  It's reasonable if there's only a single instance of this class and it lives for the life of the browser.

If it is appropriate, you don't need sTiledThebesLayerMemoryReporter, you can just do this:

  NS_RegisterMemoryReporter(new NS_MEMORY_REPORTER_NAME(TiledThebesLayer));

Comment 7

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

Thanks for the feedback! I'll clean up the patch and make it more review-worthy.

(In reply to Nicholas Nethercote [:njn] from comment #6)
> Is it worth mentioning pvrsrvkm here?

I'm not sure what all that includes.

> >    mImplData = static_cast<LayerOGL*>(this);
> > +  if (sTiledThebesLayerMemoryReporter == nullptr) {
> > +    sTiledThebesLayerMemoryReporter = new NS_MEMORY_REPORTER_NAME(TILEDTHEBESLAYER);
> > +    NS_RegisterMemoryReporter(sTiledThebesLayerMemoryReporter);
> > +  }
> 
> This reporter is never unregistered.  Is that appropriate?  I don't know
> what the lifetime of this class is.  It's reasonable if there's only a
> single instance of this class and it lives for the life of the browser.
> 
> If it is appropriate, you don't need sTiledThebesLayerMemoryReporter, you
> can just do this:
> 
>   NS_RegisterMemoryReporter(new NS_MEMORY_REPORTER_NAME(TiledThebesLayer));

So I believe multiple instances of TiledThebesLayerOGL can be created, so if I just instantiate a new one blindly in the constructor we'll have too many instances. Jeff, is there a better place to instantiate the memory reporter?

Comment 8

[Justin Lebar (not reading bugmail)]

> So I believe multiple instances of TiledThebesLayerOGL can be created, so if I just 
> instantiate a new one blindly in the constructor we'll have too many instances.

So long as you don't have /tons/ of TiledThebesLayerOGL instances, it's OK to register a memory reporter in the constructor and unregister it in the destructor.  That is, it's OK to have multiple overlapping reporters.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
>
> So I believe multiple instances of TiledThebesLayerOGL can be created, so if
> I just instantiate a new one blindly in the constructor we'll have too many
> instances. Jeff, is there a better place to instantiate the memory reporter?

There will be multiple instances of TiledThebesLayerOGL. Usually on the order of 2-3, however there isn't really a bound. gfx::Platform is probably a decent place to initiate the memory reporter.

Comment 10

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

Attachment: Add tile textures memory reporter

Cleaned up.

Comment 11

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

Comment on attachment 690963 [details] [diff] [review]
Add tile textures memory reporter

Sorry for the churn. I found a couple of problems with this patch; trying to sort them out first.

Comment 12

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

Attachment: Add tile textures memory reporter

Ok, fixed it up. Some of the type conversion wasn't working right. Also the description needs to be a proper sentence for it to not crap out about:memory :)

Comment 13

[Nicholas Nethercote [inactive]]

Comment on attachment 691124 [details] [diff] [review]
Add tile textures memory reporter

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

::: gfx/gl/GLContextUtils.cpp
@@ +471,5 @@
> +    {
> +        return 16;
> +    }
> +
> +    NS_ABORT();

MOZ_NEVER_REACHED() might be better?

::: gfx/thebes/gfxPlatform.cpp
@@ +222,5 @@
> +    return sTiledThebesLayerTextureUsage;
> +}
> +
> +void
> +gfxPlatform::RecordTiledThebesLayerTextureUsage(int64_t bytes)

Given that the arg is a delta "Update" might be a better name than "Record".

Comment 14

[Justin Lebar (not reading bugmail)]

> MOZ_NEVER_REACHED() might be better?

Do you mean MOZ_NOT_REACHED()?

MOZ_NOT_REACHED() is not an assertion; reaching it triggers undefined behavior.  It's a marker to enable compiler optimizations.

Comment 15

[Nicholas Nethercote [inactive]]

Ok then, maybe MOZ_CRASH()?  Or MOZ_ASSERT(false) if you want it to abort only in debug builds.

Comment 16

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

Attachment: Add tile textures memory reporter (v2)

(In reply to Nicholas Nethercote [:njn] from comment #13)
> 
> MOZ_NEVER_REACHED() might be better?
> 

Updated to use MOZ_ASSERT(false) based on the last two comments.

> 
> Given that the arg is a delta "Update" might be a better name than "Record".

Updated.

Further testing/use of the patch showed that there were still problems with it. BenWa, jrmuizel and I discussed it more and came up with a few changes to make it more correct and cleaner. New version attached, re-requesting review.

Comment 17

[Benoit Girard (:BenWa)]

Comment on attachment 691534 [details] [diff] [review]
Add tile textures memory reporter (v2)

This will likely under count the memory used by texture but it's probably a good start. I'm fine as long as you confirm that the counting problem is fixed.

Comment 18

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

(In reply to Benoit Girard (:BenWa) from comment #17)
> I'm fine as long as you confirm that the counting problem is
> fixed.

Yeah it's good now; the numbers seem to be stable.

https://hg.mozilla.org/integration/mozilla-inbound/rev/09d66d9ccb2e

Comment 19

[Ed Morley [:emorley]]

Backed out for for build failures on Windows:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=09d66d9ccb2e

https://hg.mozilla.org/integration/mozilla-inbound/rev/2aae75e86935

Comment 20

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

BenWa was correct in blaming this build failure on the inclusion of GLContext.h into gfxPlatform.h. I confirmed with the try run at https://tbpl.mozilla.org/?tree=Try&rev=da8b5bc068a6

I'm not really sure what the include is screwing but BenWa and I discussed it and I will move the code around so that it hopefully doesn't hit this any more. Patch coming in a bit.

Comment 21

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

Attachment: Add texture memory reporter (v3)

Moved stuff around. Also renamed some things. Otherwise it's functionally identical to the previous version. Also this one doesn't break builds as evidenced by https://tbpl.mozilla.org/?tree=Try&rev=97ae53b2a788

Comment 22

[Benoit Girard (:BenWa)]

Comment on attachment 692091 [details] [diff] [review]
Add texture memory reporter (v3)

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

Much better :)

Comment 23

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

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

Comment 24

[Ed Morley [:emorley]]

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

Comment 25

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

Filed bug 833185 to object to this patch.