[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-kimageshop
Subject: Re: Tile data management changed in Krita
From: Dmitry Kazakov <dimula73 () gmail ! com>
Date: 2016-03-22 6:54:47
Message-ID: CAEkBSfULE8ByjrUKp3p1Nv4rTAb5xH+65RJYf-v0n1kwGtgzJA () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
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
[Attachment #5 (text/html)]
<div dir="ltr">Hi, Fazekas!<br><div><div class="gmail_extra"><br><div \
class="gmail_quote"><br><blockquote class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF" \
text="#000000"><div>
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.<br></div></div></blockquote><div><br></div><div> \
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px \
solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><div> \
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.<br></div></div></blockquote><div><br></div><div>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 <b>at least theoretical</b> 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.<br></div><div><br></div><div></div><blockquote class="gmail_quote" \
style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><div>
I made some tests to see how the different threads are calling
these functions and it happens very often in an asynchronous \
way.<br></div></div></blockquote><div><br></div><div>Yes, this part is called from \
multiple threads all the time.<br></div><div><br> </div><blockquote \
class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><div>
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.<br></div></div></blockquote><div><br></div><div>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.<br><br> \
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px \
solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><div> \
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?<br></div></div></blockquote><div><br></div><div>You should pass \
-DBUILD_TESTING=ON to a cmake command.<br></div><br></div><br>-- <br><div \
class="gmail_signature">Dmitry Kazakov</div> </div></div></div>
[Attachment #6 (text/plain)]
_______________________________________________
Krita mailing list
kimageshop@kde.org
https://mail.kde.org/mailman/listinfo/kimageshop
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic