811181 - GLContext should delete its outstanding objects when destroyed

Status: NEW

(Core :: Graphics, enhancement, P5)

Description

[Kelsey Gilbert [:jgilbert]]

With share groups, objects created in one context will persist until all shared contexts are destroyed. If we share with a long-running context (like the OGL Layers GLContext), we effectively never destroy these orphaned objects.

We should destroy these objects at context destruction time to cleanly cap their lifetimes. We should also consider asserting that we don't almost leak GL objects, even though we are cleaning up anyways.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: patch

Comment 2

[Kelsey Gilbert [:jgilbert]]

Try run spinning up:
https://tbpl.mozilla.org/?tree=Try&rev=3cb657284ec7

Comment 3

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

Comment on attachment 680916 [details] [diff] [review]
patch

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

Let's do another round:

::: gfx/gl/GLContext.cpp
@@ +1770,5 @@
>  
>          fDeleteProgram(mBlitProgram);
>          mBlitProgram = 0;
>          fDeleteFramebuffers(1, &mBlitFramebuffer);
>          mBlitFramebuffer = 0;

Are the above calls still useful with the below  DeleteOutstandingObjects(); ?

@@ +3122,5 @@
>  #endif /* DEBUG */
>  
> +
> +bool
> +GLContext::HasOutstandingObjects()

const qualify?

@@ +3132,5 @@
> +           !mRenderbuffers.empty() ||
> +           !mTextures.empty();
> +}
> +
> +#define TEMP_REPORT_OBJECTS(ObjSet,TypesStr) \

This doesn't need to be a macro. Does it?

@@ +3167,5 @@
> +}
> +
> +#undef TEMP_REPORT_OBJECTS
> +
> +#define TEMP_DELETE_OBJECTS_1(Func,ObjSet,TypeStr) \

Doesn't need to be a macro either, right? You could even use one of the function pointer typedefs from GLContextSymbols.

::: gfx/gl/GLContext.h
@@ +560,5 @@
>          mTexBlit_UseDrawNotCopy = Preferences::GetBool("gl.blit-draw-not-copy", false);
>      }
>  
>      virtual ~GLContext() {
>          NS_ASSERTION(IsDestroyed(), "GLContext implementation must call MarkDestroyed in destructor!");

Not in your patch, but this NS_ASSERTION looks like a great candidate for being fatal.

@@ +3087,5 @@
> +     std::set<GLuint> mShaders;
> +     std::set<GLuint> mBuffers;
> +     std::set<GLuint> mFramebuffers;
> +     std::set<GLuint> mRenderbuffers;
> +     std::set<GLuint> mTextures;

In a follow-up patch --- this doesn't block landing this patch --- it would be nice to move this to a d-pointer kind of idiom: only declare an opaque Private nested class here and an opaque Private *mPrivate; pointer here; implement that in GLContext.cpp. So only this .cpp file would have to #include<set>.

Comment 4

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #3)
> Comment on attachment 680916 [details] [diff] [review]
> patch
> 
> Review of attachment 680916 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Let's do another round:
> 
> ::: gfx/gl/GLContext.cpp
> @@ +1770,5 @@
> >  
> >          fDeleteProgram(mBlitProgram);
> >          mBlitProgram = 0;
> >          fDeleteFramebuffers(1, &mBlitFramebuffer);
> >          mBlitFramebuffer = 0;
> 
> Are the above calls still useful with the below  DeleteOutstandingObjects();
> ?
Functionally, no, but they're important semantically. Ideally, DeleteOutstandingObjects() should be a no op, and we should follow this bug up with a patch that asserts that this is so in DEBUG builds. (Which fails on Mac, iirc)
> @@ +3122,5 @@
> >  #endif /* DEBUG */
> >  
> > +
> > +bool
> > +GLContext::HasOutstandingObjects()
> 
> const qualify?
An excellent idea.

> 
> @@ +3132,5 @@
> > +           !mRenderbuffers.empty() ||
> > +           !mTextures.empty();
> > +}
> > +
> > +#define TEMP_REPORT_OBJECTS(ObjSet,TypesStr) \
> 
> This doesn't need to be a macro. Does it?
Nope, not at all.
> 
> @@ +3167,5 @@
> > +}
> > +
> > +#undef TEMP_REPORT_OBJECTS
> > +
> > +#define TEMP_DELETE_OBJECTS_1(Func,ObjSet,TypeStr) \
> 
> Doesn't need to be a macro either, right? You could even use one of the
> function pointer typedefs from GLContextSymbols.
I don't love the idea of using, say, PFNGLDELETETEXTURES for both fDeleteTextures and fDeleteFramebuffers.
I could just use a typedef, though.
> 
> ::: gfx/gl/GLContext.h
> @@ +560,5 @@
> >          mTexBlit_UseDrawNotCopy = Preferences::GetBool("gl.blit-draw-not-copy", false);
> >      }
> >  
> >      virtual ~GLContext() {
> >          NS_ASSERTION(IsDestroyed(), "GLContext implementation must call MarkDestroyed in destructor!");
> 
> Not in your patch, but this NS_ASSERTION looks like a great candidate for
> being fatal.
True, will change.
> 
> @@ +3087,5 @@
> > +     std::set<GLuint> mShaders;
> > +     std::set<GLuint> mBuffers;
> > +     std::set<GLuint> mFramebuffers;
> > +     std::set<GLuint> mRenderbuffers;
> > +     std::set<GLuint> mTextures;
> 
> In a follow-up patch --- this doesn't block landing this patch --- it would
> be nice to move this to a d-pointer kind of idiom: only declare an opaque
> Private nested class here and an opaque Private *mPrivate; pointer here;
> implement that in GLContext.cpp. So only this .cpp file would have to
> #include<set>.
It's something to consider. I would almost prefer to hide it with a wrapper class around set<GLuint>. We can discuss this in bug 812328.

Comment 5

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 1: GC objects at GLContext destruction time

Comment 6

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 2: Allow deleting objects on destroyed contexts to delete from the shared context, if present

Comment 7

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 2: Fix 5-space indents to 4-space.

No other changes here.

Comment 8

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3: Allow deleting objects on destroyed contexts to delete from the shared context, if present

Comment 9

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

Comment on attachment 686786 [details] [diff] [review]
patch 1: GC objects at GLContext destruction time

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

::: gfx/gl/GLContext.cpp
@@ +2628,5 @@
> +
> +    std::set<GLuint>::iterator itr = objSet.begin();
> +    std::set<GLuint>::iterator end = objSet.end();
> +    if (itr != end) {
> +        for (; true; ++itr) {

I'd rather use a while(true) here, up to you.

@@ +2657,5 @@
> +
> +typedef void (GLContext::*pfnDeleteObjectT)(GLuint);
> +typedef void (GLContext::*pfnDeleteObjectArrT)(GLsizei, GLuint*);
> +
> +#define TEMP_CALL_MEMBER_PFN(object,memberPfn)  ((object).*(memberPfn))

What is the point of this macro?

Comment 10

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 686801 [details] [diff] [review]
patch 3: Allow deleting objects on destroyed contexts to delete from the shared context, if present

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

This all looks really good.

I'm a bit worried that when we call MarkDestroyed() on GLX that MakeCurrent() will succeed, but deleting the textures will crash. ISTR that's what was happening originally and was the reason for this code being added.

It might be worth just calling MarkInviable unconditionally at the start of MarkDestroyed. Or leave it as is and hope that things aren't quite that insane. We don't have tinderbox coverage for GLX currently though.

Comment 11

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #9)
> Comment on attachment 686786 [details] [diff] [review]
> patch 1: GC objects at GLContext destruction time
> 
> Review of attachment 686786 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLContext.cpp
> @@ +2628,5 @@
> > +
> > +    std::set<GLuint>::iterator itr = objSet.begin();
> > +    std::set<GLuint>::iterator end = objSet.end();
> > +    if (itr != end) {
> > +        for (; true; ++itr) {
> 
> I'd rather use a while(true) here, up to you.
It's a non-trivial structure, so I think I agree.
> 
> @@ +2657,5 @@
> > +
> > +typedef void (GLContext::*pfnDeleteObjectT)(GLuint);
> > +typedef void (GLContext::*pfnDeleteObjectArrT)(GLsizei, GLuint*);
> > +
> > +#define TEMP_CALL_MEMBER_PFN(object,memberPfn)  ((object).*(memberPfn))
> 
> What is the point of this macro?
Calling a pointer to a member function is one of the wonkier syntaxes. I actually had this wrong, anyways: ((object)->*(memberpfn))
Because this syntax is so strange, I think it's best to use a macro to make it clear what we're doing, instead of:
(gl->*pfnDelete)(cur)

Comment 12

[Kelsey Gilbert [:jgilbert]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> Comment on attachment 686801 [details] [diff] [review]
> patch 3: Allow deleting objects on destroyed contexts to delete from the
> shared context, if present
> 
> Review of attachment 686801 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This all looks really good.
> 
> I'm a bit worried that when we call MarkDestroyed() on GLX that
> MakeCurrent() will succeed, but deleting the textures will crash. ISTR
> that's what was happening originally and was the reason for this code being
> added.
> 
> It might be worth just calling MarkInviable unconditionally at the start of
> MarkDestroyed. Or leave it as is and hope that things aren't quite that
> insane. We don't have tinderbox coverage for GLX currently though.

Ok, I'll try that and see what Try thinks.

Comment 13

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 4: Catch when we delete the currently bound FB out from under ourselves.

Comment 14

[Kelsey Gilbert [:jgilbert]]

Try run is looking clean, but some important tests are still coming in:
https://tbpl.mozilla.org/?tree=Try&rev=0e2a8e042800

Comment 15

[Kelsey Gilbert [:jgilbert]]

Nope, still has Linux M1 errors. I think I know what it is, though.

Comment 16

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 686799 [details] [diff] [review]
patch 2: Fix 5-space indents to 4-space.

Moved this patch to the cleanup happening bug 821191.

Comment 17

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 689073 [details] [diff] [review]
patch 4: Catch when we delete the currently bound FB out from under ourselves.

This patch is no longer necessary with the fix from bug 821193.

Comment 18

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

Are you going to land this?