On Tue, Oct 12, 2010 at 9:47 AM, Chris Marrin <cmar...@apple.com> wrote:
> > On Oct 11, 2010, at 5:15 PM, Maciej Stachowiak wrote: > > > > > On Oct 11, 2010, at 4:03 PM, Chris Marrin wrote: > > > >> > >> On Oct 11, 2010, at 3:35 PM, James Robinson wrote: > >> > >>> On Mon, Oct 11, 2010 at 3:15 PM, Chris Marrin <cmar...@apple.com> > wrote: > >>> > >>> For accelerated 2D rendering we created a class called DrawingBuffer. > This encapsulates the accelerated drawing surface (usually in the GPU) and > the compositing layer used to display that surface on the page. The drawing > surface (which is called a Framebuffer Object or FBO) is allocated by > GraphicsContext3D, so DrawingBuffer needs a reference to that. Currently > this is a weak reference. DrawingBuffer has a ::create() method and you pass > the GraphicsContext3D to that method. > >>> > >>> If you destroy the GraphicsContext3D, DrawingBuffer has a stale > pointer. If you were to try to destroy the DrawingBuffer it would attempt to > use that pointer (to destroy its FBO) and crash or worse. Currently we have > an implicit policy that you should never destroy a GraphicsContext3D before > its DrawingBuffers are all destroyed. That works fine in the current use > case, CanvasRenderingContext2D. And we can follow that same policy when in > the future when we use DrawingBuffer in WebGLRenderingContext. > >>> > >>> My concern is that this sort of implicit policy can lead to errors in > the future when other potential clients of these classes use them. So I > posted https://bugs.webkit.org/show_bug.cgi?id=47501. In that patch I move > the creation of DrawingBuffer to the GraphicsContext3D and keep back > pointers to all the DrawingBuffers allocated so they can be cleaned up when > GraphicsContext3D is destroyed. In talking to James R. he's concerned this > adds unnecessary complexity and would rather stick with the implicit policy. > >>> > >>> So is this something I should be concerned about, or is an implicit > policy sufficient in this case? > >>> > >>> Before somebody suggests it, I think Chris and I are in agreement that > neither GraphicsContext3D nor DrawingBuffer should be RefCounted. They both > have clear single-ownership semantics. > >> > >> True, although Simon and I just chatted and he pointed out that > Refcounting both classes would solve this problem. The fact that > GraphicsContext3D wouldn't need a back pointer to DrawingBuffer means we > wouldn't have any circular references. I don't think the overhead of > refcounting is an issue here, so maybe that would be a simpler solution? > > > > I think having two independent objects that must be deleted in a specific > order, or else you crash, is a hazardous design. APIs (even internal APIs) > are better when they do not have a way to be "used wrong". So, I think this > should be changed one way or the other. > > > > I have to say that to my taste, refcounting seems like a cleaner solution > than ad-hoc weak pointers. I'm skeptical of the claim that refcounting is > not good for a heavyweight object. If there's really a time when special > resources (VRAM, etc) need to be freed at a known point in program code, > then it may be better to have an explicit "close" type function instead of > counting on the destructor. On the other hand, this might end up being > roughly equivalent to the "clear backpointers" solution, but moves the > complexity of being in a possibly-invalid state from DrawingBuffer to > GraphicsContext3D. > > > > Either way, I am pretty confident that a design where objects must be > destroyed in a specific order is not the best choice. > > So it seems like we have two choices: 1) my current patch, which uses > backpointers to manage the lifetime of the weak pointers, or 2) refcounting. > My current approach has the advantage that the resources are cleared as soon > as the DrawingBuffer is destroyed. But it is more complex and therefore more > error prone. I think that complexity is manageable so that would be my > preferred implementation. But refcounting is simpler and my current patch > has a clear() method on DrawingBuffer which gets rid of all the resources. I > could leave that method and change to a refcounted model, so the author can > control when the resources are destroyed. > Another option would be to generalize the WeakPtr<T> implementation from that patch into a generic class and use that. Then that logic could be implemented, reviewed and tested independently from the graphics code. I know that Maciej has expressed concern about this pattern in the past due to the runtime cost it imposes. Ref counting is a fairly blunt instrument but it would fit in best with the rest of the codepath. - James > > What do you think, James? > > ----- > ~Chris > cmar...@apple.com > > > > >
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
