Tile data management changed in Krita

[Dmitry Kazakov]

Hi, Fazekas!


I know it's dangerous. In my opinion Krita is still very unstable so I can
> totally imagine this bug happens sometimes. There are just too many crashes.
>



> For the ref() and deref() calls, I still think it's not a safe way to
> allocate and delete things. Because there is an unprotected time gap
> between accessing and protecting the tile. You can build extra protection
> around this, but multi threading is very tricky sometimes. I know these
> calls must be highly regulated by the lockings, but I'm also sure the
> deref() call is not protected - it cant be - by the lock I tried to avoid
> in my second commit.
>

Well, this code is here for almost five years now and there was not a
single backtrace pointing to this place. Could you please provide *at least
theoretical* sequence of multithreaded calls that could cause a problem
here? Most of the acquire() and ref() calls happen when at least one object
(mostly (*this) of a KisTile) still owns at least one ref+userCount to the
KisTileData. Or (*this) of KisMementoItem holds the ref counter. I know
that this is more a C-way of coding, but take care, that code is called
million times per second when working with 10k images. So calling ref(),
deref() on every function call is not a go. Memory barrier is not a cheap
operation, so we should avoid it if possible.

I made some tests to see how the different threads are calling these
> functions and it happens very often in an asynchronous way.
>

Yes, this part is called from multiple threads all the time.



> Perhaps I can make a test to force this bug for you, but it's not easy,
> since it would happen only if the threads are synchronized exactly right.
> And my brain hurts if I'm trying to imagine what happening in the different
> threads simultanously. So for now, I used the fact that a solution without
> time gaps is surely better.
>

I cannot understand what "gaps" you mean. Please elaborate a bit, then we
will be able to find a solution if needed. Doing a test might be a bit
difficult, but if you decide to do it, you can use
KisTiledDataManagerTest::stressTest() for a base.



> I noticed the test directory, but I don't know how to include the
> unittests for building so I left the test part untouched. But I'd like to
> make the test codes too, even for my csv plugin so how can I build the
> unittests actually?
>

You should pass -DBUILD_TESTING=ON to a cmake command.


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