GLContext should delete its outstanding objects when destroyed

NEW
Unassigned

Status

()

enhancement
P5
normal
7 years ago
2 days ago

People

(Reporter: jgilbert, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

7 years ago
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.
(Reporter)

Comment 1

7 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #680916 - Flags: review?(bjacob)
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>.
Attachment #680916 - Flags: review?(bjacob) → review-
(Reporter)

Comment 4

7 years ago
(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.
(Reporter)

Updated

7 years ago
Blocks: 812328
(Reporter)

Comment 5

7 years ago
Attachment #680916 - Attachment is obsolete: true
Attachment #686786 - Flags: review?(bjacob)
(Reporter)

Comment 7

7 years ago
No other changes here.
Attachment #686789 - Attachment is obsolete: true
Attachment #686789 - Flags: review?(matt.woodrow)
Attachment #686789 - Flags: review?(bjacob)
Attachment #686799 - Flags: review?(bjacob)
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?
Attachment #686786 - Flags: review?(bjacob) → review+
Attachment #686799 - Flags: review?(bjacob) → review+
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.
Attachment #686801 - Flags: review?(matt.woodrow) → review+
(Reporter)

Comment 11

7 years ago
(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)
(Reporter)

Comment 12

7 years ago
(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.
(Reporter)

Comment 14

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

Comment 15

7 years ago
Nope, still has Linux M1 errors. I think I know what it is, though.
Attachment #689073 - Flags: review?(bjacob) → review+
(Reporter)

Comment 16

6 years ago
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.
Attachment #686799 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Depends on: 821193
(Reporter)

Comment 17

6 years ago
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.
Attachment #689073 - Attachment is obsolete: true
Are you going to land this?
(Reporter)

Updated

2 days ago
Assignee: jgilbert → nobody
Type: defect → enhancement
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.