710594 - Adapt WebGL about:memory reporters to the new memory reporting API

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

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

about:memory is a special page where various components report on their memory usage. WebGL is doing such reporting for WebGL objects (like WebGLTextures), the current implementation is there:

http://hg.mozilla.org/mozilla-central/file/efab5016434f/content/canvas/src/WebGLContext.h#l2435
http://hg.mozilla.org/mozilla-central/file/efab5016434f/content/canvas/src/WebGLContext.cpp#l78

Meanwhile, a new, better memory reporting API has been added:

https://wiki.mozilla.org/Platform/Memory_Reporting

So we need someone to 1) read that wiki page 2) redo the WebGL memory reporting implementation to use that new API.

As usual, MXR is your friend to navigate through the code:
http://mxr.mozilla.org/mozilla-central/

Comment 1

[Nicholas Nethercote [inactive]]

> Meanwhile, a new, better memory reporting API has been added:
> 
> https://wiki.mozilla.org/Platform/Memory_Reporting

It's not so much an API as a set of conventions.  But this change is still worth doing! :)

Comment 2

[Andrew Quartey [:drexler]]

This looks interesting. I will take a shot at this.

Comment 3

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

Great, don't hesitate to ask questions. I'll be off next week. You can assign this bug to yourself if you want to indicate that it's no longer free to take.

Comment 4

[Andrew Quartey [:drexler]]

Attachment: Patch v1

Set up WebGLMemoryReporter as a MemoryMultiReporter

Comment 5

[Justin Lebar (not reading bugmail)]

What are we trying to achieve by switching to MemoryMultiReporter here?

In general, we use multi-reporters only when the number of reporters is not known ahead of time.  For example, if you were reporting the memory usage of each WebGL context, you'd need to use a multi-reporter.  But that doesn't seem to be the case here...

Comment 6

[Justin Lebar (not reading bugmail)]

> For example, if you were reporting the memory usage of each WebGL context

I should say, "of each WebGL context *separately*..."

Comment 7

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

Switching to MemoryMultiReporter allows this patch to reduce the source code size by a few dozen lines, so I was leaning in favor of it and was about to r+ this patch. Is there a downside to MemoryMultiReporter?

The main reason why I filed this bug though, is for what the wiki page says about instrumentation for DMD. Some of the WebGL memory reporters are about buffers on the heap. Would it be valuable to do what the Wiki page recommends wrt DMD?

Comment 8

[Justin Lebar (not reading bugmail)]

> Is there a downside to MemoryMultiReporter?

The main reason we avoid them is because there's no way to tell which MemoryMultiReporter will give you the report you want.  This is a pain, for example, if you want to report a value in telemetry, because now you have to invoke every memory reporter in order to find the report you're looking for.  (In fact, I don't think telemetry supports multi-reporters atm.)

> Would it be valuable to do what the Wiki page recommends wrt DMD?

Yes.

Comment 9

[Nicholas Nethercote [inactive]]

(In reply to Justin Lebar [:jlebar] from comment #8)
> > Is there a downside to MemoryMultiReporter?
> 
> The main reason we avoid them is because there's no way to tell which
> MemoryMultiReporter will give you the report you want.

Bug 689583 will fix this, if I ever get around to it.  There hasn't really been a need yet.

> This is a pain, for
> example, if you want to report a value in telemetry, because now you have to
> invoke every memory reporter in order to find the report you're looking for.
> (In fact, I don't think telemetry supports multi-reporters atm.)

Yeah, multi-reporters are all or nothing, and so don't go well with telemetry.  If you don't think you'll ever need to report WebGL numbers to telemetry and a multi-reporter makes the code simpler, might as well just to do a multi-reporter.  (Sometimes a multi-reporter lets you do a single pass over your structures instead of multiple.)

Comment 10

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

Based on this, I think we want the present MultiReporter patch. We already report WebGL usage to telemetry, and I don't think that it's useful to also report the WebGL memoryreporter values to Telemetry.

Andrew, do you plan to do the other part, as explained on the wiki, about DMD?

Comment 11

[Nicholas Nethercote [inactive]]

Comment on attachment 583562 [details] [diff] [review]
Patch v1

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

This is a good start.  I agree that a multi-reporter seems fine here, you have enough numbers being reported that it's worthwhile.

As Benoit says, the KIND_HEAP numbers should use the style described in the wiki.  You'll need something like this:

  NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN(WebGLMallocSizeOf, "webgl")

And then pass WebGLMallocSizeOf to GetBufferCacheMemoryUsed() and similar ones, and probably rename them too.

::: content/canvas/src/WebGLContext.h
@@ +2454,5 @@
>  }
>  
> +
> +
> +class WebGLMemoryReporter : nsIMemoryMultiReporter

I'd rename it "WebGLMemoryMultiReporter" or "WebGLMultiReporter";  other multi-reporters are named like that to distinguish them from non-multi-reporters.

Comment 12

[Andrew Quartey [:drexler]]

Benoit, i will take care of that. Should any issue come up, i will ping you on irc.

Comment 13

[Justin Lebar (not reading bugmail)]

Some minor stuff:

>+NS_IMETHODIMP
>+WebGLMemoryReporter::CollectReports(nsIMemoryMultiReporterCallback* aCb,
>+                                         nsISupports* aClosure)
>+{
>+   nsCAutoString textureMemoryStr("webgl-texture-memory");
>+   NS_NAMED_LITERAL_CSTRING(kTextureMemoryDesc, "Memory used by WebGL textures. The OpenGL implementation is free to store these textures in either video memory or main memory. This measurement is only a lower bound, actual memory usage may be higher for example if the storage is strided.");

Please wrap these string literals to 80 chars long.  (We usually do C-style concatenation, as in

  "foo ..."
  "bar."
  
)

>+   PRInt64 textureMemoryUsed = GetTextureMemoryUsed();
>+   aCb->Callback(EmptyCString(), textureMemoryStr,
>+                 nsIMemoryReporter::KIND_OTHER, nsIMemoryReporter::UNITS_BYTES,
>+                 textureMemoryUsed, kTextureMemoryDesc, aClosure);

We usually inline the strings (textureMemoryStr and kTextureMemoryDesc) into the call, with NS_LITERAL_CSTRING, unless they're used in more than one place.  I'd probably inline the call to GetTextureMemoryUsed(), too.

>@@ -2582,11 +2572,13 @@ class WebGLMemoryReporter
>         return result;
>     }
> 
>     static PRInt64 GetContextCount() {
>         return Contexts().Length();
>     }
> };
> 
>+
>+
> }
> 
> #endif
>

Accidental whitespace change?

Comment 14

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

Comment on attachment 583562 [details] [diff] [review]
Patch v1

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

What others said, plus the following. Waiting for new version; hoping it will include the SizeOf stuff explained by Nick.

::: content/canvas/src/WebGLContext.h
@@ -2459,5 @@
>      ~WebGLMemoryReporter();
>      static WebGLMemoryReporter* sUniqueInstance;
>  
> -    // here we store plain pointers, not RefPtrs: we don't want the WebGLMemoryReporter unique instance to keep alive all
> -    // WebGLContexts ever created.

Hey, I wouldn't remove this comment! It's referring to the mContexts array, which is unaffected by your patch.

Comment 15

[Andrew Quartey [:drexler]]

Attachment: Patch v2

Comment 16

[Nicholas Nethercote [inactive]]

Comment on attachment 584011 [details] [diff] [review]
Patch v2

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

This is close, but I think you're a little confused still about the stuff in https://wiki.mozilla.org/Platform/Memory_Reporting.

The key idea is we prefer to measure the size of each block by asking the system how big it is.  But some platforms don't support that facility, so we have a fall-back where we compute the size ourselves.  The functions created with NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN do encapsulate this process -- that's why you pass them a pointer to the block and a |computedSize| argument.

What you conceptually have is this:

  result += foo->ComputedSize();
  result += foo->SizeOfIncludingThis(MallocSizeOfFun);

So on platforms that let you find out how big a block is by asking, we'll measure the memory twice, once by computing the size ourselves, and once by getting the size from the system.  On systems that don't have that facility, we'll also measure the memory twice, once by the computing the size ourselves (in foo->ComputedSize()), and again when we fall-back to the computedSize within foo->SizeOfIncludingThis().

This code should instead be:

  result += foo->SizeOfIncludingThis(MallocSizeOfFun);

where the system request and the fall-back are both encapsulated within the SizeOfIncludingThis.

Does that make sense?  The wiki page obviously could be made clearer, I added a bit more text under the definition of the |nsMallocSizeOfFun| typedef in an attempt to improve it, but please let me know if there's more I can do.

::: content/canvas/src/WebGLContext.cpp
@@ +88,5 @@
> +                 GetTextureMemoryUsed(), 
> +                 NS_LITERAL_CSTRING("Memory used by WebGL textures. The OpenGL implementation is free to store " 
> +                                    "these textures in either video memory or main memory. This measurement is " 
> +                                    "only a lower bound, actual memory usage may be higher for example if the "
> +                                    "storage is strided."), aClosure);

Please reformat this and other strings to be 80 chars or less wide.  I realize it's a pain, It might be worth doing something like this:

  NS_NAMED_LITERAL_CSTRING(s, "Memory used by WebGL textures..."...);
  aCb->Callback(..., s, ...);

So you have a bigger part of each 80-char line to fit text into.  Or even this:

  NS_NAMED_LITERAL_CSTRING(s,
    "Memory used by..."
    ...);

::: content/canvas/src/WebGLContext.h
@@ +986,5 @@
>          return size * componentSize();
>      }
>  };
>  
> +NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN(WebGLBufferMallocSizeOfFun, "webgl_buffer_malloc")

Please use the string "webgl-buffer", that'll be consistent with the naming convention used for other such functions.

@@ +1575,5 @@
>          return mFakeBlackStatus == DoNeedFakeBlack;
>      }
>  };
>  
> +NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN(WebGLShaderMallocSizeOfFun, "webgl_shader_malloc")

And please call this "webgl-shader".

@@ +2541,2 @@
>                  if (contexts[i]->mBuffers[b]->Target() == LOCAL_GL_ELEMENT_ARRAY_BUFFER)
>                      result += contexts[i]->mBuffers[b]->ByteLength();

I think you're making the double-measurement mistake I mentioned above here.  The LOCAL_GL_ELEMENT_ARRAY_BUFFER test is confusing the matter, though, so I'm not 100% sure.

@@ +2575,5 @@
>          for(size_t i = 0; i < contexts.Length(); ++i)
> +            for (size_t s = 0; s < contexts[i]->mShaders.Length(); ++s) {
> +                result += contexts[i]->mShaders[s]->Source().Length() +
> +                          contexts[i]->mShaders[s]->SizeOfIncludingThis(WebGLShaderMallocSizeOfFun) - 
> +                          contexts[i]->mShaders[s]->SizeOfExcludingThis(WebGLShaderMallocSizeOfFun);

This isn't right.  First, subtracting a SizeOf{Ex,In}cludingThis result will screw things up for DMD.  Second, |foo->SizeOfIncludingThis(...) - foo->SizeOfExcludingThis(...)| is just equivalent to |sizeof(*foo)|.  And there's still a computed size in the Length().

@@ +2586,5 @@
>          PRInt64 result = 0;
>          for(size_t i = 0; i < contexts.Length(); ++i)
> +            for (size_t s = 0; s < contexts[i]->mShaders.Length(); ++s) {
> +                result += contexts[i]->mShaders[s]->TranslationLog().Length() +
> +                          contexts[i]->mShaders[s]->SizeOfExcludingThis(WebGLShaderMallocSizeOfFun);

Again, you've made the double-measurement mistake.

Comment 17

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

Comment on attachment 584011 [details] [diff] [review]
Patch v2

(waiting for next version incorporating Nick's comments)

Comment 18

[Andrew Quartey [:drexler]]

Totally got the wrong idea regarding the measurements. Okay, a newer patch incorporating Nicks comments should be ready by tomorrow at the latest.

Comment 19

[Andrew Quartey [:drexler]]

Attachment: Patch v3

Reformatted strings to be 80 chars or less and adjustments to the memory size calculations. 

Since GetShaderSourcesSize() and GetShaderTranslationLogsSize() measure different aspects of a WebGLShader, i splitted the SizeOfs computations accordingly to deal with that.

Comment 20

[Nicholas Nethercote [inactive]]

Comment on attachment 586333 [details] [diff] [review]
Patch v3

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

This is a clear improvement, but there are still a couple of problems.  But don't be discouraged! :)

::: content/canvas/src/WebGLContext.cpp
@@ +86,5 @@
> +   aCb->Callback(
> +     EmptyCString(), 
> +     NS_LITERAL_CSTRING("webgl-texture-memory"),
> +     nsIMemoryReporter::KIND_OTHER, nsIMemoryReporter::UNITS_BYTES,
> +     GetTextureMemoryUsed(),

This is a 3-space indent followed by a 2-space indent.  Please use 4-space indents as the rest of this file does.

::: content/canvas/src/WebGLContext.h
@@ +1599,5 @@
>      }
> +    
> +    size_t SizeOfTransLogExcludingThis(nsMallocSizeOfFun aMallocSizeOf) const {
> +       int len = mTranslationLog.Length();
> +       return aMallocSizeOf(&mTranslationLog, len * sizeof(char));

Does this crash?  |&mTranslationLog| is in the middle of a WebGLShader object, and aMallocSizeOf() should only be passed pointers to the start of heap objects.

What you really want is this:

  return mTranslationLog.SizeOfExcludingThis(aMallocSizeOf);

The |mTranslationLog| object itself is measured by the |aMallocSizeOf(this, sizeof(WebGLShader))| call in SizeOfTransLogIncludingThis().  That's why you want an "excluding this" measurement.

Unfortunately, |mTranslationLog| is an nsCString, which currently lacks a SizeOfExcludingThis() method.  Bug 710054 is open for adding it.  The implemention is non-trivial because you have to worry about multiple nsCStrings sharing the char buffer.

@@ +1604,5 @@
> +    }
> +
> +    size_t SizeOfTransLogIncludingThis(nsMallocSizeOfFun aMallocSizeOf) const {
> +       return aMallocSizeOf(this, sizeof(WebGLShader)) + SizeOfTransLogExcludingThis(aMallocSizeOf);
> +    }

When you call x->SizeOfTransLogIncludingThis() followed by x->SizeOfShaderSourceIncludingThis(), you do the "including this" measurement on x twice.

There are several options to fix this:

- Make one of the measurements "including this" and one "excluding this".

- Make them both "excluding this", and take a 3rd measurement that is just "including this".  But that's probably not a good idea.

- Just combine them into a single measurement.  It says above that "shader-sources-size" is usually only a few KB, so separating it might not be worthwhile.  I'd be inclined to do this -- "shader-sources-size" and "shader-translationlogs-size" would be combined into "shader".  (The "-size" suffix isn't necessary.)

@@ +1608,5 @@
> +    }
> +    
> +    size_t SizeOfShaderSourceExcludingThis(nsMallocSizeOfFun aMallocSizeOf) const {
> +       int len = mSource.Length(); 
> +       return aMallocSizeOf(&mSource, len * sizeof(char));    

Same thing here:  |&mSource| is in the middle of an object.  You want:

  return mSource.SizeOfExcludingThis(aMallocSizeOf);

Again, this depends on Bug 710054.

Comment 21

[Andrew Quartey [:drexler]]

(In reply to Nicholas Nethercote [:njn] from comment #20)

> This is a clear improvement, but there are still a couple of problems.  But
> don't be discouraged! :)

Not at all!! Fun stuff.

> Does this crash?  |&mTranslationLog| is in the middle of a WebGLShader
> object, and aMallocSizeOf() should only be passed pointers to the start of
> heap objects.

It shouldn't in my view(yet to fully test) but currently working on resolving the following linker issues with an updated patch addressing your earlier suggestions:

WebGLContextGL.obj : error LNK2005: "unsigned int __cdecl mozilla::WebGLBufferMallocSizeOfFun(void const *,unsigned int)" (?WebGLBufferMallocSizeOfFun@mozilla@@YAIPBXI@Z) already defined in WebGLContext.obj
WebGLContextGL.obj : error LNK2005: "unsigned int __cdecl mozilla::WebGLShaderMallocSizeOfFun(void const *,unsigned int)" (?WebGLShaderMallocSizeOfFun@mozilla@@YAIPBXI@Z) already defined in WebGLContext.obj
WebGLContextUtils.obj : error LNK2005: "unsigned int __cdecl mozilla::WebGLBufferMallocSizeOfFun(void const *,unsigned int)" (?WebGLBufferMallocSizeOfFun@mozilla@@YAIPBXI@Z) already defined in WebGLContext.obj
WebGLContextUtils.obj : error LNK2005: "unsigned int __cdecl mozilla::WebGLShaderMallocSizeOfFun(void const *,unsigned int)" (?WebGLShaderMallocSizeOfFun@mozilla@@YAIPBXI@Z) already defined in WebGLContext.obj
WebGLContextValidate.obj : error LNK2005: "unsigned int __cdecl mozilla::WebGLBufferMallocSizeOfFun(void const *,unsigned int)" (?WebGLBufferMallocSizeOfFun@mozilla@@YAIPBXI@Z) already defined in WebGLContext.obj
WebGLContextValidate.obj : error LNK2005: "unsigned int __cdecl mozilla::WebGLShaderMallocSizeOfFun(void const *,unsigned int)" (?WebGLShaderMallocSizeOfFun@mozilla@@YAIPBXI@Z) already defined in WebGLContext.obj
WebGLExtensionStandardDerivatives.obj : error LNK2005: "unsigned int __cdecl mozilla::WebGLBufferMallocSizeOfFun(void const *,unsigned int)" (?WebGLBufferMallocSizeOfFun@mozilla@@YAIPBXI@Z) already defined in WebGLContext.obj
WebGLExtensionStandardDerivatives.obj : error LNK2005: "unsigned int __cdecl mozilla::WebGLShaderMallocSizeOfFun(void const *,unsigned int)" (?WebGLShaderMallocSizeOfFun@mozilla@@YAIPBXI@Z) already defined in WebGLContext.obj
WebGLExtensionLoseContext.obj : error LNK2005: "unsigned int __cdecl mozilla::WebGLBufferMallocSizeOfFun(void const *,unsigned int)" (?WebGLBufferMallocSizeOfFun@mozilla@@YAIPBXI@Z) already defined in WebGLContext.obj
WebGLExtensionLoseContext.obj : error LNK2005: "unsigned int __cdecl mozilla::WebGLShaderMallocSizeOfFun(void const *,unsigned int)" (?WebGLShaderMallocSizeOfFun@mozilla@@YAIPBXI@Z) already defined in WebGLContext.obj
   Creating library xul.lib and object xul.exp

Comment 22

[Nicholas Nethercote [inactive]]

> WebGLContextGL.obj : error LNK2005: "unsigned int __cdecl
> mozilla::WebGLBufferMallocSizeOfFun(void const *,unsigned int)"
> (?WebGLBufferMallocSizeOfFun@mozilla@@YAIPBXI@Z) already defined in
> WebGLContext.obj

You can't use NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN in a .h file, because the header will be #included multiple times and you'll end up with multiple definitions of the function.  Just move it into the .cpp file, and you'll have to move definitions of the members that use WebGLShaderMallocSizeOfFun() into the .cpp file as well.

Comment 23

[Andrew Quartey [:drexler]]

Realized that earlier and moved those definitions to the .cpp files. Currently rebuilding it -an hour plus affair on my dinosaur :-(

Comment 24

[Nicholas Nethercote [inactive]]

If you've only modified one module, you can short-cut the build.  An example:

  make -C $OBJDIR/content/            # rebuild the modified module
  make -C $OBJDIR/toolkit/library     # redo the final linking

That can save a lot of time.  It can also waste time if you get it wrong and you end up with a build that doesn't have the changes you think it has!  So be careful :)

(BTW, it's ridiculous that you can do this, a decent build system should do exactly that kind of thing for you.)

Comment 25

[Andrew Quartey [:drexler]]

Thanks! That should significantly cut down on my build time. I have been doing that
make -C $objdir/{modified_dir}  and 
make -f client.mk at the TOP_SRC_DIR. 

before sending to try for additional tests.

Comment 26

[Nicholas Nethercote [inactive]]

BTW, that short-cut works for most modules, but not all, just to complicate things.  It does work for content/, which is the important one for this bug.

Comment 27

[Andrew Quartey [:drexler]]

Fixed the linking issues but fails on try-https://tbpl.mozilla.org/?tree=Try&rev=b46a9c2c2077. Specifically, the invocation of ~WebGLMemoryMultiReporter leads to test failures.

Comment 28

[Nicholas Nethercote [inactive]]

Here's one of the crashes from the logs:

mozilla::Mutex::Lock [obj-firefox/xpcom/build/BlockingResourceBase.cpp:261]
nsMemoryReporterManager::UnregisterMultiReporter [nsCOMArray.h:264]
NS_UnregisterMemoryMultiReporter [xpcom/base/nsMemoryReporterManager.cpp:910]
mozilla::WebGLMemoryMultiReporter::Release [nsTArray.h:946]
nsCOMArray_base::RemoveObject [obj-firefox/xpcom/build/nsCOMArray.cpp:125]
nsMemoryReporterManager::UnregisterMultiReporter [xpcom/base/nsMemoryReporterManager.cpp:651]
NS_UnregisterMemoryMultiReporter [xpcom/base/nsMemoryReporterManager.cpp:910]
mozilla::WebGLContext::~WebGLContext [content/canvas/src/WebGLContext.cpp:235]
mozilla::WebGLContext::Release [content/canvas/src/WebGLContext.cpp:1286]
nsXPCOMCycleCollectionParticipant::Unroot [obj-firefox/xpcom/build/nsCycleCollectionParticipant.cpp:78]

There's a reference counting problem.  ~WebGLContext calls RemoveWebGLContext which does |delete sUniqueInstance|, triggering ~WebGLMemoryMultiReporter.  But that calls NS_UnregisterMemoryMultiReporter, which reduces the refcount for sUniqueInstance to zero, which triggers ~WebGLMemoryMultiReporter *again*, which leads to a locking crash. 

The problem is that previously there was WebGLMemoryReporter, which contained members that were pointers to nsIMemoryReporters.  You've now conflated those two things by making WebGLMemoryMultiReporter inherit from nsIMemoryMultiReporter.  This is bad because WebGLMemoryReporter is managed with explicit new/delete, but nsIMemoryMultiReporters are managed via refcounting, as are all classes that implement nsI* interfaces;  the nsRefPtr/nsCOMPtr pointers indicate that they are refcounted.  So because of your conflation it ends up getting deleted twice, once explicitly and once due to the refcount dropping to zero.  (Mozilla's reference counting can be tricky!)

I suggest you make your patch more similar to the current code by removing that inheritance and making WebGLMemoryMultiReporter have a single |nsCOMPtr<nsIMemoryMultiReporter> mMemoryMultiReporter;|, instead of the multiple |nsCOMPtr<nsIMemoryReporter>| members it currently has.  Hopefully that makes sense.

Comment 29

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

[mentor=njn] to reflect reality; I'm still around if there is any webgl-specific question.

Comment 30

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

Comment on attachment 586333 [details] [diff] [review]
Patch v3

(Waiting for new version following Nick's r- )

Comment 31

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

Andrew, do you need help with that? Should I look into this crash?

Comment 32

[Andrew Quartey [:drexler]]

Attachment: Patch v4

(In reply to Benoit Jacob [:bjacob] from comment #31)
> Andrew, do you need help with that? Should I look into this crash?

Thanks but i figured it out myself-had to read up on XPCOM and COM to understand what goes on in the background. I tested this patch on try earlier: https://tbpl.mozilla.org/?tree=Try&rev=247310271515. I have commented out the code i will use when Bug 710054 is complete.

Comment 33

[Nicholas Nethercote [inactive]]

Comment on attachment 590749 [details] [diff] [review]
Patch v4

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

f+ because you're getting close.  The sizeOf stuff all looks good, there's just one problem with the class structure that I've explained below.  Keep going! :)

::: content/canvas/src/WebGLContext.cpp
@@ +87,5 @@
> +WebGLMemoryMultiReporter::GetBufferCacheMemoryUsed() {
> +    const ContextsArrayType & contexts = Contexts();
> +    PRInt64 result = 0;
> +    for (size_t i = 0; i < contexts.Length(); ++i) {
> +        for (size_t b = 0; b < contexts[i]->mBuffers.Length(); ++b) 

I'd use i and j for the loop indices here.

@@ +101,5 @@
> +WebGLMemoryMultiReporter::GetShaderSize() {
> +    const ContextsArrayType & contexts = Contexts();
> +    PRInt64 result = 0;
> +    for (size_t i = 0; i < contexts.Length(); ++i) {
> +        for (size_t s = 0; s < contexts[i]->mShaders.Length(); ++s) 

Ditto.

::: content/canvas/src/WebGLContext.h
@@ +1047,5 @@
>          mContext->mBuffers.RemoveElement(mMonotonicHandle);
>      }
> +    
> +    size_t SizeOfExcludingThis(nsMallocSizeOfFun aMallocSizeOf) const {
> +        return aMallocSizeOf(mData, mByteLength * sizeof(GLuint));

I should warn you that bug 715453 will very soon be landed, and it'll remove the need for the 2nd argument to aMallocSizeOf().

@@ +1051,5 @@
> +        return aMallocSizeOf(mData, mByteLength * sizeof(GLuint));
> +    }
> +
> +    size_t SizeOfIncludingThis(nsMallocSizeOfFun aMallocSizeOf) const {
> +        return aMallocSizeOf(this, sizeof(WebGLBuffer)) + SizeOfExcludingThis(aMallocSizeOf);

Ditto.

@@ +2613,5 @@
> +    {
> +      public:
> +        NS_DECL_ISUPPORTS
> +        NS_DECL_NSIMEMORYMULTIREPORTER
> +    };

Don't do that :)

What's confusing here is that the class |WebGLMemoryMultiReporter| does not inherit from |nsIMemoryMultiReporter|, as you'd expect from its name. Instead it contains a member that is a pointer to a |nsIMemoryMultiReporter|.

Really, |WebGLMemoryMultiReporter| should be renamed something like |UniqueWebGLMemoryMultiReporter| or |WebGLMemoryMultiReporterWrapper|;  it would just manage the singleton stuff.

Then you'd create a new class called |WebGLMemoryMultiReporter| that *does* inherit from |nsIMemoryMultiReporter|.  You'd initialize |mReporter| like this:

  mReporter = new WebGLMemoryMultiReporter;

And functions like |WebGLMemoryMultiReporter::MemoryMultiReporter::CollectReports| would become just |WebGLMemoryMultiReporter::CollectReports|.

Hopefully that makes sense!

Comment 34

[Andrew Quartey [:drexler]]

Attachment: Patch v5

Changes to class structure.

Comment 35

[Nicholas Nethercote [inactive]]

Comment on attachment 591187 [details] [diff] [review]
Patch v5

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

Bug 715453 has landed, so you need to remove the 2nd argument to each aMallocSizeOf() call.

Other than a couple of nits below, I'm happy!  Thanks for persisting, Andrew, not all memory reporters are as difficult as this one :)  Handing over to Benoit.

::: content/canvas/src/WebGLContext.h
@@ +1642,5 @@
> +    }
> +
> +    size_t SizeOfIncludingThis(nsMallocSizeOfFun aMallocSizeOf) const {
> +        return aMallocSizeOf(this, sizeof(WebGLShader)) + SizeOfShaderSourceExcludingThis(aMallocSizeOf) + 
> +        SizeOfTransLogExcludingThis(aMallocSizeOf);

SizeOfShareSourceExcludingThis() and SizeofTransLogExcludingThis() can both be inlined here, because this is the only place that calls them.

@@ +2617,5 @@
> +class WebGLMemoryMultiReporter : public nsIMemoryMultiReporter 
> +{
> +    public:
> +        NS_DECL_ISUPPORTS
> +        NS_DECL_NSIMEMORYMULTIREPORTER

You can move this into WebGLContextReporter.cpp now.

Comment 36

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

Comment on attachment 591187 [details] [diff] [review]
Patch v5

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

Thanks Andrew for the great work here, and sorry about the delayed review. Just a comment below, probably a follow-up bug:

::: content/canvas/src/WebGLContext.cpp
@@ +109,5 @@
>  
> +NS_IMETHODIMP
> +WebGLMemoryMultiReporter::GetExplicitNonHeap(PRInt64 *aAmount)
> +{
> +    // WebGLMemoryMultiReporterWrapper has no KIND_NONHEAP measurements.

This can be a follow-up, but KIND_NONHEAP didn't exist when I wrote the first version and it sounds like something we want here? Typically (in anything but a pure-software GL renderer), OpenGL objects are not on the heap, they live in video memory. In this case, since video memory is virtualized, they can happen to be in real memory (but not on the heap).

Comment 37

[Nicholas Nethercote [inactive]]

> Thanks Andrew for the great work here, and sorry about the delayed review.

I can land the patch.


> > +    // WebGLMemoryMultiReporterWrapper has no KIND_NONHEAP measurements.
> 
> This can be a follow-up, but KIND_NONHEAP didn't exist when I wrote the
> first version and it sounds like something we want here? Typically (in
> anything but a pure-software GL renderer), OpenGL objects are not on the
> heap, they live in video memory. In this case, since video memory is
> virtualized, they can happen to be in real memory (but not on the heap).

Is video memory always virtualized?  Even if it is, changing to KIND_NONHEAP will cause the video memory to end up in the "explicit" tree, and we probably don't want that.  In which case no follow-up is needed, IMO.

Comment 38

[Andrew Quartey [:drexler]]

Attachment: Patch v6

Update to previous patch incorporating changes from bug 710054, bug 715453 and suggestion per comment 35.
In addition, when tested with a WebGL demo: http://www.chromeexperiments.com/detail/tunneltilt/?f=webgl, i found that when mTranslationLog isn't assigned a value before the SizeOfIncludingThis call, it results in a failure. Hence, the addition of checks before SizeOfExcludingThis calls in WebGLShader.

Comment 39

[Nicholas Nethercote [inactive]]

> In addition, when tested with a WebGL demo:
> http://www.chromeexperiments.com/detail/tunneltilt/?f=webgl, i found that
> when mTranslationLog isn't assigned a value before the SizeOfIncludingThis
> call, it results in a failure. Hence, the addition of checks before
> SizeOfExcludingThis calls in WebGLShader.

What do you mean by "failure"?  Did it crash, or something else?

If this check is necessary it would be better if it were done within SizeOfExcludingThis()...

Comment 40

[Andrew Quartey [:drexler]]

To be specific, on windows debug builds, i get an assertion:_CrtIsValidHeapPointer(pUserData) which causes a hang.

Comment 41

[Nicholas Nethercote [inactive]]

(In reply to Andrew Quartey [:drexler] from comment #40)
> To be specific, on windows debug builds, i get an
> assertion:_CrtIsValidHeapPointer(pUserData) which causes a hang.

That sounds like we're passing a pointer to _msize() which wasn't created with |malloc| or |new|, which is bad.  I'll investigate before I land this.  Thanks, Andrew!

Comment 42

[Nicholas Nethercote [inactive]]

I'm working on fixing the string SizeOf* functions for bug 682431.  Once I've done that I'll be able to land this patch.  Sorry for the delay.

Comment 43

[Nicholas Nethercote [inactive]]

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

Thanks for all your work on this, Andrew!  This bug report will be closed when the patch is merged from mozilla-inbound to mozilla-central, which should happen in the next day or two.

Comment 44

[Andrew Quartey [:drexler]]

Sure. Thanks for the reviews and pointers throughout :)

Comment 45

[Ed Morley [:emorley]]

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

:-)

Comment 46

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

Woot!! Huge kudos to Andrew and Nick.

Comment 47

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

Nick, do we want a follow-up bug about reporting video memory as KIND_NONHEAP instead of the current KIND_OTHER?

Comment 48

[Nicholas Nethercote [inactive]]

(In reply to Benoit Jacob [:bjacob] from comment #47)
> Nick, do we want a follow-up bug about reporting video memory as
> KIND_NONHEAP instead of the current KIND_OTHER?

See comment 37.

Comment 49

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

(In reply to Nicholas Nethercote [:njn] from comment #37)
> Is video memory always virtualized?

Not always. Depends on OS and drivers.

> Even if it is, changing to KIND_NONHEAP
> will cause the video memory to end up in the "explicit" tree, and we
> probably don't want that.  In which case no follow-up is needed, IMO.

OK, I agree. Thanks!