| dkazakov |
| Krita |
Reduces contention on the mutex in KisCanvasUpdatesCompressor by not getting updates one by one but in a batch (without this, there will be a constant list of 20 to 30 items in the list during drawing).
Switches from QList to class instance scoped QVectors to make sure no dynamic allocations happen here.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
(I would consider using move semantics to be more elegant, and just rely on return value optimization instead of passing a reference, but it's more of a personal taste... Though return values are usually clearer than passing by reference.)
In general, the patch is fine. There are only a few small issues that can be easily fixed. Please fix them and push the patch.
I'll join the flameful dispute about returning a vector from KisCanvasUpdatesCompressor::takeUpdateInfo(). I would recommend just return const KisUpdateInfoList object. Qt's implicit sharing feature will do all the needed optimizations automatically, and adding 'const' to the object will forbid detaching. Passing '&list' is basically the same as NRVO for a copy of the internal QVector.
| libs/ui/canvas/kis_canvas2.cpp | ||
|---|---|---|
| 120 | Please rename it into something like 'updateInfosBuffer' so we would know that it doesn't store anything in between the updates | |
| libs/ui/canvas/kis_canvas_updates_compressor.cpp | ||
| 40 | This construct is dangerous. You are modifying a vector while holding a pointer to its data buffer. If ever m_updatesList.takeLast() reallocates the array, then the iterator will become invalid. The iterator is just a raw pointer. | |
| 63 | If you want to keep that 'swap'-optimization, please change this line to an assert: KIS_SAFE_ASSERT_RECOVER (list.isEmpty()) { list.clear(); } The buffer should in 'always-empty' state when not used. That will help us to avoid debugging time in the future, when the infoList object will be accidentally reused. | |
| libs/ui/canvas/kis_canvas2.cpp | ||
|---|---|---|
| 120 | Renamed. | |
| libs/ui/canvas/kis_canvas_updates_compressor.cpp | ||
| 40 | You're right. I assumed that takeLast will only shrink the array and thus never reallocate, but it's documented nowhere. I switched to QList again to get around the linear time erase and thus reverted the change here altogether. QVector is not worth the hassle here. | |
| 63 | This I don't understand. Usually list will not be empty. Upon calling this function, list will be the list updateInfosBuffer in KisCanvas2. The swap below then swaps the list in KisCanvas2 with the one accumulated by KisCanvasUpdatesCompressor. In turn, KisCanvasUpdatesCompressor gets the old list from KisCanvas2. Since that old list will usually not be empty, it needs to be cleared before the swap. Or would the following be clearer? QMutexLocker l(&m_mutex); m_updatesList.swap(list); m_updatesList.clear(); I just like swap because it guarantees not doing any reallocations. | |
| libs/ui/canvas/kis_canvas_updates_compressor.h | ||
| 28 | I undid the whole QVector thing after an observation by @dkazakov with a flawed erase. I moved type typedef out from KisCanvasUpdatesCompressor, so it's kind of public now. | |
| 38 | Fixed. | |
Hi @dkazakov, I didn't add the assert in KisCanvasUpdatesCompressor::takeUpdateInfo because I don't think I understand it. Can you take another look?