Use LinkedList instead of arrays for WebGLContext to track WebGL objects

RESOLVED FIXED in mozilla15

Status

()

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

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

unspecified
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: webgl-next)

Attachments

(1 attachment, 1 obsolete attachment)

Created attachment 626982 [details] [diff] [review]
use LinkedList for WebGL object arrays in WebGLContext

WebGLContext needs to keep track of all the WebGL objects associated with this context. We currently use arrays for that. The hope was that by appending new elements at the end, assuming that typically the newest elements are the most likely to get destroyed, destroying WebGL objects should be cheap i.e. only a small portion of the array would have to be shifted back.

In Bug 754303 we saw that that was not the case. This mechanism was causing long GC pauses as objects were getting GC'd in random order, so we were doing many large array shifts. See the stack in Bug 754303 Comment 4.

By itself, Bug 754303 was trivial to fix as it was only about WebGLUniformLocation which didn't actually need to be tracked. However, the same bug could happen with other WebGL object types. What mitigates it is that other WebGL object types are typically larger, so less likely to be created and destroyed in large numbers, but still. Some pages have over 1000 buffers, our current implementation has quadratic complexity (so we're talking about realistically 1e+6 operations) and this affects the duration of GC pauses.

The original motivation for this WebGLFastArray class was to have cheap and easy element removal. But MFBT LinkedList is designed exactly for that. Read the comment in mfbt/LinkedList.h. The contained type must inherit LinkedListElement<Itself> which makes it store its own prev/next pointers. This allows to quickly remove an element knowing just itself, without having to also know a pointer to where in the linked list it's stored.
Attachment #626982 - Flags: review?(jgilbert)
Comment on attachment 626982 [details] [diff] [review]
use LinkedList for WebGL object arrays in WebGLContext

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

::: content/canvas/src/WebGLContext.h
@@ +1560,5 @@
>      void Delete() {
>          mImageInfos.Clear();
>          mContext->MakeContextCurrent();
>          mContext->gl->fDeleteTextures(1, &mGLName);
> +        remove();

I am hesitant about inheriting from LinkedListElement, but I really don't like that WebGLTexture::remove() opaquely removes |this| from mContext->mTextures.

@@ +2058,5 @@
>      ~WebGLShader() {
>          DeleteOnce();
>      }
>      
> +    size_t SizeOfIncludingThis(nsMallocSizeOfFun aMallocSizeOf) const {

Yay const.
Attachment #626982 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 2

5 years ago
Created attachment 627213 [details] [diff] [review]
use LinkedList for WebGL object arrays in WebGLContext

(In reply to Jeff Gilbert [:jgilbert] from comment #1)
> Comment on attachment 626982 [details] [diff] [review]
> use LinkedList for WebGL object arrays in WebGLContext
> 
> Review of attachment 626982 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContext.h
> @@ +1560,5 @@
> >      void Delete() {
> >          mImageInfos.Clear();
> >          mContext->MakeContextCurrent();
> >          mContext->gl->fDeleteTextures(1, &mGLName);
> > +        remove();
> 
> I am hesitant about inheriting from LinkedListElement, but I really don't
> like that WebGLTexture::remove() opaquely removes |this| from
> mContext->mTextures.

I hear you. Nontrivial problem. This new version makes it hopefully explicit enough, both with explicit :: scoping and with a comment. To get a fully satisfactory solution, we would have to change how LinkedList works. Currently LinkedList is designed so one LinkedListElement can participate in only one LinkedList, so the list is implicit from the element.
Attachment #626982 - Attachment is obsolete: true
Attachment #627213 - Flags: review?(jgilbert)
Comment on attachment 627213 [details] [diff] [review]
use LinkedList for WebGL object arrays in WebGLContext

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

::: content/canvas/src/WebGLContext.h
@@ +1415,5 @@
>          mContext->gl->fDeleteBuffers(1, &mGLName);
>          free(mData);
>          mData = nsnull;
>          mByteLength = 0;
> +        LinkedListElement<WebGLBuffer>::remove(); // remove from mContext->mBuffers

Close enough.
Attachment #627213 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 4

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/63e714d8902e
Assignee: nobody → bjacob
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/63e714d8902e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.