Krita shared pointers usage

Sat Dec 20 18:48:24 UTC 2014

Hi, Stefano!

On Sat, Dec 20, 2014 at 4:58 PM, Stefano Bonicatti <smjert at gmail.com> wrote:

>
> 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).
> 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.
>
> In general i think the same as him ->
> http://www.stroustrup.com/C++11FAQ.html#std-shared_ptr, 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.
>

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.

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':

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.

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.

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.

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.

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.

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

1) unique_ptr cannot be passed anywhere outside a class. Therefore we use
it only for Private data objects (in a form of QScopedPointer)
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.

> > 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)
> There are a couple of problems with Krita weak pointers i'm facing though:
> 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.
> 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.
>

Probably, this should be fixed in KisWeakSharedPtr. Or we should implement
a method KisWeakSharedPtr::toSharedPtr(). I'm open for suggestions :)

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

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.

You can also write a unittest like this:

KisWeakSharedPtr<int> a;

{
    KisSharedPtr<int> b(new int(1));
    a = b;
}

QVERIFY(!a.isValid());

KisWeakSharedPtr<int> c;
c = a;
QVERIFY(!c.isValid());

KisSharedPtr<int> d;
d = a;
QVERIFY(!d.isValid());

> This is why i think sometimes smart pointers get a bit in the way, they
> make you think everything is safer while.. it depends..
> 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?
>

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

> > 2) Upgrade weak shared pointer to real pointer before use in UI (?)
> I agree with these, but can you elaborate more point 2 (what you mean with
> real pointer?).
>

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.

And a conclusion:

1) Yes, we have memory leak problems caused by shared pointers owned by UI
elements
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.
3) I cannot agree that we should mix raw pointers and shared pointers. See
a part about multithreading.
4) We might need to fix weak shared pointers to work for our cases better :)

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