1139252 - Fix D3D9 memory reporters

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicholas Nethercote [inactive]]

Bug 1128765 added three memory reporters. D3D9SurfaceImageReporter is ok, but D3D9TextureReporter and D3D9SharedTextureReporter are not:

> class D3D9TextureReporter MOZ_FINAL : public nsIMemoryReporter
> {
>   ...
>   NS_IMETHOD CollectReports(nsIHandleReportCallback *aHandleReport,
>                             nsISupports* aData, bool aAnonymize) MOZ_OVERRIDE
>   {
>     return MOZ_COLLECT_REPORT("d3d9-shared-textures", KIND_OTHER, UNITS_BYTES,
>                               gfxWindowsPlatform::sD3D9MemoryUsed,
>                               "Memory used for D3D9 shared textures");
>   }
> };
> 
> class D3D9SharedTextureReporter MOZ_FINAL : public nsIMemoryReporter
> {
>   ...
>   NS_IMETHOD CollectReports(nsIHandleReportCallback *aHandleReport,
>                             nsISupports* aData, bool aAnonymize) MOZ_OVERRIDE
>   {
>     return MOZ_COLLECT_REPORT("d3d9-shared-texture", KIND_OTHER, UNITS_BYTES,
>                               gfxWindowsPlatform::sD3D9SharedTextureUsed,
>                               "Memory used for D3D9 shared textures");
>   }
> };

The following things should be consistent within each reporters:
- the reporter class name;
- the report path;
- the static variable name;
- the description (which should also end with a '.').

It's enough of a copy+paste mess that I can't work out which parts need to be changed to fix it.

Jeff, can you fix this? Thank you.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: Fix reporters

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 8573452 [details] [diff] [review]
Fix reporters

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

Looking much better! I'd to check it one more time before landing, thanks.

::: gfx/layers/D3D9SurfaceImage.cpp
@@ +34,5 @@
>   * on destruction */
>  class TextureMemoryMeasurer9 : public IUnknown
>  {
>  public:
>    TextureMemoryMeasurer9(size_t aMemoryUsed)

Nit: it's pre-existing, but the '9' suffix here surprised me. Would 'D3D9' be better?

::: gfx/layers/d3d11/TextureD3D11.cpp
@@ +372,5 @@
>   * on destruction */
>  class TextureMemoryMeasurer : public IUnknown
>  {
>  public:
>    TextureMemoryMeasurer(size_t aMemoryUsed)

Similarly, would 'TextureMemoryMeasurerD3D11' be a better name?

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +332,2 @@
>  
> +class D3DTextureReporter MOZ_FINAL : public nsIMemoryReporter

Putting them all in a single reporter class is a good idea.

@@ +342,2 @@
>  
> +    rv =  MOZ_COLLECT_REPORT("d3d-shared-textures/d3d11", KIND_OTHER, UNITS_BYTES,

You now have the following paths:

- "d3d-shared-textures/d3d11"
- "d3d-shared-textures/d3d9"
- "d3d9-shared-texture-client"
- "d3d9-surface-image"

The first two will be grouped together in a subtree, though I suspect one of them will always be zero because it's unlikely that D3D9 and D3D11 are running at the same time? (I'm not sure about that.) But the latter two will be separate.

Would it instead make sense to do something like this:

- "d3d11/shared-textures"
- "d3d9/shared-textures"
- "d3d9/shared-texture-client"
- "d3d9/surface-image"

So that all the D3D9 measurements end up in the same subtree? If so, please change the variable names to match the paths, e.g. sD3D11SharedTextures.

Also, you might want to skip reporting values that are zero. You could create a local REPORT macro to encapsulate that. See the one in xpcom/base/nsCycleCollector.cpp for an example, and don't forget to #undef it afterwards.

Comment 3

[u279076]

Jeff, looks like this stalled 14 months ago. Any updates?

Comment 4

[Jeff Muizelaar [:jrmuizel]]

The code has changed enough that my patch is no longer useful. Nick, are there still changes that you want made in the current code?

Comment 5

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

If there are, Mason can you pick it up instead?

Comment 6

[Nicholas Nethercote [inactive]]

Sometimes it's just easier to do something yourself.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: Fix D3D texture memory reporters

This patch makes the names and reporter paths more consistent. It also removes
sD3D9MemoryUsed, which was unused.

Comment 8

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9f15ab5c6c5dce095d6d5622a554bf14cb6db1b
Bug 1139252 - Fix D3D texture memory reporters. r=jrmuizel.

Comment 9

[Carsten Book [:Tomcat]]

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