Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Add about:memory accounting to WebGL

RESOLVED FIXED

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: jrmuizel, Assigned: bjacob)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(6 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Reporter)

Updated

7 years ago
Assignee: nobody → bjacob
(Reporter)

Updated

7 years ago
Blocks: 637449

Updated

6 years ago
Blocks: 563700
Blocks: 648411
Blocks: 643651, 651695
Duplicate of this bug: 655363
(Assignee)

Updated

6 years ago
Blocks: 657115
Whiteboard: [MemShrink:P2]
(Assignee)

Comment 2

6 years ago
I've actually had much of theses patches for a while, but I didn't finish them and put them up for review because I hesitated on the approach.

These patches rely on keeping track only of WebGLContexts, and letting each WebGLContext enumerate its objects (such as textures). The advantage is that this is the simplest to implement, but the disadvantage is that this wouldn't account for leaked objects (such as textures) that would no longer be referenced from any WebGLContext.

For this reason I then contemplated a different approach based on plain counters updated by the objects themselves, e.g. a texture memory usage counter updated by the WebGLTexture objects when their buffers are allocated/reallocated/freed. I got stuck there for a very long time as this is very inconvenient to implement. In particular, this was inconvenient for mipmapped textures as our WebGLTexture class tries to take advantage of knowledge that a mipmap has been autogenerated to simplify its bookkeeping, so switching to/from auto-generated-mipmap state would result in complicated updates to the texture memory usage metric.

Moreover the above-mentioned problem with the initial approach, namely that it would account for textures no longer referenced from any context, turns out to not matter in practice, at least for now: a look at the code shows that there is little risk that a WebGLTexture could not be referenced from a context, since WebGLTextures can only be created by WebGLContext::createTexture which stores a reference to the newly created texture in the WebGLContext. This is currently a strong reference, by the way, and we may want to let that be a weak reference in order to help freeing texture memory more aggressively in case of unreferenced textures, even though the canonical way of freeing texture memory in WebGL is to explicitly call the deleteTexture() function.
(Assignee)

Comment 3

6 years ago
Created attachment 543345 [details] [diff] [review]
part 1: initial implementation accounting for WebGLContexts and WebGLTextures
Attachment #543345 - Flags: review?(jmuizelaar)
(Assignee)

Comment 4

6 years ago
Created attachment 543346 [details] [diff] [review]
part 2: add accounting for WebGLBuffers

coming soon: part 3 for renderbuffers
Attachment #543346 - Flags: review?(jmuizelaar)
(Assignee)

Updated

6 years ago
Attachment #543345 - Attachment description: initial implementation, accounting for WebGLContexts and WebGLTextures → part 1: initial implementation accounting for WebGLContexts and WebGLTextures
(Assignee)

Comment 5

6 years ago
Try:
http://tbpl.mozilla.org/?tree=Try&rev=c2df6455c534
(In reply to comment #2)
> 
> These patches rely on keeping track only of WebGLContexts, and letting each
> WebGLContext enumerate its objects (such as textures). The advantage is that
> this is the simplest to implement, but the disadvantage is that this
> wouldn't account for leaked objects (such as textures) that would no longer
> be referenced from any WebGLContext.
> 
> For this reason I then contemplated a different approach based on plain
> counters updated by the objects themselves, e.g. a texture memory usage
> counter updated by the WebGLTexture objects when their buffers are
> allocated/reallocated/freed.

I think the former approach (compute from scratch) is better than the latter approach (maintain some counters).  See bug 663271 comment 0 for more.  So I'm pleased you went with the former approach!
(Assignee)

Comment 7

6 years ago
Thanks for the link. Yes, the concern outlined there echoes mine.
(Assignee)

Comment 8

6 years ago
Try builds are ready at:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bjacob@mozilla.com-c2df6455c534/
(Reporter)

Comment 9

6 years ago
Comment on attachment 543345 [details] [diff] [review]
part 1: initial implementation accounting for WebGLContexts and WebGLTextures

>+    static PRInt64 GetTextureMemoryUsed() {
>+        const ContextsArrayType & contexts = Contexts();
>+        PRInt64 result = 0;
>+        for(size_t i = 0; i < contexts.Length(); ++i) {
>+            PRInt64 textureMemoryUsageForThisContext = 0;

I think I would prefer to account memory on creation and deletion instead of using enumeration. That way we're more likely to have leaks show up, but this is fine for now provided you add a comment to the enumeration code that suggests the alternative.


>+PRUint32 WebGLContext::GetTexelSize(WebGLenum format, WebGLenum type)
>+{
>+    if (type == LOCAL_GL_UNSIGNED_BYTE || type == LOCAL_GL_FLOAT) {
>+        int multiplier = type == LOCAL_GL_FLOAT ? 4 : 1;

I think that channelSize or channelBytes is a better name than multiplier.
Attachment #543345 - Flags: review?(jmuizelaar) → review+
(Reporter)

Comment 10

6 years ago
Comment on attachment 543346 [details] [diff] [review]
part 2: add accounting for WebGLBuffers

We'll eventually probably want to split these out by context, but we can wait until the about:compartments stuff lands to get an idea of the best way to expose that.
Attachment #543346 - Flags: review?(jmuizelaar) → review+
(In reply to comment #10)
> 
> We'll eventually probably want to split these out by context, but we can
> wait until the about:compartments stuff lands to get an idea of the best way
> to expose that.

about:compartments (bug 625305) will never land.  Per-compartment reporters in about:memory (bug 661474) landed this week.
Is there an easy way that I can adapt my patches to make them use per-compartment reporters? Example code?
Per-compartment reporters make sense for JS because compartments are the primary data structure in the JS engine.  See  XPConnectJSCompartmentsMultiReporter in js/src/xpconnect/src/xpcjsruntime.cpp.  It traverses the entire GC heap, starting at the compartment level and working down to individual objects, recording numbers along the way.

Do per-compartment reporters make sense for WebGL?  Does WebGL's structure use compartments?
WebGL objects have two "sides": they are XPCOM objects (ultimately created by Javascript), and they are OpenGL objects. The WebGL implementation takes care of the correspondence between these two aspects. So it doesn't matter that OpenGL objects by themselves don't care about compartments, the XPCOM side of each WebGL object should know what compartment it was associated with, just like any other JS-created object, right? Does that make sense?
Yes, that makes sense.  If you are given any random JSObject, can you tell if it's a WebGL object?  If so, you can modify CellCallback (http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/xpcjsruntime.cpp#1472) accordingly.  Note that CellCallback is part of an nsIMemoryMultiReporter, the interface for which is a bit different to nsIMemoryReporter.
Each WebGL object type implements a specific XPCOM interface so could be identified by a QueryInterface. However, looking that this CellCallBack code, it's not obvious to me how/where to do this, and also it's not clear to me why something specific has to be done for WebGL objects there whereas other JS objects don't seem to require anything specific there (this CellCallBack function does not have a lot of special cases for all sorts of objects already).
CellCallback doesn't do anything depending on the type of the object (with the exception of strings, which have a different kind anyway).  If I wanted to count RegExp objects separately, for example, then I'd have to do more querying on the object kind.  And I thought you might be able to do something similar for WebGL objects.  But I don't know if you can call QueryInterface on a JSObject.  Hmm.

Is there another way to find WebGL objects other than crawling the heap and inspecting every object?  Eg. can you iterate over the WebGL contexts and find WebGL objects that way?  For any given JSObject you can find its compartment with Cell::compartment(), because JSObject is a sub-class of Cell.
(In reply to comment #17)
> CellCallback doesn't do anything depending on the type of the object (with
> the exception of strings, which have a different kind anyway).  If I wanted
> to count RegExp objects separately, for example, then I'd have to do more
> querying on the object kind.  And I thought you might be able to do
> something similar for WebGL objects.  But I don't know if you can call
> QueryInterface on a JSObject.  Hmm.

FYI, the JSAPI allows you to convert JSObjects to XPCOM objects implementing a given interface, see e.g. xpc_qsUnwrapArg, see how it's used in CustomQS_WebGL.h.

> 
> Is there another way to find WebGL objects other than crawling the heap and
> inspecting every object?  Eg. can you iterate over the WebGL contexts and
> find WebGL objects that way?

Sure! That's exactly what my patches here are doing. The WebGLMemoryReporter implemented here maintains an array of WebGLContext*'s that is updated everytime a context is created or deleted. Each context is able to list its associated objects such as textures, etc.

> For any given JSObject you can find its
> compartment with Cell::compartment(), because JSObject is a sub-class of
> Cell.

My problem here is that by the time I can enumerate my WebGL objects, these are XPCOM objects, not JS objects. I would need an interface allowing me to get, for a given XPCOM objects, a JS object corresponding to it (I do not know if that would be unique, but that doesn't matter as all JS objects corresponding to it would belong to the same compartment).
Created attachment 543633 [details] [diff] [review]
part 3: add accounting for WebGLRenderbuffers

Also allowed myself to expand the description for textures and buffers. Please dont r- just for that.
Attachment #543633 - Flags: review?(jmuizelaar)
Created attachment 543634 [details] [diff] [review]
part 4: add accounting for WebGLBbuffer caches

WebGLBuffer stores on the heap a copy of the buffer data in case of element array buffers. That seemed worth reporting. I also double checked the implementation that we don't have a stupid leak there that wouldn't be accounted for.
Attachment #543634 - Flags: review?(jmuizelaar)
Created attachment 543650 [details] [diff] [review]
part 5: add accounting for WebGLShaders

That should be it. Shaders store two C strings: the source, and the translation log. Both would be interesting to know if they get abnormally high. Large source size would indicate a WebGL page with absurdly big shaders; large translation logs could happen if shader compiler has gone crazy, or could indicate that there is something weird in a shader.
Attachment #543650 - Flags: review?(jmuizelaar)
Note that I've been a bit aggressive with adding probes as I have no idea, if the WebGL leak bug reports are real, where the leak is, so I'm adding probes on everything that could conceivably eat memory. It will be time to remove useless probes once concerns of WebGL leaks are gone, I wish we didn't have worse problems than useless probes. Also, these only show up in about:memory if WebGL has been used, so it won't disturb too much people who don't care.
Try:
http://tbpl.mozilla.org/?tree=Try&rev=087d304b9757
(Reporter)

Updated

6 years ago
Attachment #543633 - Flags: review?(jmuizelaar) → review+
(Reporter)

Updated

6 years ago
Attachment #543634 - Flags: review?(jmuizelaar) → review+
(Reporter)

Updated

6 years ago
Attachment #543650 - Flags: review?(jmuizelaar) → review+
http://hg.mozilla.org/mozilla-central/rev/966e8782acce
http://hg.mozilla.org/mozilla-central/rev/cfbc51123f1b
http://hg.mozilla.org/mozilla-central/rev/b750b070b26b
http://hg.mozilla.org/mozilla-central/rev/1f255e99e205
http://hg.mozilla.org/mozilla-central/rev/f6ccce7761cf
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Shouldn't the nsIMemoryReporter instances be held in nsCOMPtrs?  Otherwise, I can't see how they are ever freed.  This was pointed out to me by Nick in bug 672755 comment 10, where I copied the same bug into my code.
Thanks for the catch. Making them nsRefPtrs, as I don't see the need for nsCOMPtr here.
Created attachment 548864 [details] [diff] [review]
fix reporters leak
Attachment #548864 - Flags: review?(jmuizelaar)
(In reply to comment #26)
> Making them nsRefPtrs, as I don't see the need for nsCOMPtr here.

My understanding is that you use nsRefPtr for concrete classes, but nsCOMPtr for abstract classes, and this is the latter.
Nicholas is correct. nsRefPtr can cause build errors when used with certain abstract XPCOM interfaces, so prefer nsCOMPtr if it's an interface type.
Created attachment 549101 [details] [diff] [review]
fix reporters leak
Attachment #548864 - Attachment is obsolete: true
Attachment #548864 - Flags: review?(jmuizelaar)
Attachment #549101 - Flags: review?(jmuizelaar)
(Reporter)

Updated

6 years ago
Attachment #549101 - Flags: review?(jmuizelaar) → review+
http://hg.mozilla.org/mozilla-central/rev/55b4a5e8b5a3
You need to log in before you can comment on or make changes to this bug.