705904 - Kill nsRefPtrHashtables of WebGL objects, allow unreferenced objects to be freed, fix about:memory reporting of deleted objects

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

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

Currently, WebGLContext has nsRefPtrHashtables of all WebGL objects created from it, like mMapTextures.

First, a hashtable is useless, a nsTArray is enough. The point of a hashtable was to be able to look up WebGL objects from just the OpenGL integer 'name', but we don't use that capability at all.

Second, having RefPtrs here means that the WebGLContext keeps alive all WebGL objects, even if they go completely unreferenced, unless deleteXxx() is called on them. For sure, explicit memory management is strongly recommended with WebGL and, in many use cases, is necessary to get acceptable memory behavior. So I'm not advocating changing that or encouraging people to ignore deleteXxx() functions. But the question is what should we do about bad code that does not properly use deleteXxx() functions? Allowing unreferenced objects to be freed by the garbage collector does not change semantics at all, and can result in lower memory usage in real-world (if pathological) cases, so I think we should do it. Also do note that the traditional argument about the GC being completely impredictable isn't true anymore: a page running an animation with WebGL will trigger the GC after at most 20 seconds, since bug 656120 landed.

Thirdly, about:memory accounting of delete-requested WebGL objects was broken. If a WebGL object is delete-requested but is still referenced by another WebGL object (e.g. a WebGLTexture by a WebGLFramebuffer), then it isn't actually freed. Our current code gets fooled in this situation and incorrectly omits to account for such objects.

Comment 1

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

Attachment: kill mMapTextures

Comment 2

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

Attachment: kill mMapTextures

Comment 3

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

Attachment: kill mMapBuffers

Comment 4

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

Attachment: kill mMapShaders

Comment 5

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

Attachment: kill mMapPrograms

Comment 6

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

Attachment: kill mMapRenderbuffers

Comment 7

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

Attachment: kill mMapFramebuffers

Comment 8

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=111dad4c8af6

Comment 9

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

Attachment: kill mMapTextures

Comment 10

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

Attachment: kill mMapBuffers

Comment 11

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

Attachment: kill mMapPrograms

Comment 12

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

Attachment: kill mMapShaders

Comment 13

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

Attachment: kill mMapRenderbuffers

Comment 14

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

Attachment: kill mMapFramebuffers

Comment 15

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

New try: https://tbpl.mozilla.org/?tree=Try&rev=e978b048d6f5

This new version of these patches should perform better:
 - we avoid being useless slow when deleting arrays of objects (remove useless search for element and shrinking of array, N times)
 - we use sorted insertion and deletion so that deletion doesn't involve a search everytime. Here I'm actually *more* concerned about deletion time than I am about creation time, because deletion time means duration of GC pauses. My reason for not being concerned about creation time is that the only expensive part in that InsertElementSorted call is resizing the array and copying its contents, and that's clearly less expensive than what scripts do right after when they create a texture (the subsequent texture upload is more expensive) or create a shader (the subsequent shader compilation is more expensive) etc. In short, scripts do very expensive things anyway when they create objects, while on the other hand object deletion is not under scripts' control (depends on when GC happens) and is not inherently expensive, and is quite performance critical (shorter GC pauses matter).

Comment 16

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

Attachment: 1: textures

Comment 17

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

Attachment: 2: buffers

Comment 18

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

Attachment: 3: programs

Comment 19

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

Attachment: 4: shaders

Comment 20

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

Attachment: 5: renderbuffers

Comment 21

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

Attachment: 6: framebuffers

Comment 22

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

OK this time I'm happy about the result: no compromise anymore:
 - adding objects is fast because it's just appending at the end of the array. In the previous version of the patches, it depended on pointers, now it doesn't anymore, we just append at the end so it's always cheap.
 - removing objects is still fast because the arrays are still sorted, but in a different sense: it's now an array of (pointer,id) pairs where the ids are monotonically increasing.
 - having this separate class allowed for a cleaner design.

Comment 23

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

note that since the newest arrays elements are now always sorted at the end of the array, removing them is also faster (the remaining part of the array that has to be shifted, is typically small).

Comment 24

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 578200 [details] [diff] [review]
1: textures

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

Leaving it r? for one metric round of bikeshedding.

::: content/canvas/src/WebGLContext.h
@@ +389,5 @@
>  protected:
>      T *mRawPtr;
>  };
>  
> +typedef PRUint64 WebGLMonotonicIdType;

If we change 'mMonotonicId' below, we will probably want to match that here.

@@ +392,5 @@
>  
> +typedef PRUint64 WebGLMonotonicIdType;
> +
> +template<typename WebGLObjectType>
> +class WebGLArrayOfPlainPtrsToAllObjects

This should probably have a more succinct name.

@@ +425,5 @@
> +    size_t Length() const {
> +        return mArray.Length();
> +    }
> +
> +    void DeleteElementsAndClear()

Not loving this name that much either. 'Delete elements' sounds like it's just freeing internal list memory before clearing. 'DeleteObjectsAndClear' might be better?

@@ +456,5 @@
> +        return mCurrentMonotonicId.value();
> +    }
> +
> +    nsTArray<Entry> mArray;
> +    CheckedInt<WebGLMonotonicIdType> mCurrentMonotonicId;

I don't love that we don't explicitly initialize this to 0, but it is done in the default constructor for CheckedInt, so we're fine.

@@ +850,5 @@
>      WebGLRefPtr<WebGLFramebuffer> mBoundFramebuffer;
>      WebGLRefPtr<WebGLRenderbuffer> mBoundRenderbuffer;
>  
>      // lookup tables for GL name -> object wrapper
> +    WebGLArrayOfPlainPtrsToAllObjects<WebGLTexture> mTextures;

I think 'mTextureList' would be better. It makes lines like 'mTextureList.DeleteElementsAndClear();' make more sense

@@ +1163,5 @@
>          , mFakeBlackStatus(DoNotNeedFakeBlack)
>      {
>          mContext->MakeContextCurrent();
>          mContext->gl->fGenTextures(1, &mGLName);
> +        mMonotonicId = mContext->mTextures.AppendElement(this);

I think the ideal for this line would be closer to:
mTrackingId = mContext->mTextureList.Add(this);

@@ +1174,5 @@
>      void Delete() {
>          mImageInfos.Clear();
>          mContext->MakeContextCurrent();
>          mContext->gl->fDeleteTextures(1, &mGLName);
> +        mContext->mTextures.OnElementDeleted(mMonotonicId);

Something like:
mContext->mTextureList.OnObjectDeleted(mTrackingId);
?

@@ +1285,5 @@
>  
>      bool mHaveGeneratedMipmap;
>      FakeBlackStatus mFakeBlackStatus;
>  
> +    WebGLMonotonicIdType mMonotonicId;

How about mHandleId instead? mMonotonicId doesn't tell anything about what it is used for.

Comment 25

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 578201 [details] [diff] [review]
2: buffers

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

r+ pending above bikeshedding.

Comment 26

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 578202 [details] [diff] [review]
3: programs

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

r+ pending above bikeshedding.

Comment 27

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 578203 [details] [diff] [review]
4: shaders

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

r+ pending above bikeshedding.

Comment 28

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 578204 [details] [diff] [review]
5: renderbuffers

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

r+ pending above bikeshedding.

Comment 29

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 578205 [details] [diff] [review]
6: framebuffers

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

r+ pending above bikeshedding.

Comment 30

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

Attachment: 1: textures, updated

Here's a different design. The array class is renamed to WebGLFastArray which is just what it is, a flavor of Array that's designed for WebGL use and that's fast for our needs, but otherwise is just a general array class: it doesn't call DeleteOnce anymore, it could be used with any ElementType. If you want we can rename it to WebGLFastRemovalArray to be more specific, but I don't think it's worth it.

So we now use e.g. WebGLFastArray<WebGLTexture*> which makes it clear that we only store plain pointers there.

I removed the bool flag to decide whether to actually remove elements from the array. Instead, arrays are now destruct last-to-first like this,

    while (mTextures.Length())
        mTextures.Last()->DeleteOnce();

and I don't care about the slight inefficiency of doing the binary search for every element: that binary search is fast enough (basically 10 branchings, for an array of 1000 elements), compared to the makecurrent and glDeleteXxx business we have to do for each element.

In this way, WebGLFastArray is really just a general-purpose array class, not tied to WebGL types.

Comment 31

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

Oops, I'm evil: I landed this without realizing that part 1 was still pending review. Let's do a post mortem review and change what you think needs changing.

http://hg.mozilla.org/integration/mozilla-inbound/rev/a9f0158d9494
http://hg.mozilla.org/integration/mozilla-inbound/rev/eaf41f64aad7
http://hg.mozilla.org/integration/mozilla-inbound/rev/111be7b1a9e8
http://hg.mozilla.org/integration/mozilla-inbound/rev/724663b7a0ce
http://hg.mozilla.org/integration/mozilla-inbound/rev/905f0d81cc47
http://hg.mozilla.org/integration/mozilla-inbound/rev/4efe225df2e8

Comment 32

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 578437 [details] [diff] [review]
1: textures, updated

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

Here's the missing review. Not much to say, luckily. We can always submit more bugs, if we need to, though. :)

::: content/canvas/src/WebGLContext.cpp
@@ +347,5 @@
>  
>      mAttribBuffers.Clear();
>  
> +    while (mTextures.Length())
> +        mTextures.Last()->DeleteOnce();

I would think it would be nicer to have this as a member function in the purpose-built array class, but I am split on whether it's nicer to leave it relatively specialized.

::: content/canvas/src/WebGLContext.h
@@ +1258,5 @@
>  
>      bool mHaveGeneratedMipmap;
>      FakeBlackStatus mFakeBlackStatus;
>  
> +    WebGLMonotonicHandle mMonotonicHandle;

I still don't think it's relevant to the client class that the handle is monotonic, but this is probably fine.

Comment 33

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

(In reply to Jeff Gilbert [:jgilbert] from comment #32)
> Comment on attachment 578437 [details] [diff] [review] [diff] [details] [review]
> 1: textures, updated
> 
> Review of attachment 578437 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Here's the missing review. Not much to say, luckily. We can always submit
> more bugs, if we need to, though. :)
> 
> ::: content/canvas/src/WebGLContext.cpp
> @@ +347,5 @@
> >  
> >      mAttribBuffers.Clear();
> >  
> > +    while (mTextures.Length())
> > +        mTextures.Last()->DeleteOnce();
> 
> I would think it would be nicer to have this as a member function in the
> purpose-built array class, but I am split on whether it's nicer to leave it
> relatively specialized.

[I read 'specialized' as 'unspecialized']

Yeah, I thought that the benefit of keeping it unspecialized outweighed the benefit of having such a custom delete-array function.

> 
> ::: content/canvas/src/WebGLContext.h
> @@ +1258,5 @@
> >  
> >      bool mHaveGeneratedMipmap;
> >      FakeBlackStatus mFakeBlackStatus;
> >  
> > +    WebGLMonotonicHandle mMonotonicHandle;
> 
> I still don't think it's relevant to the client class that the handle is
> monotonic, but this is probably fine.

I guess that what I was trying to achieve here is make it clear to the client what this handle is going to be useful for. Maybe s/Monotonic/Search/ ?

Comment 34

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mozilla-central/rev/a9f0158d9494
http://hg.mozilla.org/mozilla-central/rev/eaf41f64aad7
http://hg.mozilla.org/mozilla-central/rev/111be7b1a9e8
http://hg.mozilla.org/mozilla-central/rev/724663b7a0ce
http://hg.mozilla.org/mozilla-central/rev/905f0d81cc47
http://hg.mozilla.org/mozilla-central/rev/4efe225df2e8