Cache effect and selection masks
ClosedPublic

Authored by poke1024 on Oct 1 2017, 6:37 PM.

Details

Reviewers
dkazakov
Group Reviewers
Krita
Summary

Effect masks are calculated every time an update job is added, and the calculation is relatively expensive:

The 2.9% of effectMasks seen above is actually only one call. There are five calls in total, i.e. the call above is only 20% of all calls to that function, as can be seen here:

With this PR effect masks are precalculated each time the layer's children get changes (adding or removing of nodes), and each time any change (in terms of KisBaseNode::baseNodeChangedCallback) in a mask node happens that lies beneath a layer node. As far as I understand, this should cover all cases (adding, removing, visibility, activeness) that are checked for in the construction of the effects masks list. I lack knowledge of Krita though to really test this.

The caching of selection masks was added as it seems natural to do it in the same way (there was no profiling evidence).

Though this change adds brittleness due to the notification system, it removes an expensive call from the inner dab loops.

With this PR, the call to effectMasks vanishes from profiling:

The main caller, KisLayer::changeRect goes from 4.1% to 0.4%. As this is only 20% of all calls, we can expect a performance benefit of up to (4.1-0.4)*5 = 18.5%.

Diff Detail

Repository
R37 Krita
Lint
Lint Skipped
Unit
Unit Tests Skipped
poke1024 created this revision.Oct 1 2017, 6:37 PM
dkazakov requested changes to this revision.Oct 3 2017, 11:26 AM
dkazakov added a subscriber: dkazakov.

Hi, @poke1024!

The idea of the change sounds useful. Thow there is still a few problems:

  1. Krita crashes as soon as one tries to create a global selection:
    1. Open Krita
    2. Activate select-rectangular tool
    3. Draw something

Here is a backtrace:

(gdb) bt
#0  KisSelection::projection (this=0x0) at /home/devel5/kde-src/krita2/libs/image/kis_selection.cc:193
#1  0x00007ffff35b4969 in KisSelection::pixelSelection() const () at /home/devel5/kde-src/krita2/libs/image/kis_selection.cc:194
#2  0x00007ffff775e1ed in KisSelectionToolHelper::ApplyToPixelSelection::paint (this=0x1822ab10) at /home/devel5/kde-src/krita2/libs/ui/tool/kis_selection_tool_helper.cpp:101
#3  0x00007ffff784c305 in KisTransactionBasedCommand::redo (this=0x1822ab10) at /home/devel5/kde-src/krita2/libs/ui/kis_transaction_based_command.cpp:34
#4  0x00007ffff351c8ea in KisStrokeStrategyUndoCommandBased::doStrokeCallback (this=0x181ba8e0, data=<optimized out>) at /home/devel5/kde-src/krita2/libs/image/kis_stroke_strategy_undo_command_based.cpp:118
#5  0x00007ffff3675e0a in KisStrokeJob::run (this=<optimized out>) at /home/devel5/kde-build/krita2/libs/image/../../../../kde-src/krita2/libs/image/kis_stroke_job.h:44
#6  KisUpdateJobItem::run (this=0x182b7ad0) at /home/devel5/kde-build/krita2/libs/image/../../../../kde-src/krita2/libs/image/kis_update_job_item.h:84
#7  0x00007ffff5865793 in ?? () from /home/devel5/kde-install/krita2-deps/lib/libQt5Core.so.5
#8  0x00007ffff5869398 in ?? () from /home/devel5/kde-install/krita2-deps/lib/libQt5Core.so.5
#9  0x00007ffff00ac6ba in start_thread (arg=0x7fff9ceec700) at pthread_create.c:333
#10 0x00007ffff4f6082d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109

As far as I can guess, the following happens: selection tool helper asks KisView about the active selection, the latter one asks active node about it. Active node should check if it has a local selection, and if not (in this case not), ask KisImage about the global selection (which is technically a child of the root node). Somewhere is this chain, a null pointer is returned.

  1. From API point of view I would recommend a few changes as well:
    1. Rename childNodesChanged() into childNodeChanged(), and add a parameter with a changed node to it: e.g. KisNode::childNodeChanged(KisNodeSP changedChildNode)
    2. Then in KisLayer::childNodeChanged() check if the changed node is really a mask and only in that case call notifyChildMaskChanged()

These changes would make the code clearer and ease debugging a lot (notifyChildMaskChanged will not be called every time the used switches visibility of a child layer) :)

This revision now requires changes to proceed.Oct 3 2017, 11:26 AM
poke1024 updated this revision to Diff 20417.Oct 7 2017, 6:28 AM

Several renames as suggested by @dkazakov.

Fixed the selection crash. The problem was that properties (like active or visible) were not notified and the cached state was not aware of a selection getting active (so it returned no selection, even though there was one, which then produced a null ptr). So I added KisBaseNode::setNodeProperty to change properties and made the properties getter const. In terms of notification, I wondered whether to add a KisBaseNode::notifyPropertyChanged, but then I saw that the docs for baseNodeChangedCallback actually already state:

This callback is called when some meta state of the base node
     * that can be interesting to the UI has changed. E.g. visibility,
     * lockness, opacity, compositeOp and etc.

So, it's actually a bug that baseNodeChangedCallback is not called on property changes on the current implementation. I changed that accordingly.

I tested this with selections and also by adding filter masks and turning on and off their visibility to see if the change gets propagated. Everything I tested seems to work. No crashes.

rempt added a subscriber: rempt.Oct 9 2017, 9:19 AM

I don't get crashes either.

dkazakov accepted this revision.Oct 9 2017, 10:04 AM

Hi, @poke1024!

The patch works perfectly fine now. Please add this double-signal-emission fix before pushing the patch to master:

1diff --git a/libs/image/kis_base_node.cpp b/libs/image/kis_base_node.cpp
2index 99ef468..8e81091 100644
3--- a/libs/image/kis_base_node.cpp
4+++ b/libs/image/kis_base_node.cpp
5@@ -134,7 +134,6 @@ void KisBaseNode::setOpacity(quint8 val)
6
7 setNodeProperty("opacity", val);
8
9- baseNodeChangedCallback();
10 baseNodeInvalidateAllFramesCallback();
11 }
12
13diff --git a/libs/image/kis_paint_layer.cc b/libs/image/kis_paint_layer.cc
14index c691182..cebcd07 100644
15--- a/libs/image/kis_paint_layer.cc
16+++ b/libs/image/kis_paint_layer.cc
17@@ -306,13 +306,11 @@ void KisPaintLayer::setOnionSkinEnabled(bool state)
18 m_d->onionSkinConnection.clear();
19 }
20
21- setNodeProperty("onionskin", state);
22-
23 if (m_d->contentChannel) {
24 m_d->contentChannel->setOnionSkinsEnabled(state);
25 }
26
27- baseNodeChangedCallback();
28+ setNodeProperty("onionskin", state);
29 }
30
31 void KisPaintLayer::slotExternalUpdateOnionSkins()

This revision is now accepted and ready to land.Oct 9 2017, 10:04 AM
poke1024 closed this revision.Oct 18 2017, 5:58 PM