Last Comment Bug 705904 - Kill nsRefPtrHashtables of WebGL objects, allow unreferenced objects to be freed, fix about:memory reporting of deleted objects
: Kill nsRefPtrHashtables of WebGL objects, allow unreferenced objects to be fr...
Status: RESOLVED FIXED
[MemShrink:P3]
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
: Milan Sreckovic [:milan]
Mentors:
Depends on: 704839
Blocks: 707033
  Show dependency treegraph
 
Reported: 2011-11-28 15:01 PST by Benoit Jacob [:bjacob] (mostly away)
Modified: 2011-12-05 10:20 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
kill mMapTextures (7.12 KB, patch)
2011-11-28 15:02 PST, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
kill mMapTextures (7.34 KB, patch)
2011-11-28 16:00 PST, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
kill mMapBuffers (7.90 KB, patch)
2011-11-28 16:02 PST, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
kill mMapShaders (7.50 KB, patch)
2011-11-28 16:03 PST, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
kill mMapPrograms (4.77 KB, patch)
2011-11-28 16:03 PST, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
kill mMapRenderbuffers (6.62 KB, patch)
2011-11-28 16:04 PST, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
kill mMapFramebuffers (5.78 KB, patch)
2011-11-28 16:05 PST, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
kill mMapTextures (9.14 KB, patch)
2011-11-29 12:54 PST, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
kill mMapBuffers (8.15 KB, patch)
2011-11-29 12:55 PST, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
kill mMapPrograms (4.99 KB, patch)
2011-11-29 12:56 PST, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
kill mMapShaders (7.68 KB, patch)
2011-11-29 12:57 PST, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
kill mMapRenderbuffers (6.89 KB, patch)
2011-11-29 12:57 PST, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
kill mMapFramebuffers (6.34 KB, patch)
2011-11-29 12:58 PST, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
1: textures (11.11 KB, patch)
2011-12-01 00:57 PST, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
2: buffers (8.66 KB, patch)
2011-12-01 00:58 PST, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
Details | Diff | Splinter Review
3: programs (5.68 KB, patch)
2011-12-01 00:59 PST, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
Details | Diff | Splinter Review
4: shaders (8.10 KB, patch)
2011-12-01 00:59 PST, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
Details | Diff | Splinter Review
5: renderbuffers (7.37 KB, patch)
2011-12-01 01:00 PST, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
Details | Diff | Splinter Review
6: framebuffers (6.80 KB, patch)
2011-12-01 01:01 PST, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
Details | Diff | Splinter Review
1: textures, updated (11.38 KB, patch)
2011-12-01 16:26 PST, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
Details | Diff | Splinter Review

Description Benoit Jacob [:bjacob] (mostly away) 2011-11-28 15:01:11 PST
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) 2011-11-28 15:02:42 PST
Created attachment 577393 [details] [diff] [review]
kill mMapTextures
Comment 2 Benoit Jacob [:bjacob] (mostly away) 2011-11-28 16:00:41 PST
Created attachment 577408 [details] [diff] [review]
kill mMapTextures
Comment 3 Benoit Jacob [:bjacob] (mostly away) 2011-11-28 16:02:14 PST
Created attachment 577410 [details] [diff] [review]
kill mMapBuffers
Comment 4 Benoit Jacob [:bjacob] (mostly away) 2011-11-28 16:03:11 PST
Created attachment 577411 [details] [diff] [review]
kill mMapShaders
Comment 5 Benoit Jacob [:bjacob] (mostly away) 2011-11-28 16:03:43 PST
Created attachment 577412 [details] [diff] [review]
kill mMapPrograms
Comment 6 Benoit Jacob [:bjacob] (mostly away) 2011-11-28 16:04:19 PST
Created attachment 577413 [details] [diff] [review]
kill mMapRenderbuffers
Comment 7 Benoit Jacob [:bjacob] (mostly away) 2011-11-28 16:05:26 PST
Created attachment 577414 [details] [diff] [review]
kill mMapFramebuffers
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2011-11-28 18:54:43 PST
Try: https://tbpl.mozilla.org/?tree=Try&rev=111dad4c8af6
Comment 9 Benoit Jacob [:bjacob] (mostly away) 2011-11-29 12:54:36 PST
Created attachment 577715 [details] [diff] [review]
kill mMapTextures
Comment 10 Benoit Jacob [:bjacob] (mostly away) 2011-11-29 12:55:13 PST
Created attachment 577716 [details] [diff] [review]
kill mMapBuffers
Comment 11 Benoit Jacob [:bjacob] (mostly away) 2011-11-29 12:56:03 PST
Created attachment 577717 [details] [diff] [review]
kill mMapPrograms
Comment 12 Benoit Jacob [:bjacob] (mostly away) 2011-11-29 12:57:11 PST
Created attachment 577719 [details] [diff] [review]
kill mMapShaders
Comment 13 Benoit Jacob [:bjacob] (mostly away) 2011-11-29 12:57:41 PST
Created attachment 577721 [details] [diff] [review]
kill mMapRenderbuffers
Comment 14 Benoit Jacob [:bjacob] (mostly away) 2011-11-29 12:58:15 PST
Created attachment 577723 [details] [diff] [review]
kill mMapFramebuffers
Comment 15 Benoit Jacob [:bjacob] (mostly away) 2011-11-29 13:07:29 PST
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) 2011-12-01 00:57:38 PST
Created attachment 578200 [details] [diff] [review]
1: textures
Comment 17 Benoit Jacob [:bjacob] (mostly away) 2011-12-01 00:58:24 PST
Created attachment 578201 [details] [diff] [review]
2: buffers
Comment 18 Benoit Jacob [:bjacob] (mostly away) 2011-12-01 00:59:00 PST
Created attachment 578202 [details] [diff] [review]
3: programs
Comment 19 Benoit Jacob [:bjacob] (mostly away) 2011-12-01 00:59:33 PST
Created attachment 578203 [details] [diff] [review]
4: shaders
Comment 20 Benoit Jacob [:bjacob] (mostly away) 2011-12-01 01:00:15 PST
Created attachment 578204 [details] [diff] [review]
5: renderbuffers
Comment 21 Benoit Jacob [:bjacob] (mostly away) 2011-12-01 01:01:05 PST
Created attachment 578205 [details] [diff] [review]
6: framebuffers
Comment 22 Benoit Jacob [:bjacob] (mostly away) 2011-12-01 01:05:49 PST
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) 2011-12-01 01:08:54 PST
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 Jeff Gilbert [:jgilbert] 2011-12-01 06:07:01 PST
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 Jeff Gilbert [:jgilbert] 2011-12-01 06:08:28 PST
Comment on attachment 578201 [details] [diff] [review]
2: buffers

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

r+ pending above bikeshedding.
Comment 26 Jeff Gilbert [:jgilbert] 2011-12-01 06:09:17 PST
Comment on attachment 578202 [details] [diff] [review]
3: programs

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

r+ pending above bikeshedding.
Comment 27 Jeff Gilbert [:jgilbert] 2011-12-01 06:11:13 PST
Comment on attachment 578203 [details] [diff] [review]
4: shaders

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

r+ pending above bikeshedding.
Comment 28 Jeff Gilbert [:jgilbert] 2011-12-01 06:12:06 PST
Comment on attachment 578204 [details] [diff] [review]
5: renderbuffers

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

r+ pending above bikeshedding.
Comment 29 Jeff Gilbert [:jgilbert] 2011-12-01 06:13:16 PST
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) 2011-12-01 16:26:03 PST
Created attachment 578437 [details] [diff] [review]
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 32 Jeff Gilbert [:jgilbert] 2011-12-05 08:04:08 PST
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) 2011-12-05 08:47:36 PST
(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/ ?

Note You need to log in before you can comment on or make changes to this bug.