Krita shared pointers usage

[Cyrille Berger]

On Monday 15 December 2014 01:00:08 Stefano Bonicatti wrote:
> 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.

Have you tried to use the KisMemoryLeakTracker? It can help to find where 
objects are referenced.

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

It is always a cost/benefit question. What you describe *can* happen, but I 
don't remember an actual case of it ever happening (in Krita or elsewhere). 
While I have seen dozen of crashes when trying to access objects that had been 
deleted. And also, from the end-user perspective, it is probably better to 
have some strange behavior, rather than a crash that lead to loss of work.

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

If you are lucky. But especially with modern libc, when you do a delete, the 
memory stay in the application and is reused and affected to an other part of 
the application, so instead of a crash you get weird behavior and a crash in 
an unrelated part of the application. So yes, if the application was crashing 
right away, I would agree with your point.

This is something that the weak pointers actually do, they don't keep the 
reference and assert if used after the deletion of the pointer.

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

The good news is that atomic operations are extremely cheap, they are usually 
contained within a single cycle (unless there is a cache miss, but I would 
suspect that during loading, the pointer to the root layer stay in the cache), 
I would say that the average CPU nowdays is 2Ghz, that is 2 billions cycles, 
52k cycles is 2.6e-2 ms. That is why Dmitry don't see them in his valgrind 
traces :)

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

That is indeed how it should be. And weak shared pointer should be used 
otherwise, but the weak shared pointer were introduced much later and that is 
why their use is not that wide spread.

-- 
Cyrille Berger Skott