Tile data management changed in Krita

[Fazekas László]

Hello Dmitry,

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.

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

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

Fazek

2016-03-21 13:10 keltezéssel, Dmitry Kazakov írta:
> Hi, Fazek!
>
> Your patch is really dangerous, especially after we announced the alpha
> version. The thread-safe access to the tile data object is guaranteed by
> the wrapper object KisTile. It puts ref() and deref() calls every tile the
> tile data is accessed. The same applies to the swapper and pooler threads.
> If you see a problem in thread safety, please describe it here or write a
> test case. We should also check how the performance is affected by this
> change. I think the best option now is ti revert this patch and put to the
> review board with th explanation of the problem it fixes.
>
> On Mon, Mar 21, 2016 at 2:50 PM, Boudewijn Rempt <boud at valdyas.org> wrote:
>
>> Hi Fazek,
>>
>> This is quite dangerous... It's typically the sort of commit that should
>> go through review and testing first because it's the core-most part of
>> Krita. I also get a build failure now, so I'm guessing you're not building
>> the unittests?
>>
>> Scanning dependencies of target KisLowMemoryTests
>> [ 50%] Building CXX object
>> libs/image/tiles3/tests/CMakeFiles/KisLowMemoryTests.dir/kis_low_memory_tests.cpp.o
>> /home/boud/dev/krita/libs/image/tiles3/tests/kis_low_memory_tests.cpp: In
>> member function ‘void KisLowMemoryTests::hangingTilesTest()’:
>> /home/boud/dev/krita/libs/image/tiles3/tests/kis_low_memory_tests.cpp:167:52:
>> error: cannot convert ‘KisTileDataSP {aka QSharedPointer<KisTileData>}’ to
>> ‘KisTileData*’ in initialization
>>       KisTileData *weirdTileData = dstTile->tileData();
>>                                                      ^
>> /home/boud/dev/krita/libs/image/tiles3/tests/kis_low_memory_tests.cpp:180:52:
>> error: cannot convert ‘KisTileDataSP {aka QSharedPointer<KisTileData>}’ to
>> ‘KisTileData*’ in initialization
>>       KisTileData *cowedTileData = dstTile->tileData();
>>                                                      ^
>> [ 50%] Linking CXX executable KisPainterTest
>> libs/image/tiles3/tests/CMakeFiles/KisLowMemoryTests.dir/build.make:62:
>> recipe for target
>> 'libs/image/tiles3/tests/CMakeFiles/KisLowMemoryTests.dir/kis_low_memory_tests.cpp.o'
>> failed
>> make[2]: ***
>> [libs/image/tiles3/tests/CMakeFiles/KisLowMemoryTests.dir/kis_low_memory_tests.cpp.o]
>> Error 1
>> CMakeFiles/Makefile2:17296: recipe for target
>> 'libs/image/tiles3/tests/CMakeFiles/KisLowMemoryTests.dir/all' failed
>> make[1]: ***
>> [libs/image/tiles3/tests/CMakeFiles/KisLowMemoryTests.dir/all] Error 2
>>
>>
>>
>> On Mon, 21 Mar 2016, Fazekas László wrote:
>>
>> Hello all,
>>> I just modified the internal logic of the tile data management in Krita
>>> (T1835 in phabricator). If you encounter any strange behaviour related to
>>> tiles please tell me. Specially if the program crashes or complains about
>>> remaining tiles when exiting.
>>>
>>> Thanks,
>>> Fazek
>>>
>>> _______________________________________________
>>> Krita mailing list
>>> kimageshop at kde.org
>>> https://mail.kde.org/mailman/listinfo/kimageshop
>>>
>>>
>> --
>> Boudewijn Rempt | http://www.krita.org, http://www.valdyas.org
>> _______________________________________________
>> Krita mailing list
>> kimageshop at kde.org
>> https://mail.kde.org/mailman/listinfo/kimageshop
>>
>>
>
>
>
> _______________________________________________
> Krita mailing list
> kimageshop at kde.org
> https://mail.kde.org/mailman/listinfo/kimageshop

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kimageshop/attachments/20160321/f458c73f/attachment-0001.html>