Closed Bug 410392 Opened 12 years ago Closed 12 years ago

Stack-allocate gfxContext (and friends) in /widget/

Categories

(Core :: Widget, defect, P3)

defect

Tracking

()

RESOLVED WONTFIX

People

(Reporter: alfredkayser, Assigned: alfredkayser)

Details

(Keywords: memory-footprint, perf)

Attachments

(1 file, 1 obsolete file)

Spinoff from bug 383166
See attachment 2 [details] [diff] [review] for the widget part. Note however this needs some more testing and research, as in some cases the stack-allocated object is then passed on. Not sure if that is completely safe.
Status: NEW → ASSIGNED
Version: unspecified → Trunk
OK, I applied attachment 294250 [details] [diff] [review] from bug 383166 and rebuilt SeaMonkey debug on OS/2. It crashes with a double free exception and a call stack passing through nsWindow::OnPaint(), nsCOMPtr<nsIRenderingContext>::~nsCOMPtr(), ..., and gfxContext::Release() already on startup. As I modeled the code in OnPaint() after the respective Windows painting code, there could be a similar problem there... When backing out the OS/2 part of the patch it seems to run, even canvases display correctly with the canvas part in.
That's why I didn't want to check that part in yet.
Found the issue: when passing a pointer to stack-allocated nsISupports objects to someone doing ref-counting, makes the ref-counting code then tries to delete the stack-allocated object. Simply do NS_ADDREF to prevent deallocation of stack objects
To be able to stack-allocate gfxImageSurface in Windows/nsWindow.cpp some code reshuffling is needed.
Attachment #295135 - Flags: review?
Attachment #295135 - Flags: review? → review?(pavlov)
I really do'nt like all these manual addrefs.  There has to be a better way to do this.
Possibly, if someone provides me some clue here. The point is that these 'objects' are not allocated as 'object' and should never be released, so we need to make sure that for these 'stack based instances', the refcount never reached zero. NS_ADDREF seems to be the most simple solution here. 
Sounds evil to stack allocate refcounted objects.
Would it be possible to make those non-refcounted, or have
AddRef/Release No-Ops (and document that) - similar to nsIFrame objects?
That would mean that we must create special classes for these (gfxContextStack or such?). Note that the mozilla codebase is trying to move from reference counting to garbage collection where this is all no longer relevant.
(In reply to comment #9)
> Note that the mozilla codebase is trying to move from reference
> counting to garbage collection where this is all no longer relevant.
That is moz2. I assume this bug is something to be fixed in 1.9.
And stack based gc-able-objects in a gc-system wouldn't work well either.

(But I'm not a gfx peer or anything so if stuart accepts the approach, I won't
 complain more :) )
I still think that using NS_ADDREF is a simple solution to make some objects stack based. For Moz2 a better solution is still required.

A total different approach is to make 'gfxContext' a member of the renderingContext where it is used, instead of allocating it separately. (RC's are the only users of refcounted gfxContext).

Similar would be to combine gfxImageSurface and gfxContext into a 'gfxImageContext', to prevent separate allocation of these.

But this is all way more complex than just adding a few NS_ADDREF's to the stack allocated objects (which only say: This function has a strong reference to this object because it is stack allocated).
(In reply to comment #12)
> I still think that using NS_ADDREF is a simple solution to make some objects
> stack based.
That works only if it can be ensured that no one holds a pointer to the object
after the block, in which the stack object is declared, is cleared.
If that it the case with gfxXXX objects, fine, but must be, IMO, also documented
somewhere that one must not hold an owning pointer to such object longer than
the lifetime of the block.
Attachment #295135 - Flags: approval1.9+
Attachment #295135 - Flags: approval1.9+ → approval1.9?
Yea - want this for b3...
Flags: blocking1.9+
Priority: -- → P1
Attachment #295135 - Flags: approval1.9?
vlad: any thoughts on a better solution here?
also, we may want to revert 383166 or apply a similar solution to it.
Attachment #295135 - Flags: review?(pavlov) → review-
We can stack-allocate gfxContext, but we cannot stack allocate any gfx*Surface.   The problem is that the reference count for gfxSurfaces is tied to the cairo surface refcount; calling AddRef/Release just does a cairo_surface_reference()/cairo_surface_release().  The actual wrapper object is stored as user data on the surface, and is deleted via a user data destructor callback, again all within the cairo surface code.  Doing it this way is pretty much required, because otherwise it's a disaster to keep track of refcounts (because we might need to wrap surfaces we get from cairo, not just ones we create ourselves).

If we call NS_ADDREF on a stack object, we'll end up addrefing the cairo surface and then never releasing it.  If we do release it in some way, we'll end up crashing, because the destructor code from the wrapper callback will try to free the wrapper.

Note that the only thing we're stack allocating here is the gfx*Surface -- we're always going to allocate the cairo_surface_*_t, because we don't control that.  If this is showing up as a problem, we can allocate all wrappers via a slab allocator.
Pav - what we want to do here?
this shouldn't block.  it won't make a huge difference, but would be nice to do if we can figure out a good way to do it.
Flags: blocking1.9+ → blocking1.9-
This patch only puts those gfxContext instances on the stack that will not be referenced by pointer.
Attachment #295135 - Attachment is obsolete: true
Attachment #304987 - Flags: review?(pavlov)
Requesting 'wanting1.9' for the smaller and safer patch.
Flags: wanted1.9.0.x?
I think this is a WONTFIX -- out of the safer bits, 3 of them are only used when dragging (and only once), and the fourth is only used when doing translucent windows under Gtk.  An extra allocation in any of these cases won't make any difference, and I don't think we should get in the habit of stack-allocating refcounted objects (at the very least, the code would be confusing to someone reading it who might not understand why it's OK to do it there but not elsewhere).
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: wanted1.9.0.x? → wanted1.9.0.x-
Resolution: --- → WONTFIX
Attachment #304987 - Flags: review?(pavlov)
You need to log in before you can comment on or make changes to this bug.