Last Comment Bug 818060 - Add a memory reporter for graphics textures
: Add a memory reporter for graphics textures
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Toolkit
Classification: Components
Component: about:memory (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla20
Assigned To: away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com)
:
: Nicholas Nethercote [:njn]
Mentors:
Depends on: 833185
Blocks: 792138 818546
  Show dependency treegraph
 
Reported: 2012-12-04 08:02 PST by away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com)
Modified: 2013-01-21 19:30 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
A sketch (5.72 KB, patch)
2012-12-10 12:35 PST, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
A working sketch (7.01 KB, patch)
2012-12-10 22:45 PST, away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Splinter Review
Add tile textures memory reporter (7.76 KB, patch)
2012-12-11 10:57 PST, away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Splinter Review
Add tile textures memory reporter (7.79 KB, patch)
2012-12-11 16:10 PST, away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com)
n.nethercote: review+
Details | Diff | Splinter Review
Add tile textures memory reporter (v2) (12.04 KB, patch)
2012-12-12 14:37 PST, away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com)
b56girard: review+
n.nethercote: review+
Details | Diff | Splinter Review
Add texture memory reporter (v3) (10.84 KB, patch)
2012-12-13 17:08 PST, away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com)
b56girard: review+
Details | Diff | Splinter Review

Description away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-12-04 08:02:36 PST
: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) 2012-12-04 08:09:49 PST
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 [:njn] 2012-12-04 13:39:43 PST
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 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-12-10 11:55:03 PST
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] 2012-12-10 12:35:06 PST
Created attachment 690514 [details] [diff] [review]
A sketch

Something like this.
Comment 5 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-12-10 22:45:22 PST
Created attachment 690743 [details] [diff] [review]
A working sketch

Here is an updated version of jeff's patch that compiles and works.
Comment 6 Nicholas Nethercote [:njn] 2012-12-11 01:49:51 PST
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 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-12-11 08:18:54 PST
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) 2012-12-11 08:22:45 PST
> 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] 2012-12-11 08:54:27 PST
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-12-11 10:57:03 PST
Created attachment 690963 [details] [diff] [review]
Add tile textures memory reporter

Cleaned up.
Comment 11 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-12-11 13:42:52 PST
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 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-12-11 16:10:21 PST
Created attachment 691124 [details] [diff] [review]
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 [:njn] 2012-12-11 20:52:35 PST
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) 2012-12-11 21:56:13 PST
> 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 [:njn] 2012-12-12 00:42:37 PST
Ok then, maybe MOZ_CRASH()?  Or MOZ_ASSERT(false) if you want it to abort only in debug builds.
Comment 16 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-12-12 14:37:39 PST
Created attachment 691534 [details] [diff] [review]
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) 2012-12-12 14:43:17 PST
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 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-12-13 08:39:09 PST
(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] 2012-12-13 09:18:43 PST
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 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-12-13 11:38:00 PST
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 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-12-13 17:08:12 PST
Created attachment 692091 [details] [diff] [review]
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) 2012-12-16 14:02:19 PST
Comment on attachment 692091 [details] [diff] [review]
Add texture memory reporter (v3)

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

Much better :)
Comment 23 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-12-16 16:18:10 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9d3089866cf
Comment 24 Ed Morley [:emorley] 2012-12-17 05:56:51 PST
https://hg.mozilla.org/mozilla-central/rev/c9d3089866cf
Comment 25 Benoit Jacob [:bjacob] (mostly away) 2013-01-21 19:30:54 PST
Filed bug 833185 to object to this patch.

Note You need to log in before you can comment on or make changes to this bug.