Krita shared pointers usage

[Stefano Bonicatti]

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.
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.
I would like to talk about them in what, I hope, is a positive critic.

My concern with it is their overusage around the code, in places where they
shouldn't be needed.
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".
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.

To explain better why i'm concerned:

1) They lead to subtle and hard to resolve memory leaks.
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.
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.

2) They lead to hard to debug malfunctions of the code.
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.

3) They introduce hard to solve crashes.
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.
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.
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.
In this case i think it's highly better to crash early and in development
that later have leaks and crashes in stable.

4) They add distributed overhead that is hard to pinpoint.
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.
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).
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.

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".
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.

What i'm proposing:

Shared pointers should be used as class fields, to acquire just allocated
memory or to keep alive objects used by threads.

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.
In general though the shared pointers, wherever they are, shouldn't be
passed down the stack, but up.
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).
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).
This is because, following the second point, we already have a shared
pointer
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.

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.
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).

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.
What do you think?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kimageshop/attachments/20141215/36b0d90e/attachment.html>