On Oct 12, 2010, at 12:44 PM, James Robinson wrote: > 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.
Weak pointers are both more complicated than refcounting and introduce a comparable or possibly even greater level of runtime cost. So if there's ever a problem that can be solved either way, I would tend to prefer refcounting. Regards, Maciej
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
