<div dir="ltr">Hi, Stefano!<br><div><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Dec 20, 2014 at 4:58 PM, Stefano Bonicatti <span dir="ltr"><<a href="mailto:smjert@gmail.com" target="_blank">smjert@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br><div dir="ltr"><span class=""></span><div><div>Don't want to go offtopic but "the developer shouldn't care" it's more fitted for a library, but library are quite hard to make, so that the developer really doesn't care how the underlying code works (and still some information is given by the doc).</div><div>So what i mean is that i "never" trust, unless it's a well tested library, the functions i call etc and check what they actually do, so that i can also use them properly in other cases.</div></div><br><div>In general i think the same as him -> <a href="http://www.stroustrup.com/C++11FAQ.html#std-shared_ptr" target="_blank">http://www.stroustrup.com/C++11FAQ.html#std-shared_ptr</a>, not only because he his who he his, but because it actually make sense what he says about making the object lifetime cycle complex when shared pointer are used so often.</div></div></blockquote><div><br></div><div><br>Of course, I cannot argue with Stroustroup. His opinion is obviously correct. But we are speaking about different level of scale and design. In this very case I believe "the developer shouldn't care" is perfectly true. Krita codebase is more than 300k lines of code, which makes it far larger than most of the libraries it uses.<br><br></div><div>Speaking truly, when I came to Krita some time ago, I had exactly the same feeling as yours: "shared pointers make design weird". But then I found quite a few 'buts':<br><br></div><div>1) Krita is really large, so the interfaces should be unified. E.g. the function should be callable by both owner of the object and not-owner. With raw pointers it won't work as you can see in example I showed.<br><br></div><div>2) Objects can be passed using Qt signals. Which, depending on the weather on the moon (= too many conditions to bother about), can be either directly called or passed via a global events queue.<br></div><div><br></div><div>3) For some objects, like KisNodeSP we really don't know it's lifetime. I'm serious. When the node is deleted it is put into the undo stack and stored there until the undo limit is reached. The node can also be resurrected from the undo stack when the user redo's. The developer will go crazy if he needs to transfer the ownership of every node manually.<br><br></div><div>4) Continuation of 2). Krita is a multithreaded application where some actions like updates and strokes can be running asynchronously in the background. And more that that the can interleave, that is one action can access a node while the other one will delete it. The updater system is designed in a way that even if you delete a layer while the update, the merge will continue for some time (until it recognizes the layer has been deleted) but its result will be "undefined". Instead of this "undefined" and probably unfinished merge a new merge process is initiated. This design works like CompareAndSwap operation in multithreaded primitives, but in "a bit larger" scale.<br><br></div><div>The point 3) makes the term "owner" simply undefined for some time. That is, we just cannot define *who* is the owner of a node in this moment of time. All the users share it. So the reasoning of Stroustroup just don't apply to our case, because here the usage is really shared.<br></div><br><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div></div><div>Maybe having unique_ptr like pointers and shared pointer passed as const references instead of raw pointer would be better, so that it's more explicit the fact that you should not delete it manually and it's cheaper, copy wise and with the ref/deref not happening when passed around in code that shouldn't actually prolong its life.</div><div>You would still have shared pointers around but it would be much easier for the memory leak tracker to keep track on who is really holding the memory.</div></div></blockquote><div><br></div><div>1) unique_ptr cannot be passed anywhere outside a class. Therefore we use it only for Private data objects (in a form of QScopedPointer)<br></div><div>2) Passing a const reference for a shared pointer is quite meaningless. The counter is 'mutable' so it doesn't prevent the callee to own this object. This is true for both Krita and Qt shared pointers. The speed benefit is really arguable.<br><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><span class=""><div></div><div>> <span style="font-size:13px">The idea provided by Qt's shared pointers might be very helpful here (Qt's weak shared pointer doesn't allow dereferencing the pointer until upgrade to a real shared pointer)</span></div><div><span style="font-size:13px"></span></div></span><div><span style="font-size:13px">There are a couple of problems with Krita weak pointers i'm facing though:</span></div><div><span style="font-size:13px">1) If you pass a dead weak pointer to a shared pointer the check to see if the weak pointer is valid is done in an assert only after the shared pointers tries to increase the reference count.</span></div><div>This works if the weak pointer internal pointer was set to NULL, because shared pointer ref() function checks for that, but if the weak pointer points to deleted memory.. well ref() behaviour is undefined.</div></div></blockquote><div><br></div><div>Probably, this should be fixed in KisWeakSharedPtr. Or we should implement a method KisWeakSharedPtr::toSharedPtr(). I'm open for suggestions :)<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div></div><div>2) Here i'm talking about that issue with the resource manager holding a shared pointer to the current activated node, that should be deleted when the image is deleted.</div><div>You suggested to use a weak pointer but due to how QVariant works, as of now, you can't safely store (or actually retrieve) a weak pointer inside because there's no way to check isValid() function before a copy happens (through QVariant value<>() function), and this as point 1, may access to deleted memory.</div><div>This is kind of complicated because you can put the isValid check in the copy constructor before loading the pointer into the new weak shared pointer, so that the internal pointer is set to 0 if it's not valid, but it would add the overhead of the isValid check for a specific case to every copy done.</div></div></blockquote><div><br></div><div>Can you fix it in KisWeakSharedPtr? I guess if we copy an invalid weak shared pointer, the copy should successfully happen, but the resulting pointer should also become invalid.<br><br></div><div>You can also write a unittest like this:<br></div><div><br>KisWeakSharedPtr<int> a;<br><br>{<br>    KisSharedPtr<int> b(new int(1));<br></div><div>    a = b;<br>}<br><br></div><div>QVERIFY(!a.isValid());<br><br>KisWeakSharedPtr<int> c;<br></div><div>c = a;<br>QVERIFY(!c.isValid());<br><br>KisSharedPtr<int> d;<br>d = a;<br>QVERIFY(!d.isValid());<br><br><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>This is why i think sometimes smart pointers get a bit in the way, they make you think everything is safer while.. it depends.. </div><div>For point 2 i think then it should be who actually set that node (KoCanvasResourceProvider through an event), that always through an event (image closed/deletion?) removes it (so the QVariant too), instead of relying on a weak pointer?</div></div></blockquote><div><br></div><div>Our usage of weak pointers was always limited, so their definition might be not entirely complete. We need to fix these inconsistencies, and they will work fine for us :)<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><span class=""></span><span class=""><div><div style="font-size:13px"></div><div style="font-size:13px">> 2) Upgrade weak shared pointer to real pointer before use in UI (?)<br></div></div></span><div style="font-size:13px">I agree with these, but can you elaborate more point 2 (what you mean with real pointer?).</div></div></blockquote><div><br></div><div>Just check the interface of QWeakSharedPointer. You cannot use it directly, you always need to convert it into QSharedPointer before any usage. I'm not sure we really need this in Krita or, more precisely, we can rewrite this amount of code to follow new design. But at least the general idea should be used somehow.<br><br></div><div>And a conclusion:<br><br></div><div>1) Yes, we have memory leak problems caused by shared pointers owned by UI elements<br></div><div>2) Yes, we might need spend some time on splitting ownership for UI elements. E.g. use weak shared pointers in UI and dereference them only when needed.<br></div><div>3) I cannot agree that we should mix raw pointers and shared pointers. See a part about multithreading.<br></div><div>4) We might need to fix weak shared pointers to work for our cases better :)<br></div></div><br><br clear="all"><br>-- <br><div class="gmail_signature">Dmitry Kazakov</div>
</div></div></div>