KOColor constructors in dab constructors
ClosedPublic

Authored by poke1024 on Sep 20 2017, 7:17 PM.

Details

Summary

This might be classic micro-optimizing, but maybe it's still worth pointing out.

Inside KisDabCache::fetchDab() a lot of KoColor construction and destruction takes place, which is relatively expensive due to the color space lookups and memory allocations in KoColor::KoColor; ok, this is still only about 0.3% of the runtime (see attached profiling screenshot). This patch gets rid of all of the KoColor constructions but one.

Diff Detail

Repository
R37 Krita
Lint
Lint Skipped
Unit
Unit Tests Skipped
poke1024 created this revision.Sep 20 2017, 7:17 PM
poke1024 added a reviewer: Krita.

Actually, my calculation above was wrong; it's 0.3% for one thread, but since that code ran in 7 threads in the profiler (all summing up to 100%), the impact is more like 0.3% * 7 = 2,1%.

Wouldn't it make more sense to try to optimize KoColor instead? It is being used all over the place in Krita and having to do two heap allocations/deallocations each time it one constructed/destructed would seem to be dreadful (even worse on Windows).

If you look at the profiling call trace in a bottom-up view and not limited to KisDabCache, how much time is taken inside KoColor?

May I suggest you to join the IRC channel #krita on freenode? Most of the Krita developers/contributors hang out there.

@alvinhochun Yes, good point, I'm currently headed for holidays, but I'll stop by #krita when I'm back.

And you're right: I looked a bit more at KoColor, and drawing around with Basic_circle, there are actually four different callers of KoColor::KoColor that show up in profiling:

Apple's profiling instrument doesn't seem to allow for displaying an inverse summed of impact of one method call (maybe I need to switch tools), but from what I sum up manually, the four calls above (i.e. only KoColor::KoColor) take up 4 to 5%, which is a lot of time some simple byte shifting.

I've tested doing a reimp of KoColor without dynamic memory allocations, I'll do a PR sometime in the next weeks.

dkazakov requested changes to this revision.Sep 25 2017, 1:39 PM
dkazakov added a subscriber: dkazakov.

Hi, @poke1024!

@alvinhochun was right about putting to KoColor, it is better to optimize KoColor itself.

The only trouble is that we are no longer sure if KoColorSpaceRegistry::permanentColorspace() is actually needed. As far as I remember we never delete color spaces, so that method might not be needed, though it should be checked in the code manually :(

And mind you, if you decide to do any changes in KisDabCache, please base your changes on 'kazakov/multithreaded-brushes' branch. The class is fully refactored in that branch (mostly moved into KisDabCacheBase)

This revision now requires changes to proceed.Sep 25 2017, 1:39 PM
poke1024 updated this revision to Diff 19983.Sep 27 2017, 2:19 PM

So this is the "KOColor without dynamic allocations" refactoring I came up.

Two points I'd like to point your attention to:

  • the permanent color space check in assertPermanentColorspace() now always compares pointers, whereas one of the old constructors actually compared objects (i.e. Q_ASSERT(*d->colorSpace == *KoColorSpaceRegistry::instance()->permanentColorspace(d->colorSpace));)
  • the s_prefab.colorSpace in KoColor::init must be made permanent, otherwise there will be a crash

I'm not sure I understand what permanent colors spaces really are, that's why I point out these two changes.

poke1024 updated this revision to Diff 19984.Sep 27 2017, 2:21 PM

fixes a wrong const

rempt added a subscriber: rempt.Sep 28 2017, 8:14 AM
rempt added inline comments.
krita/main.cc
157

Hm, are you completely sure that during the construction of KisApplication or the loading of libraries/plugins no KoColors are created? I'd be very surprised if that didn't happen!

libs/pigment/KoColor.h
188

Please don't rename d to m_d -- we're actually trying to standardize on plain 'd' instead of 'm_d' which we used to use in Krita only.

poke1024 updated this revision to Diff 20091.Sep 29 2017, 2:01 PM

renamed m_d to d

KoColor::init() internally creates the first KoColorSpaceRegistry instance, which needs QApplication, so it cannot happen before the QApplication constructor. I added a Q_ASSERT in the KoColor constructor to assert that it's initialized.

At the place it is now, in KisApplication::KisApplication, everything should be fine; there seem to be no KoColor constructions before the current call to KoColor::init.

poke1024 updated this revision to Diff 20092.Sep 29 2017, 2:03 PM

removed debugging code

rempt accepted this revision.Sep 29 2017, 3:00 PM

I haven't got any remarks anymore, please push :-)

A few more details (and an analysis comparable to the one in D8033):

I measured KoColor::KoColor allocation time:

QElapsedTimer timer;
const int size = 8192;
static KoColor *buf[8192];
timer.start();
for (int i = 0; i < size; i++) {
    buf[i] = new KoColor();
}
printf("mean alloc time %f\n", timer.nsecsElapsed() / (float)size);

Which gives: a mean allocation time of 7772 ns on my system.

Now, I counted the invocations of the various KoColor::KoColor constructors while drawing (each call counted as 1; total number and number of calls per):

alloc KOColor 1294336 81,9
alloc KOColor 1298432 46,5
alloc KOColor 1302528 70,6
alloc KOColor 1306624 81,9
alloc KOColor 1310720 80,3
alloc KOColor 1314816 105,0
alloc KOColor 1318912 58,5
alloc KOColor 1323008 75,9
alloc KOColor 1327104 80,3
alloc KOColor 1331200 63,0
alloc KOColor 1335296 48,2
alloc KOColor 1339392 83,6
alloc KOColor 1343488 59,4
alloc KOColor 1347584 68,3
alloc KOColor 1351680 68,3
alloc KOColor 1355776 110,7
alloc KOColor 1359872 71,9

This was achieved by calling the following count_alloc from each KoColor constructor:

QAtomicInt _total_count;
QElapsedTimer _timer;
QMutex _mutex;

inline void count_alloc() {

    int count = _total_count.fetchAndAddRelaxed(1);
    const int slice = 4096;
    if ((count % slice) == 0) {
        _mutex.lock();
        long ms = _timer.elapsed();
        _timer.restart();
        printf("alloc KOColor %d %.1f\n", count, slice / (float)ms);
        _mutex.unlock();
    }
}

RESULTS

Time spent in KoColor::KoColor under load: 7500 ns * 100 KOColor per ms = 750000 ns per ms (1000000 ns) = 75%.

Which means, during drawing, on a multicore system, 3/4 of one core is allocated doing KOColor stuff.

And this doesn't even take into account KoColor::~KoColor.

dkazakov requested changes to this revision.Oct 3 2017, 9:16 AM

With the patch applied I have a weird crash on start :(

It seems like it is somehow related to color proofing settings

(gdb) bt
#0  KoLcmsColorProofingConversionTransformation::KoLcmsColorProofingConversionTransformation (adaptationState=1, gamutWarning=0x7fffffffa128 "", conversionFlags=..., 
    proofingIntent=KoColorConversionTransformation::IntentAbsoluteColorimetric, renderingIntent=KoColorConversionTransformation::IntentPerceptual, proofingSpace=0x0, dstProfile=0x1010090, 
    dstColorSpaceType=279705, dstCs=<optimized out>, srcProfile=0x1010090, srcColorSpaceType=279705, srcCs=<optimized out>, this=0x175ac6a0)
    at /home/devel5/kde-src/krita2/plugins/color/lcms2engine/IccColorSpaceEngine.cpp:135
#1  IccColorSpaceEngine::createColorProofingTransformation (this=<optimized out>, srcColorSpace=<optimized out>, dstColorSpace=<optimized out>, proofingSpace=0x0, 
    renderingIntent=KoColorConversionTransformation::IntentPerceptual, proofingIntent=KoColorConversionTransformation::IntentAbsoluteColorimetric, conversionFlags=..., gamutWarning=0x7fffffffa128 "", 
    adaptationState=1) at /home/devel5/kde-src/krita2/plugins/color/lcms2engine/IccColorSpaceEngine.cpp:280
#2  0x00007ffff209437b in KoColorSpace::createProofingTransform (this=0x11edab0, dstColorSpace=0x11edab0, proofingSpace=0x0, renderingIntent=KoColorConversionTransformation::IntentPerceptual, 
    proofingIntent=KoColorConversionTransformation::IntentAbsoluteColorimetric, conversionFlags=..., gamutWarning=0x7fffffffa128 "", adaptationState=1)
    at /home/devel5/kde-src/krita2/libs/pigment/KoColorSpace.cpp:445
#3  0x00007ffff772cfe6 in KisTextureTileUpdateInfo::generateProofingTransform (this=<optimized out>, adaptationState=1, 
    gamutWarning=<error reading variable: access outside bounds of object referenced via synthetic pointer>, conversionFlags=..., proofingIntent=<optimized out>, renderingIntent=<optimized out>, 
    proofingSpace=0x0, dstCS=0x11edab0) at /home/devel5/kde-src/krita2/libs/ui/opengl/kis_texture_tile_update_info.h:242
#4  KisOpenGLImageTextures::updateCacheImpl (this=<optimized out>, rect=..., srcImage=..., convertColorSpace=convertColorSpace@entry=true)
    at /home/devel5/kde-src/krita2/libs/ui/opengl/kis_opengl_image_textures.cpp:348
#5  0x00007ffff772ddda in KisOpenGLImageTextures::updateCache (this=this@entry=0x14f3bd00, rect=..., srcImage=...) at /home/devel5/kde-src/krita2/libs/ui/opengl/kis_opengl_image_textures.cpp:258
#6  0x00007ffff772e12e in KisOpenGLImageTextures::initGL (this=0x14f3bd00, f=<optimized out>) at /home/devel5/kde-src/krita2/libs/ui/opengl/kis_opengl_image_textures.cpp:119
#7  0x00007ffff7725b73 in KisOpenGLCanvas2::initializeGL (this=0x14f16050) at /home/devel5/kde-src/krita2/libs/ui/opengl/kis_opengl_canvas2.cpp:250
#8  0x00007ffff648a5a8 in ?? () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#9  0x00007ffff648a5fa in QOpenGLWidget::resizeEvent(QResizeEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#10 0x00007ffff646b5be in QWidget::event(QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#11 0x00007ffff6425dcc in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#12 0x00007ffff642b306 in QApplication::notify(QObject*, QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#13 0x00007ffff7889017 in KisApplication::notify (this=<optimized out>, receiver=0x14f16050, event=0x7fffffffa710) at /home/devel5/kde-src/krita2/libs/ui/KisApplication.cpp:587
#14 0x00007ffff5a030b8 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Core.so.5
#15 0x00007ffff6463f72 in QWidgetPrivate::sendPendingMoveAndResizeEvents(bool, bool) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#16 0x00007ffff6467d13 in QWidgetPrivate::show_helper() () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#17 0x00007ffff6467c97 in QWidgetPrivate::showChildren(bool) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#18 0x00007ffff6467d2f in QWidgetPrivate::show_helper() () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#19 0x00007ffff646ab4d in QWidget::setVisible(bool) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#20 0x00007ffff6467ca8 in QWidgetPrivate::showChildren(bool) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#21 0x00007ffff6467d2f in QWidgetPrivate::show_helper() () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#22 0x00007ffff646ab4d in QWidget::setVisible(bool) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#23 0x00007ffff6467ca8 in QWidgetPrivate::showChildren(bool) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#24 0x00007ffff6467d2f in QWidgetPrivate::show_helper() () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#25 0x00007ffff646ab4d in QWidget::setVisible(bool) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#26 0x00007ffff6467ca8 in QWidgetPrivate::showChildren(bool) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#27 0x00007ffff6467d2f in QWidgetPrivate::show_helper() () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#28 0x00007ffff646ab4d in QWidget::setVisible(bool) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#29 0x00007ffff6585d5d in QMdiSubWindow::changeEvent(QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#30 0x00007ffff646af9a in QWidget::event(QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#31 0x00007ffff65866cb in QMdiSubWindow::event(QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#32 0x00007ffff6425dcc in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#33 0x00007ffff642b306 in QApplication::notify(QObject*, QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#34 0x00007ffff7889017 in KisApplication::notify (this=<optimized out>, receiver=0x14f4fbd0, event=0x7fffffffb0c0) at /home/devel5/kde-src/krita2/libs/ui/KisApplication.cpp:587
#35 0x00007ffff5a030b8 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Core.so.5
#36 0x00007ffff6467328 in QWidget::setWindowState(QFlags<Qt::WindowState>) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#37 0x00007ffff6467a91 in QWidget::showMaximized() () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#38 0x00007ffff658647c in QMdiSubWindow::eventFilter(QObject*, QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#39 0x00007ffff5a02e22 in QCoreApplicationPrivate::sendThroughObjectEventFilters(QObject*, QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Core.so.5
#40 0x00007ffff6425da5 in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#41 0x00007ffff642b306 in QApplication::notify(QObject*, QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#42 0x00007ffff7889017 in KisApplication::notify (this=<optimized out>, receiver=0x14f18a70, event=0x7fffffffb490) at /home/devel5/kde-src/krita2/libs/ui/KisApplication.cpp:587
#43 0x00007ffff5a030b8 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Core.so.5
#44 0x00007ffff6467328 in QWidget::setWindowState(QFlags<Qt::WindowState>) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#45 0x00007ffff6467a91 in QWidget::showMaximized() () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#46 0x00007ffff78bfa65 in KisMainWindow::showView (this=0x13965950, imageView=0x14f18a70) at /home/devel5/kde-src/krita2/libs/ui/KisMainWindow.cpp:593
#47 0x00007ffff78bc9ba in KisMainWindow::addView (this=this@entry=0x13965950, view=view@entry=0x14f18a70) at /home/devel5/kde-src/krita2/libs/ui/KisMainWindow.cpp:534
#48 0x00007ffff78bcaa2 in KisMainWindow::addViewAndNotifyLoadingCompleted (this=0x13965950, document=<optimized out>) at /home/devel5/kde-src/krita2/libs/ui/KisMainWindow.cpp:840
---Type <return> to continue, or q <return> to quit---
#49 0x00007ffff78d7138 in KisPart::startCustomDocument (this=<optimized out>, doc=<optimized out>) at /home/devel5/kde-src/krita2/libs/ui/KisPart.cpp:469
#50 0x00007ffff79731bc in KisPart::qt_static_metacall (_o=<optimized out>, _c=<optimized out>, _id=<optimized out>, _a=<optimized out>) at /home/devel5/kde-build/krita2/libs/ui/moc_KisPart.cpp:140
#51 0x00007ffff5a2f951 in QMetaObject::activate(QObject*, int, int, void**) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Core.so.5
#52 0x00007ffff796467f in KisOpenPane::documentSelected (this=<optimized out>, _t1=0x14ec5670) at /home/devel5/kde-build/krita2/libs/ui/moc_KisOpenPane.cpp:236
#53 0x00007ffff5a2f951 in QMetaObject::activate(QObject*, int, int, void**) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Core.so.5
#54 0x00007ffff6505cb2 in QAbstractButton::clicked(bool) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#55 0x00007ffff6505f04 in ?? () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#56 0x00007ffff65079b7 in ?? () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#57 0x00007ffff6507b34 in QAbstractButton::mouseReleaseEvent(QMouseEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#58 0x00007ffff646b128 in QWidget::event(QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#59 0x00007ffff6425dcc in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#60 0x00007ffff642bb5e in QApplication::notify(QObject*, QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#61 0x00007ffff7889017 in KisApplication::notify (this=<optimized out>, receiver=0x14e00ee0, event=0x7fffffffbdc0) at /home/devel5/kde-src/krita2/libs/ui/KisApplication.cpp:587
#62 0x00007ffff5a030b8 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Core.so.5
#63 0x00007ffff642a855 in QApplicationPrivate::sendMouseEvent(QWidget*, QMouseEvent*, QWidget*, QWidget*, QWidget**, QPointer<QWidget>&, bool) ()
   from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#64 0x00007ffff6485780 in ?? () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#65 0x00007ffff64880c3 in ?? () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#66 0x00007ffff6425dcc in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#67 0x00007ffff642b306 in QApplication::notify(QObject*, QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#68 0x00007ffff7889017 in KisApplication::notify (this=<optimized out>, receiver=0x14e4ca30, event=0x7fffffffc240) at /home/devel5/kde-src/krita2/libs/ui/KisApplication.cpp:587
#69 0x00007ffff5a030b8 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Core.so.5
#70 0x00007ffff5daf9b0 in QGuiApplicationPrivate::processMouseEvent(QWindowSystemInterfacePrivate::MouseEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Gui.so.5
#71 0x00007ffff5db1545 in QGuiApplicationPrivate::processWindowSystemEvent(QWindowSystemInterfacePrivate::WindowSystemEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Gui.so.5
#72 0x00007ffff5d8f17b in QWindowSystemInterface::sendWindowSystemEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Gui.so.5
#73 0x00007fffe9ec3236 in ?? () from /home/devel5/kde-install/krita2-deps/plugins/platforms/../../lib/libQt5XcbQpa.so.5
#74 0x00007ffff5a00e4a in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Core.so.5
#75 0x00007ffff660af2c in QDialog::exec() () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#76 0x00007ffff78bbd34 in KisMainWindow::slotFileNew (this=<optimized out>) at /home/devel5/kde-src/krita2/libs/ui/KisMainWindow.cpp:1379
#77 0x00007ffff78cbb48 in KisMainWindow::qt_static_metacall (_o=0x13965950, _c=<optimized out>, _id=<optimized out>, _a=0x7fffffffc7c0) at /home/devel5/kde-build/krita2/libs/ui/moc_KisMainWindow.cpp:290
#78 0x00007ffff5a2f951 in QMetaObject::activate(QObject*, int, int, void**) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Core.so.5
#79 0x00007ffff641c8c2 in QAction::triggered(bool) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#80 0x00007ffff641f3e0 in QAction::activate(QAction::ActionEvent) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#81 0x00007ffff658a922 in ?? () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#82 0x00007ffff6590a96 in ?? () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#83 0x00007ffff6594cc9 in QMenu::mouseReleaseEvent(QMouseEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#84 0x00007ffff646b128 in QWidget::event(QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#85 0x00007ffff659576b in QMenu::event(QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#86 0x00007ffff6425dcc in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#87 0x00007ffff642bb5e in QApplication::notify(QObject*, QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#88 0x00007ffff7889017 in KisApplication::notify (this=<optimized out>, receiver=0x14404a20, event=0x7fffffffcd70) at /home/devel5/kde-src/krita2/libs/ui/KisApplication.cpp:587
#89 0x00007ffff5a030b8 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Core.so.5
#90 0x00007ffff6594357 in ?? () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#91 0x00007ffff6594ab6 in QMenu::mouseReleaseEvent(QMouseEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#92 0x00007ffff646b128 in QWidget::event(QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#93 0x00007ffff659576b in QMenu::event(QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#94 0x00007ffff6425dcc in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#95 0x00007ffff642bb5e in QApplication::notify(QObject*, QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#96 0x00007ffff7889017 in KisApplication::notify (this=<optimized out>, receiver=0x149fe380, event=0x7fffffffd360) at /home/devel5/kde-src/krita2/libs/ui/KisApplication.cpp:587
#97 0x00007ffff5a030b8 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Core.so.5
#98 0x00007ffff642a855 in QApplicationPrivate::sendMouseEvent(QWidget*, QMouseEvent*, QWidget*, QWidget*, QWidget**, QPointer<QWidget>&, bool) ()
   from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#99 0x00007ffff6485d97 in ?? () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#100 0x00007ffff64880c3 in ?? () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#101 0x00007ffff6425dcc in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#102 0x00007ffff642b306 in QApplication::notify(QObject*, QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Widgets.so.5
#103 0x00007ffff7889017 in KisApplication::notify (this=<optimized out>, receiver=0x14a01390, event=0x7fffffffd7e0) at /home/devel5/kde-src/krita2/libs/ui/KisApplication.cpp:587
#104 0x00007ffff5a030b8 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Core.so.5
#105 0x00007ffff5daf9b0 in QGuiApplicationPrivate::processMouseEvent(QWindowSystemInterfacePrivate::MouseEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Gui.so.5
---Type <return> to continue, or q <return> to quit---
#106 0x00007ffff5db1545 in QGuiApplicationPrivate::processWindowSystemEvent(QWindowSystemInterfacePrivate::WindowSystemEvent*) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Gui.so.5
#107 0x00007ffff5d8f17b in QWindowSystemInterface::sendWindowSystemEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Gui.so.5
#108 0x00007fffe9ec3236 in ?? () from /home/devel5/kde-install/krita2-deps/plugins/platforms/../../lib/libQt5XcbQpa.so.5
#109 0x00007ffff5a00e4a in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () from /home/devel5/kde-install/krita2-deps/lib/libQt5Core.so.5
#110 0x00007ffff5a0949c in QCoreApplication::exec() () from /home/devel5/kde-install/krita2-deps/lib/libQt5Core.so.5
#111 0x00000000004053f9 in main (argc=1, argv=<optimized out>) at /home/devel5/kde-src/krita2/krita/main.cc:305
This revision now requires changes to proceed.Oct 3 2017, 9:16 AM

Hi, @poke1024!

I have three questions/issues:

  1. Is it possible to make the preinitialized s_prefab member a singleton with auto-initialization? I guess it could solve that "do not use KoColor while KisApplication init" issue. There is a Q_GLOBAL_STATIC macro for that.
  1. There is no need to optimize color space comparison to pointers-only. The first line of KoColorSpace::operator==() checks the actual addresses of the objects, so it is more safe to use the operator instead.
  1. After KoColor::Private has been moved into the header file, there is actually no reason in having the d-pointer pattern. Is this move really needed? If yes, can you just make the contents of Private be the direct members of KoColor? That would resolve quite a few boilerplate issues.
poke1024 updated this revision to Diff 20294.Oct 3 2017, 2:26 PM

This fixes that crash inside KoLcmsColorProofingConversionTransformation::KoLcmsColorProofingConversionTransformation Dmitry observed.

I'm a bit ruffled that I didn't see this one which means I was pretty confused during testing and never actually opened a canvas with this version of the code.

The reason for the crash was this: color space proofing (whatever that is) uses certain KoColorSpaces, which are registered in a KoColorSpaceRegistry. This registry is initially filled by the LcmsEnginePlugin constructor. Now, this plugin loads the color spaces resources using KoResourcePaths.

When KoColor::init runs, it fetches the first KoColorSpace and initializes the whole color space architecture, including LcmsEnginePlugin, which then fetches its resources. But: KoColor::init ran too early, as KisApplication had not yet set up the resources paths. So, what happened was that a singleton of KoColorSpaceRegistrywas created, that did not have all color spaces due to lacking paths. These lacking color spaces then lead to a nullptr color space in a getter and that lead to the crash.

So the fix was to move KoColor::init even further down into KisApplication::start() when all resource paths are set up and the color spaces can be safely loaded.

I changed the Q_ASSERT in KoColor for checking that an init happened to a KIS_ASSERT so it also runs in production code, in case something might come up during app initialization that creates a color. It doesn't seem to happen though. It should not happen though, as then the whole color space problem just described would happen.

So it's actually a good thing that KoColor::init now has a defined place in the startup code that guarantees that color spaces init does not happen before that.

I added several checks and warnings that should really detect if anything in color space init or lookup goes wrong (I added an assert that color space loading now cannot happen before the resource paths are in place).

Hi, @dkazakov,

concerning (1), I believe it's better to actually enforce an assert as everything else will lead to color space init issues as described in my crash fix text. If the first color space is created before KisApplication has set up the resource paths, we're in big trouble (and this has nothing to do with this PR, to make this clear).

I'll change (2) and (3).

dkazakov accepted this revision.Oct 3 2017, 2:58 PM

The patch works fine now. Please fix (2) and (3) and push the patch without further review :)

This revision is now accepted and ready to land.Oct 3 2017, 2:58 PM

I pushed the implementation to krita:bliebl/kocolor now. Is this the right approach?

I had to rework the prefab thing a bit, as I didn't want to add three separate static prefab attributes.

I pushed the implementation to krita:bliebl/kocolor now. Is this the right approach?

I had to rework the prefab thing a bit, as I didn't want to add three separate static prefab attributes.

Well, actually, unless you feel you want to continue poking at it, an accepted review request means you can just merge/push it to master. We can always revert if you make a mistake, so don't worry :)

OK, pushed.

jounip added a subscriber: jounip.Oct 9 2017, 3:15 PM

This change seems to break a large number of unit tests. Specifically, they fail in KoResourcePaths::assertReady().

Many unit tests attempt to create a color space, which indirectly causes LcmsEnginePlugin to be constructed and thus trigger this assert.

rempt reopened this revision.Oct 18 2017, 10:21 AM

We really need to figure out how to fix the unittests....

This revision is now accepted and ready to land.Oct 18 2017, 10:21 AM
rempt requested changes to this revision.Oct 18 2017, 10:21 AM
This revision now requires changes to proceed.Oct 18 2017, 10:21 AM
poke1024 updated this revision to Diff 20997.EditedOct 20 2017, 6:00 AM

@rempt KoResourcePaths::assertReady() is really only useful inside KisApplication. I suggest we deactivate by default and only activate it explicitly inside KisApplication.

This revision was automatically updated to reflect the committed changes.