<div dir="ltr"><div style="font-size:13px">Hello everyone, i'm the guy who goes by the name Smjert on irc, and I started contributing to this beautiful project a week ago.</div><div style="font-size:13px"><div>During the last few days I had the chance to examine a good portion of the code, either because it was something related to what I was debugging or because I was just curious as to how specific features were implemented; during this time, I couldn't help but notice how shared pointers are use throughout the project.</div><div>I would like to talk about them in what, I hope, is a positive critic.</div></div><div style="font-size:13px"><br></div><div style="font-size:13px">My concern with it is their overusage around the code, in places where they shouldn't be needed.</div><div style="font-size:13px">A shared pointer should express shared ownership between two object instances, but i see them used more like a "keep it alive while this is running".</div><div style="font-size:13px">While this is part of their correct usage (or better a convenience), they should be limited in scope (thing that, especially thanks to how Krita shared pointers are written, you can, instead of standard ones), otherwise they complicates quite a lot the object lifetime cycle.</div><div style="font-size:13px"><br></div><div style="font-size:13px">To explain better why i'm concerned: </div><div style="font-size:13px"><br></div><div style="font-size:13px">1) They lead to subtle and hard to resolve memory leaks. </div><div style="font-size:13px">With a raw pointer if you have a leak you run valgrind and you get which pointer is leaking and then you are just left to see where this pointer gets deleted (or not). If it's inside a class destructor then one just need to understand why this class instance doesn't die and so on.</div><div style="font-size:13px">With a shared pointer instead you may arrive to the same destructor but since the ownership is shared, the pointer is not deleted there anymore, but somewhere else in the code. And if the shared pointer is passed around very often it is very very hard to find where things go wrong, because you have to follow every path it takes and also keep track of the number of refs and derefs to see where it is kept alive.</div><div style="font-size:13px"><br></div><div style="font-size:13px">2) They lead to hard to debug malfunctions of the code.</div><div style="font-size:13px">This is easy to explain, a class that stays alive beyond what one thought it would, could receive function calls that accesses it or other classes, making it do things it shouldn't at that point in time.</div><div style="font-size:13px"><br></div><div style="font-size:13px">3) They introduce hard to solve crashes.</div><div style="font-size:13px">This is basicly connected to the first and second point, if the object leaks it can be still accessed, and if this doesn't lead to a malfunction it may lead to a crash due to accessing parts of it (or other classes) that are already deleted.</div><div style="font-size:13px">With a raw pointer instead if you get a crash it may be because of a wrong delete or in general a wrong architecture design/object lifetime, and you can fix it because the crash warns you about it.</div><div style="font-size:13px">With a shared pointer you hide the crash while mantaining the architectural problem and then potentially adding other problems like a leak, that if then fixed may lead to the crash you were tryng to hide. So double the work.</div><div style="font-size:13px">In this case i think it's highly better to crash early and in development that later have leaks and crashes in stable.</div><div style="font-size:13px"><br></div><div style="font-size:13px">4) They add distributed overhead that is hard to pinpoint.</div><div style="font-size:13px">Due to all the copying, but especially since the atomic increase of the reference counter, they add overhead, that is difficult to analyze because it's spreaded into all the functions (when the shared pointer usage is so common) and you won't have one hotspot.<br>While i didn't tried to count, over a certain amount of time, how many refs and derefs of shared pointers there are globally, i had the occasion to count refs/derefs of a single shared pointer (the rootLayer in KisImage, one that was leaking memory).</div><div style="font-size:13px">Only by opening and closing one image i got 26k refs and 26k derefs of that single pointer, that seems to me quite a lot for such a simple operation.</div><div style="font-size:13px"><br></div><div style="font-size:13px">Now since most of this problems revolve around leaks one may say "but Krita has a memory tracker that can show the call stacks of the shared pointer creations, the ones that are still alive at the moment of the dump call".</div><div style="font-size:13px">It's true, this even helped me pinpoint where the shared pointers containing the rootLayer where held beyond the correct time, but you don't always know when to dump the references to have the least number of false positives (shared pointers that are ok to be still alive, and others that are actually going to die soon after the dump call, but that are not dead already), especially when you can have a leak that spans all the application life, then there and with excessive shared pointers usage you will have tons of false positives.</div><div style="font-size:13px"><br></div><div style="font-size:13px">What i'm proposing:</div><div style="font-size:13px"><br></div><div style="font-size:13px">Shared pointers should be used as class fields, to acquire just allocated memory or to keep alive objects used by threads.</div><div style="font-size:13px"><br></div><div style="font-size:13px">The first one should be clear, the second one means that as soon as one has a "new" of something, the pointer should be put inside a shared pointer.</div><div style="font-size:13px">In general though the shared pointers, wherever they are, shouldn't be passed down the stack, but up.</div><div style="font-size:13px">So for instance if one has some function that allocate memory.. then it is ok for it to return a shared pointer (this is passed up the stack).<br></div><div style="font-size:13px">If though an object is allocated and then passed down the stack in a code that maybe sets this shared pointer as a class field, it should be passed as raw pointer until it reaches this precise code, where a shared pointer can be created (this again thanks to the fact that the reference counter resides on the object and not on the shared pointer).</div><div style="font-size:13px">This is because, following the second point, we already have a shared pointer</div><div style="font-size:13px">holding the raw pointer, and passing this down the stack is unneeded since the first shared pointer will be kept alive until the execution goes back up the stack.</div><div style="font-size:13px"><br></div><div style="font-size:13px">For the third one, if one is really unsure about some object lifetime that is used by a thread, then it can be passed or acquired once as a shared pointer during the life of the thread.</div><div style="font-size:13px">I said once, but clearly it depends on how is the scope of that shared pointer (again if you have to pass it down it shouldn't be needed to use a shared pointer).</div><div style="font-size:13px"><br></div><div style="font-size:13px">i know this is a long mail, so take your time, but i'm quite sure that changing how shared pointers are used in Krita will lead to... tons of crashes! Ok joking aside, it leads to easier to understand and debug code, it will fix (or expose) hidden architecture issues that may lead to crashes, but earlier, and also increase the program efficiency.</div><div style="font-size:13px">What do you think?</div></div>