The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla11

Status

()

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

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

unspecified
mozilla11
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P3])

Attachments

(6 attachments, 14 obsolete attachments)

8.66 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
5.68 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
8.10 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
7.37 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
6.80 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
11.38 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
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.
(Assignee)

Comment 1

5 years ago
Created attachment 577393 [details] [diff] [review]
kill mMapTextures
Attachment #577393 - Flags: review?(jgilbert)
(Assignee)

Comment 2

5 years ago
Created attachment 577408 [details] [diff] [review]
kill mMapTextures
Attachment #577408 - Flags: review?(jgilbert)
(Assignee)

Updated

5 years ago
Attachment #577393 - Attachment is obsolete: true
Attachment #577393 - Flags: review?(jgilbert)
(Assignee)

Comment 3

5 years ago
Created attachment 577410 [details] [diff] [review]
kill mMapBuffers
Attachment #577410 - Flags: review?(jgilbert)
(Assignee)

Comment 4

5 years ago
Created attachment 577411 [details] [diff] [review]
kill mMapShaders
Attachment #577411 - Flags: review?(jgilbert)
(Assignee)

Comment 5

5 years ago
Created attachment 577412 [details] [diff] [review]
kill mMapPrograms
Attachment #577412 - Flags: review?(jgilbert)
(Assignee)

Comment 6

5 years ago
Created attachment 577413 [details] [diff] [review]
kill mMapRenderbuffers
Attachment #577413 - Flags: review?(jgilbert)
(Assignee)

Comment 7

5 years ago
Created attachment 577414 [details] [diff] [review]
kill mMapFramebuffers
Attachment #577414 - Flags: review?(jgilbert)
(Assignee)

Updated

5 years ago
Whiteboard: [MemShrink?]
(Assignee)

Comment 8

5 years ago
Try: https://tbpl.mozilla.org/?tree=Try&rev=111dad4c8af6
(Assignee)

Comment 9

5 years ago
Created attachment 577715 [details] [diff] [review]
kill mMapTextures
Attachment #577408 - Attachment is obsolete: true
Attachment #577408 - Flags: review?(jgilbert)
Attachment #577715 - Flags: review?(jgilbert)
Created attachment 577716 [details] [diff] [review]
kill mMapBuffers
Attachment #577410 - Attachment is obsolete: true
Attachment #577410 - Flags: review?(jgilbert)
Attachment #577716 - Flags: review?(jgilbert)
Created attachment 577717 [details] [diff] [review]
kill mMapPrograms
Attachment #577412 - Attachment is obsolete: true
Attachment #577412 - Flags: review?(jgilbert)
Attachment #577717 - Flags: review?(jgilbert)
Created attachment 577719 [details] [diff] [review]
kill mMapShaders
Attachment #577411 - Attachment is obsolete: true
Attachment #577411 - Flags: review?(jgilbert)
Attachment #577719 - Flags: review?(jgilbert)
Created attachment 577721 [details] [diff] [review]
kill mMapRenderbuffers
Attachment #577413 - Attachment is obsolete: true
Attachment #577413 - Flags: review?(jgilbert)
Attachment #577721 - Flags: review?(jgilbert)
Created attachment 577723 [details] [diff] [review]
kill mMapFramebuffers
Attachment #577414 - Attachment is obsolete: true
Attachment #577414 - Flags: review?(jgilbert)
Attachment #577723 - Flags: review?(jgilbert)
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).
Assignee: nobody → bjacob
Whiteboard: [MemShrink?] → [MemShrink:P3]
Created attachment 578200 [details] [diff] [review]
1: textures
Attachment #577715 - Attachment is obsolete: true
Attachment #577715 - Flags: review?(jgilbert)
Attachment #578200 - Flags: review?(jgilbert)
Created attachment 578201 [details] [diff] [review]
2: buffers
Attachment #577716 - Attachment is obsolete: true
Attachment #577716 - Flags: review?(jgilbert)
Attachment #578201 - Flags: review?(jgilbert)
Created attachment 578202 [details] [diff] [review]
3: programs
Attachment #577717 - Attachment is obsolete: true
Attachment #577717 - Flags: review?(jgilbert)
Attachment #578202 - Flags: review?(jgilbert)
Created attachment 578203 [details] [diff] [review]
4: shaders
Attachment #577719 - Attachment is obsolete: true
Attachment #577719 - Flags: review?(jgilbert)
Attachment #578203 - Flags: review?(jgilbert)
Created attachment 578204 [details] [diff] [review]
5: renderbuffers
Attachment #577721 - Attachment is obsolete: true
Attachment #577721 - Flags: review?(jgilbert)
Attachment #578204 - Flags: review?(jgilbert)
Created attachment 578205 [details] [diff] [review]
6: framebuffers
Attachment #577723 - Attachment is obsolete: true
Attachment #577723 - Flags: review?(jgilbert)
Attachment #578205 - Flags: review?(jgilbert)
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.
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 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 on attachment 578201 [details] [diff] [review]
2: buffers

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

r+ pending above bikeshedding.
Attachment #578201 - Flags: review?(jgilbert) → review+
Comment on attachment 578202 [details] [diff] [review]
3: programs

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

r+ pending above bikeshedding.
Attachment #578202 - Flags: review?(jgilbert) → review+
Comment on attachment 578203 [details] [diff] [review]
4: shaders

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

r+ pending above bikeshedding.
Attachment #578203 - Flags: review?(jgilbert) → review+
Comment on attachment 578204 [details] [diff] [review]
5: renderbuffers

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

r+ pending above bikeshedding.
Attachment #578204 - Flags: review?(jgilbert) → review+
Comment on attachment 578205 [details] [diff] [review]
6: framebuffers

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

r+ pending above bikeshedding.
Attachment #578205 - Flags: review?(jgilbert) → review+
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.
Attachment #578200 - Attachment is obsolete: true
Attachment #578200 - Flags: review?(jgilbert)
Attachment #578437 - Flags: review?(jgilbert)
Blocks: 707033
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 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.
Attachment #578437 - Flags: review?(jgilbert) → review+
(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/ ?
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.