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

RESOLVED FIXED in mozilla13

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bjacob, Assigned: drexler)

Tracking

unspecified
mozilla13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=njn] [MemShrink:P2] [gfx.relnote.13] [lang=c++])

Attachments

(2 attachments, 4 obsolete attachments)

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/

Updated

5 years ago
Whiteboard: [mentor=bjacob] [MemShrink?] → [mentor=bjacob] [MemShrink?] [lang=c++]

Updated

5 years ago
QA Contact: canvas.webgl → bjacob
> 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! :)
Whiteboard: [mentor=bjacob] [MemShrink?] [lang=c++] → [mentor=bjacob] [MemShrink:P2] [lang=c++]
(Assignee)

Comment 2

5 years ago
This looks interesting. I will take a shot at this.
(Reporter)

Comment 3

5 years ago
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.
(Assignee)

Updated

5 years ago
Assignee: nobody → andrew.quartey
(Assignee)

Comment 4

5 years ago
Created attachment 583562 [details] [diff] [review]
Patch v1

Set up WebGLMemoryReporter as a MemoryMultiReporter

Updated

5 years ago
Attachment #583562 - Flags: review?(bjacob)
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...
> For example, if you were reporting the memory usage of each WebGL context

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

Comment 7

5 years ago
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?
> 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.
(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.)
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 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.
Attachment #583562 - Flags: feedback+
(Assignee)

Comment 12

5 years ago
Benoit, i will take care of that. Should any issue come up, i will ping you on irc.
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 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.
(Assignee)

Comment 15

5 years ago
Created attachment 584011 [details] [diff] [review]
Patch v2
Attachment #583562 - Attachment is obsolete: true
Attachment #583562 - Flags: review?(bjacob)
Attachment #584011 - Flags: review?(n.nethercote)
Attachment #584011 - Flags: review?(bjacob)
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.
Attachment #584011 - Flags: review?(n.nethercote) → review-
Comment on attachment 584011 [details] [diff] [review]
Patch v2

(waiting for next version incorporating Nick's comments)
Attachment #584011 - Flags: review?(bjacob)
(Assignee)

Comment 18

5 years ago
Totally got the wrong idea regarding the measurements. Okay, a newer patch incorporating Nicks comments should be ready by tomorrow at the latest.
(Assignee)

Comment 19

5 years ago
Created attachment 586333 [details] [diff] [review]
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.
Attachment #584011 - Attachment is obsolete: true
Attachment #586333 - Flags: review?(n.nethercote)
Attachment #586333 - Flags: review?(bjacob)
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.
Attachment #586333 - Flags: review?(n.nethercote) → review-
Depends on: 710054
(Assignee)

Comment 21

5 years ago
(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
> 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.
(Assignee)

Comment 23

5 years ago
Realized that earlier and moved those definitions to the .cpp files. Currently rebuilding it -an hour plus affair on my dinosaur :-(
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.)
(Assignee)

Comment 25

5 years ago
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.
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.
(Assignee)

Comment 27

5 years ago
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.
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.
[mentor=njn] to reflect reality; I'm still around if there is any webgl-specific question.
Whiteboard: [mentor=bjacob] [MemShrink:P2] [lang=c++] → [mentor=njn] [MemShrink:P2] [lang=c++]
Comment on attachment 586333 [details] [diff] [review]
Patch v3

(Waiting for new version following Nick's r- )
Attachment #586333 - Flags: review?(bjacob)
Andrew, do you need help with that? Should I look into this crash?
(Assignee)

Comment 32

5 years ago
Created attachment 590749 [details] [diff] [review]
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.
Attachment #586333 - Attachment is obsolete: true
Attachment #590749 - Flags: feedback?(n.nethercote)
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!
Attachment #590749 - Flags: feedback?(n.nethercote) → feedback+
(Assignee)

Comment 34

5 years ago
Created attachment 591187 [details] [diff] [review]
Patch v5

Changes to class structure.
Attachment #590749 - Attachment is obsolete: true
Attachment #591187 - Flags: feedback?(n.nethercote)
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.
Attachment #591187 - Flags: review?(bjacob)
Attachment #591187 - Flags: feedback?(n.nethercote)
Attachment #591187 - Flags: feedback+
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).
Attachment #591187 - Flags: review?(bjacob) → review+
> 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.
(Assignee)

Comment 38

5 years ago
Created attachment 595291 [details] [diff] [review]
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.
Attachment #595291 - Flags: feedback?(n.nethercote)
> 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()...
(Assignee)

Comment 40

5 years ago
To be specific, on windows debug builds, i get an assertion:_CrtIsValidHeapPointer(pUserData) which causes a hang.
(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!
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.
Attachment #595291 - Flags: feedback?(n.nethercote) → feedback+
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.
(Assignee)

Comment 44

5 years ago
Sure. Thanks for the reviews and pointers throughout :)
https://hg.mozilla.org/mozilla-central/rev/ac02ca06e127

:-)
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Woot!! Huge kudos to Andrew and Nick.
Nick, do we want a follow-up bug about reporting video memory as KIND_NONHEAP instead of the current KIND_OTHER?
(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.
(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!

Updated

5 years ago
Whiteboard: [mentor=njn] [MemShrink:P2] [lang=c++] → [mentor=njn] [MemShrink:P2] [gfx.relnote.13] [lang=c++]
You need to log in before you can comment on or make changes to this bug.