Krita shared pointers usage

[Dmitry Kazakov]

Hi, Stefano!

I agree with most of your concerns, though I feel a bit of skeptical about
some of the points, especially in the solution part.


> 1) They lead to subtle and hard to resolve memory leaks.
>


Well, yes. This is the most troublesome part. And yes, we should solve this
problem, because this is one of the reasons why KisTileDataPool sometimes
cannot free the memory chunks on closing the image. Though I feel that the
solution should be a bit different, not raw pointers, but weak shared
pointers.

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

Right now you are most probably talking about our dockers. The solution
here is either to try to fix the design of the part that handles UI and
dockers or use weak shared pointers. *But* here we need shared pointers in
terms if Qt, not Krita.

   | 3) They introduce hard to solve crashes.

This is rather rare. Most of the time the code that owns a dangling shared
pointer it runs "somehow" and at least doesn't crash :)

   |  4) They add distributed overhead that is hard to pinpoint.

Well,  to tell you the truth I never saw shared pointers being a problem in
the dumps of Valgrind and VTune. And what is more, we use shared pointers
for storing the tiles inside a tile engine and they don't seem to appear in
dums as well. So even if it creates much overhead, then it is really-really
distributed somewhere in the memory bandwidth, that hides in all the logs.

What i'm proposing:
> ...
>  In general though the shared pointers, wherever they are, shouldn't be
> passed down the stack, but up.

Well, I cannot agree with it. Passing a raw pointer down the stack and
acquiring it somewhere below will make the code crashy and really hard to
understand why. Who would guarantee how the passed object will be used
inside the function? Will it be stored or not? How the caller has allocate
and stored the pointer? Does he own it or not? There might be really funny
situations, like:

struct SomeTempStruct {
    SomeTempStruct(KisNode *node) : m_node(node) {}
    KisNodeSP m_node;

    void bar() { /* do something */  }
};

void foo(KisNode* node) {
    SomeTempStruct myStruct(node);
    myStruct.bar();
}

KisNode *node = new KisNode();
foo(node);
delete node; // <-- double-delete crash here

This nice and quite expected code will crash happily :)

So the general idea is: "one should not mix raw and shared pointers even
when the counter is stored inside the object itself". Of course, "...when
possible".

>From the four shared pointers drawbacks you listed I can fully agree only
with the first one. Most of the leaks happen when the dockers or some UI
managers store a shared pointer to the painting object. This can be fixed
by obliging the UI not to store shared pointers, but store weak shared
pointers instead. Of course, when possible. 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). Though for catching design bugs (in form of crashes) our weak
shared pointers might also be used.

So as a conclusion for my long mail I would suggest to consider the
following:

1) Use weak shared pointers in UI dockers or elements.
2) Upgrade weak shared pointer to real pointer before use in UI (?)
3) Don't use manual deletion of Private classes. Use
QScopedPointer<Private> instead. This is the most popular cause of memory
leaks and dangling shared pointer in Krita, btw. It also guarantees the
"c++ rule of three" is satisfied properly (it disables default copy-ctor by
default).



-- 
Dmitry Kazakov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kimageshop/attachments/20141220/cda10a92/attachment.html>