Faster simplejob start
ClosedPublic

Authored by jtamate on Fri, Jan 26, 5:02 PM.

Details

Summary

Changing the signals connections to the new syntax. From being expensive to not being visible in kcachegrind.

Test Plan

kioclient5 move of 3000 files and move of 3000 files in dolphin to different disks (no rename).

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
jtamate created this revision.Fri, Jan 26, 5:02 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptFri, Jan 26, 5:02 PM
jtamate requested review of this revision.Fri, Jan 26, 5:02 PM

From spending 11.65% to 7.06%

Is there any problems to change other connects too?

Is there any problems to change other connects too?

There is only one problem, that I do not know how to do it.

broulik added inline comments.
src/core/simplejob.cpp
138–139

Try something along the lines of

q->connect(slave, &Slave::infoMessage, q, std::bind(&SimpleJobPrivate::_k_slotSlaveInfoMessage, this);

Note that if there's a disconnect for that particular signal somewhere in the code this will not work but at a quick glance I can only see a disconnect(m_slave); which is fine

QObject::connect is static, so you might as well do QObject::connect instead of q->connect

jtamate updated this revision to Diff 26047.Sat, Jan 27, 11:46 AM
  • Faster simplejob start

Finally the C++11 lambda syntax worked.
Now it only uses 0.3% of cpu according to callgrind.

jtamate updated this revision to Diff 26109.Sun, Jan 28, 10:21 AM

Reverting to first patch.

The lambda syntax has some problems with private d_func pointers under dolphin

Thread 1 "dolphin" received signal SIGSEGV, Segmentation fault.
0x00007ffff21b5b4c in KJob::d_func (this=0x4000000000000000) at /g/5kde/frameworks/kcoreaddons/src/lib/jobs/kjob.h:651
651 Q_DECLARE_PRIVATE(KJob)
(gdb) where
#0 0x00007ffff21b5b4c in KJob::d_func (this=0x4000000000000000) at /g/5kde/frameworks/kcoreaddons/src/lib/jobs/kjob.h:651
#1 0x00007ffff21b3a77 in KJob::totalAmount (this=0x4000000000000000, unit=KJob::Bytes) at /g/5kde/frameworks/kcoreaddons/src/lib/jobs/kjob.cpp:235
#2 0x00007ffff39a22f3 in KIO::SimpleJobPrivate::slotTotalSize (this=0xd4de80, size=2097152) at /g/5kde/frameworks/kio/src/core/simplejob.cpp:268
#3 0x00007ffff39a1332 in KIO::SimpleJobPrivate::<lambda(KIO::filesize_t)>::operator()(KIO::filesize_t) const (__closure=0xdd0bc0, size=2097152)

at /g/5kde/frameworks/kio/src/core/simplejob.cpp:153

#4 0x00007ffff39a4120 in QtPrivate::FunctorCall<QtPrivate::IndexesList<0>, QtPrivate::List<long long unsigned int>, void, KIO::SimpleJobPrivate::start(KIO::Slave*)::<lambda(KIO::filesize_t)> >::call(KIO::SimpleJobPrivate::<lambda(KIO::filesize_t)> &, void **) (f=..., arg=0x7fffffffcc50)

at /usr/include/qt5/QtCore/qobjectdefs_impl.h:130
mwolff requested changes to this revision.Tue, Jan 30, 3:37 PM
mwolff added a subscriber: mwolff.

the crash may be due to missing context, the three-arg connect shouldn't ever be used imo

src/core/simplejob.cpp
141–142

use four-arg connect, i.e. also add q as context

144–145

dito

147–148

dito

154–155

dito

157

dito

160

dito

This revision now requires changes to proceed.Tue, Jan 30, 3:37 PM
jtamate updated this revision to Diff 26239.Wed, Jan 31, 9:05 AM
jtamate edited the summary of this revision. (Show Details)
jtamate edited the test plan for this revision. (Show Details)

Thanks to mwolf, broulik & anthonyfieroni.
It works now.

jtamate marked 7 inline comments as done.Wed, Jan 31, 9:05 AM

Unfortunately, the last connect creates a memory leak:

1018== 10,152 (72 direct, 10,080 indirect) bytes in 1 blocks are definitely lost in loss record 928 of 943

1018== at 0x4C2E6FF: operator new(unsigned long) (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)

1018== by 0xCAEFCFC: QObjectPrivate::connectImpl(QObject const*, int, QObject const*, void**, QtPrivate::QSlotObjectBase*, Qt::ConnectionType, int const*, QMetaObject const*) (in /usr/lib64/libQt5Core.so.5.10.0)

1018== by 0xCAF0084: QObject::connectImpl(QObject const*, void, QObject const*, void, QtPrivate::QSlotObjectBase*, Qt::ConnectionType, int const*, QMetaObject const*) (in /usr/lib64/libQt5Core.so.5.10.0)

1018== by 0x9141BB5: std::enable_if<QtPrivate::FunctionPointer<KIO::SimpleJobPrivate::start(KIO::Slave*)::{lambda(unsigned long long)#4}>::ArgumentCount==(-1), QMetaObject::Connection>::type QObject::connect<void (KIO::SlaveInterface::*)(unsigned long long), KIO::SimpleJobPrivate::start(KIO::Slave*)::{lambda(unsigned long long)#4}>(QtPrivate<void (KIO::SlaveInterface::*)(unsigned long long)>::Object const*, std::enable_if<QtPrivate::FunctionPointer<KIO::SimpleJobPrivate::start(KIO::Slave*)::{lambda(unsigned long long)#4}>::ArgumentCount==(-1), QMetaObject::Connection>::type, QObject const*, QtPrivate::FunctionPointer, Qt::ConnectionType) (qobject.h:338)

1018== by 0x913FCCB: KIO::SimpleJobPrivate::start(KIO::Slave*) (simplejob.cpp:151)

1018== by 0x914B3D1: KIO::TransferJobPrivate::start(KIO::Slave*) (transferjob.cpp:362)

1018== by 0x914D063: startJob(KIO::SimpleJob*, KIO::Slave*) (scheduler.cpp:60)

1018== by 0x914F661: KIO::ProtoQueue::startAJob() (scheduler.cpp:635)

1018== by 0x9152AC4: KIO::ProtoQueue::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) (moc_scheduler_p.cpp:252)

1018== by 0xCAEBDB9: QMetaObject::activate(QObject*, int, int, void**) (in /usr/lib64/libQt5Core.so.5.10.0)

1018== by 0xCAF8236: QTimer::timeout(QTimer::QPrivateSignal) (in /usr/lib64/libQt5Core.so.5.10.0)

1018== by 0xCAF8567: QTimer::timerEvent(QTimerEvent*) (in /usr/lib64/libQt5Core.so.5.10.0)

mwolff added a comment.Fri, Feb 2, 3:53 PM

only that one, not the others? strange, why should that be the case? are you sure q and slave get destroyed?

only that one, not the others? strange, why should that be the case? are you sure q and slave get destroyed?

Yes, only that one. I've tried with const parámeters in the lambdas, same problem.
And https://forum.qt.io/topic/86962/qt5-new-signal-to-lambda-connections-can-result-memory-leak/3 says the leaks were fixed in Qt5.0.1
How to reproduce:
apply the patch :-)
valgrind --log-file=whatever.log dolphin (that uses the compiled kio)
copy&paste one file and drag&drop one file.
Exit and check whatever.log

mwolff added a comment.Fri, Feb 2, 4:37 PM

can you test and make sure that q and slave actually get destroyed? If that is the case, then the slotobj should get destroyed too. If not then this is a bug in Qt, which would make me wonder... Do you have a selfcompiled Qt, or one shipped by your distro? There was https://codereview.qt-project.org/#/c/215333/ but that's for QMetaObject::invoke which isn't relevant here... Odd!

can you test and make sure that q and slave actually get destroyed? If that is the case, then the slotobj should get destroyed too. If not then this is a bug in Qt, which would make me wonder... Do you have a selfcompiled Qt, or one shipped by your distro? There was https://codereview.qt-project.org/#/c/215333/ but that's for QMetaObject::invoke which isn't relevant here... Odd!

Here doesn't happen!. Only in the other Linux. I'll try again next week in the other linux.
Both linux have Qt from OpenSuse.

False alarm. :-)
Do not know what caused it, but after a complete kdesrc-build, the leak is gone.

So, good to go?

Looks good, but this means you can also remove the Q_PRIVATE_SLOT declaration for those slots defined in the private class (slotConnected, _k_foo etc.)

jtamate updated this revision to Diff 26787.Thu, Feb 8, 8:17 PM

Removed the private slots.

dfaure accepted this revision.Thu, Feb 8, 8:54 PM
This revision was not accepted when it landed; it landed in state Needs Review.Thu, Feb 8, 9:19 PM
This revision was automatically updated to reflect the committed changes.