638549 - Add about:memory accounting to WebGL

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Comment 2

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

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.

Comment 3

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

Attachment: part 1: initial implementation accounting for WebGLContexts and WebGLTextures

Comment 4

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

Attachment: part 2: add accounting for WebGLBuffers

coming soon: part 3 for renderbuffers

Comment 5

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

Try:
http://tbpl.mozilla.org/?tree=Try&rev=c2df6455c534

Comment 6

[Nicholas Nethercote [inactive]]

(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!

Comment 7

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

Thanks for the link. Yes, the concern outlined there echoes mine.

Comment 8

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

Try builds are ready at:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c2df6455c534/

Comment 9

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 11

[Nicholas Nethercote [inactive]]

(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.

Comment 12

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

Is there an easy way that I can adapt my patches to make them use per-compartment reporters? Example code?

Comment 13

[Nicholas Nethercote [inactive]]

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?

Comment 14

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

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?

Comment 15

[Nicholas Nethercote [inactive]]

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.

Comment 16

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

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).

Comment 17

[Nicholas Nethercote [inactive]]

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.

Comment 18

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

(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).

Comment 19

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

Attachment: part 3: add accounting for WebGLRenderbuffers

Also allowed myself to expand the description for textures and buffers. Please dont r- just for that.

Comment 20

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

Attachment: 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.

Comment 21

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

Attachment: 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.

Comment 22

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

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.

Comment 23

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

Try:
http://tbpl.mozilla.org/?tree=Try&rev=087d304b9757

Comment 24

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

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

Comment 25

[Matthew Gregan [:kinetik]]

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.

Comment 26

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

Thanks for the catch. Making them nsRefPtrs, as I don't see the need for nsCOMPtr here.

Comment 27

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

Attachment: fix reporters leak

Comment 28

[Nicholas Nethercote [inactive]]

(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.

Comment 29

[Josh Matthews [:jdm]]

Nicholas is correct. nsRefPtr can cause build errors when used with certain abstract XPCOM interfaces, so prefer nsCOMPtr if it's an interface type.

Comment 30

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

Attachment: fix reporters leak

Comment 31

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

http://hg.mozilla.org/mozilla-central/rev/55b4a5e8b5a3