D10124: Faster simplejob start

[Jaime Torres Amate]

jtamate added a comment.


  
  
  In D10124#213918 , @dfaure wrote:
  
  > This commit leads to
  >
  > 20:29:06.184 okteta(12932) QObject::connect|?libKF5KIOCore.so.5? 
QObject::connect: No such slot KIO::ListJob::slotTotalSize(KIO::filesize_t) in 
/d/kde/src/5/frameworks/kio/src/core/listjob.cpp:289
  >
  > Please fix ;)
  
  
  Please, take a look at: https://phabricator.kde.org/D10537

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10124

To: jtamate, #frameworks, mwolff, dfaure
Cc: mwolff, broulik, ngraham, anthonyfieroni, michaelh

D10124: Faster simplejob start

[David Faure]

dfaure added a comment.


  This commit leads to
  
  20:29:06.184 okteta(12932) QObject::connect|?libKF5KIOCore.so.5? 
QObject::connect: No such slot KIO::ListJob::slotTotalSize(KIO::filesize_t) in 
/d/kde/src/5/frameworks/kio/src/core/listjob.cpp:289
  
  Please fix ;)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10124

To: jtamate, #frameworks, mwolff, dfaure
Cc: mwolff, broulik, ngraham, anthonyfieroni, michaelh, kmorwinski

D10124: Faster simplejob start

[Jaime Torres Amate]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R241:42fed2e70a21: Faster simplejob start (authored by 
jtamate).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10124?vs=26787=26792

REVISION DETAIL
  https://phabricator.kde.org/D10124

AFFECTED FILES
  src/core/simplejob.cpp
  src/core/simplejob.h

To: jtamate, #frameworks, mwolff, dfaure
Cc: mwolff, broulik, ngraham, anthonyfieroni, michaelh

D10124: Faster simplejob start

[David Faure]

dfaure accepted this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10124

To: jtamate, #frameworks, mwolff, dfaure
Cc: mwolff, broulik, ngraham, anthonyfieroni, michaelh

D10124: Faster simplejob start

[Jaime Torres Amate]

jtamate updated this revision to Diff 26787.
jtamate added a comment.


  Removed the private slots.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10124?vs=26239=26787

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D10124

AFFECTED FILES
  src/core/simplejob.cpp
  src/core/simplejob.h

To: jtamate, #frameworks, mwolff, dfaure
Cc: mwolff, broulik, ngraham, anthonyfieroni, michaelh

D10124: Faster simplejob start

[David Faure]

dfaure added a comment.


  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.)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10124

To: jtamate, #frameworks, mwolff, dfaure
Cc: mwolff, broulik, ngraham, anthonyfieroni, michaelh

D10124: Faster simplejob start

[Jaime Torres Amate]

jtamate added a reviewer: dfaure.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10124

To: jtamate, #frameworks, mwolff, dfaure
Cc: mwolff, broulik, ngraham, anthonyfieroni, michaelh

D10124: Faster simplejob start

[Kai Uwe Broulik]

broulik added a comment.


  So, good to go?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10124

To: jtamate, #frameworks, mwolff
Cc: mwolff, broulik, ngraham, anthonyfieroni, michaelh

D10124: Faster simplejob start

[Jaime Torres Amate]

jtamate added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10124

To: jtamate, #frameworks, mwolff
Cc: mwolff, broulik, ngraham, anthonyfieroni, michaelh

D10124: Faster simplejob start

[Jaime Torres Amate]

jtamate added a comment.


  In https://phabricator.kde.org/D10124#199645, @mwolff wrote:
  
  > 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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10124

To: jtamate, #frameworks, mwolff
Cc: mwolff, broulik, ngraham, anthonyfieroni, michaelh

D10124: Faster simplejob start

[Milian Wolff]

mwolff added a comment.


  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!

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10124

To: jtamate, #frameworks, mwolff
Cc: mwolff, broulik, ngraham, anthonyfieroni, michaelh

D10124: Faster simplejob start

[Jaime Torres Amate]

jtamate added a comment.


  In https://phabricator.kde.org/D10124#199601, @mwolff wrote:
  
  > 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 one file and drag one file.
  Exit and check whatever.log

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10124

To: jtamate, #frameworks, mwolff
Cc: mwolff, broulik, ngraham, anthonyfieroni, michaelh

D10124: Faster simplejob start

[Milian Wolff]

mwolff added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10124

To: jtamate, #frameworks, mwolff
Cc: mwolff, broulik, ngraham, anthonyfieroni, michaelh

D10124: Faster simplejob start

[Jaime Torres Amate]

jtamate added a comment.


  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::ArgumentCount==(-1), QMetaObject::Connection>::type 
QObject::connect(QtPrivate::Object 
const*, 
std::enable_if::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)
  
-

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10124

To: jtamate, #frameworks, mwolff
Cc: mwolff, broulik, ngraham, anthonyfieroni, michaelh

D10124: Faster simplejob start

[Jaime Torres Amate]

jtamate marked 7 inline comments as done.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10124

To: jtamate, #frameworks, mwolff
Cc: mwolff, broulik, ngraham, anthonyfieroni, michaelh

D10124: Faster simplejob start

[Jaime Torres Amate]

jtamate updated this revision to Diff 26239.
jtamate edited the summary of this revision.
jtamate edited the test plan for this revision.
jtamate added a comment.


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

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10124?vs=26109=26239

REVISION DETAIL
  https://phabricator.kde.org/D10124

AFFECTED FILES
  src/core/simplejob.cpp

To: jtamate, #frameworks, mwolff
Cc: mwolff, broulik, ngraham, anthonyfieroni, michaelh

D10124: Faster simplejob start

[Milian Wolff]

mwolff requested changes to this revision.
mwolff added a comment.
This revision now requires changes to proceed.


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

INLINE COMMENTS

> simplejob.cpp:141
>  
> -q->connect(slave, SIGNAL(connected()),
> -   SLOT(slotConnected()));
> +QObject::connect(slave, ::infoMessage,
> +   [=](const QString& message){ 
> this->_k_slotSlaveInfoMessage(message);} );

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

> simplejob.cpp:144
>  
> -q->connect(slave, SIGNAL(finished()),
> -   SLOT(slotFinished()));
> +QObject::connect(slave, ::connected,
> +   [=](){ this->slotConnected();} );

dito

> simplejob.cpp:147
>  
> -q->connect(slave, SIGNAL(privilegeOperationRequested()),
> -   SLOT(slotPrivilegeOperationRequested()));
> +QObject::connect(slave, ::privilegeOperationRequested,
> +   [=](){ this->slotPrivilegeOperationRequested();} );

dito

> simplejob.cpp:152
>  
> -q->connect(slave, SIGNAL(processedSize(KIO::filesize_t)),
> -   SLOT(slotProcessedSize(KIO::filesize_t)));
> +QObject::connect(slave, ::totalSize,
> +   [=](KIO::filesize_t size){ this->slotTotalSize(size);} );

dito

> simplejob.cpp:155
> +
> +QObject::connect(slave, ::processedSize,
> +   [=](KIO::filesize_t size){ this->slotProcessedSize(size);} );

dito

> simplejob.cpp:158
> +
> +QObject::connect(slave, ::speed,
> +   [=](ulong speed){ this->slotSpeed(speed);} );

dito

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10124

To: jtamate, #frameworks, mwolff
Cc: mwolff, broulik, ngraham, anthonyfieroni, michaelh

D10124: Faster simplejob start

[Jaime Torres Amate]

jtamate updated this revision to Diff 26109.
jtamate added a comment.


  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.
  0x721b5b4c in KJob::d_func (this=0x4000) at 
/g/5kde/frameworks/kcoreaddons/src/lib/jobs/kjob.h:651
  651 Q_DECLARE_PRIVATE(KJob)
  (gdb) where
  #0  0x721b5b4c in KJob::d_func (this=0x4000) at 
/g/5kde/frameworks/kcoreaddons/src/lib/jobs/kjob.h:651
  #1  0x721b3a77 in KJob::totalAmount (this=0x4000, 
unit=KJob::Bytes) at /g/5kde/frameworks/kcoreaddons/src/lib/jobs/kjob.cpp:235
  #2  0x739a22f3 in KIO::SimpleJobPrivate::slotTotalSize 
(this=0xd4de80, size=2097152) at 
/g/5kde/frameworks/kio/src/core/simplejob.cpp:268
  #3  0x739a1332 in 
KIO::SimpleJobPrivate::::operator()(KIO::filesize_t) 
const (__closure=0xdd0bc0, size=2097152)
  
at /g/5kde/frameworks/kio/src/core/simplejob.cpp:153
  
  #4  0x739a4120 in QtPrivate::FunctorCall, 
QtPrivate::List, void, 
KIO::SimpleJobPrivate::start(KIO::Slave*):: 
>::call(KIO::SimpleJobPrivate:: &, void **) (f=..., 
arg=0x7fffcc50)
  
at /usr/include/qt5/QtCore/qobjectdefs_impl.h:130

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10124?vs=26047=26109

BRANCH
  start (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D10124

AFFECTED FILES
  src/core/simplejob.cpp

To: jtamate, #frameworks
Cc: broulik, ngraham, anthonyfieroni, michaelh

D10124: Faster simplejob start

[Jaime Torres Amate]

jtamate updated this revision to Diff 26047.
jtamate added a comment.


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

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10124?vs=26011=26047

BRANCH
  simplejob (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D10124

AFFECTED FILES
  src/core/simplejob.cpp

To: jtamate, #frameworks
Cc: broulik, ngraham, anthonyfieroni, michaelh

D10124: Faster simplejob start

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> simplejob.cpp:142
>  
>  q->connect(slave, SIGNAL(infoMessage(QString)),
> SLOT(_k_slotSlaveInfoMessage(QString)));

Try something along the lines of

  q->connect(slave, ::infoMessage, q, 
std::bind(::_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`

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10124

To: jtamate, #frameworks
Cc: broulik, ngraham, anthonyfieroni

D10124: Faster simplejob start

[Anthony Fieroni]

anthonyfieroni added a comment.


  This -> https://wiki.qt.io/New_Signal_Slot_Syntax#Overload

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10124

To: jtamate, #frameworks
Cc: ngraham, anthonyfieroni

D10124: Faster simplejob start

[Jaime Torres Amate]

jtamate added a comment.


  In https://phabricator.kde.org/D10124#196534, @anthonyfieroni wrote:
  
  > Is there any problems to change other connects too?
  
  
  There is only one problem,  that I do not know how to do it.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10124

To: jtamate, #frameworks
Cc: ngraham, anthonyfieroni

D10124: Faster simplejob start

[Jaime Torres Amate]

jtamate created this revision.
jtamate added a reviewer: Frameworks.
Restricted Application added a project: Frameworks.
jtamate requested review of this revision.

REVISION SUMMARY
  Changing only 4 of the signals connections to the new syntax almost
  doubles the spped of simplejob::start in a kioclient5 copy of 3000
  files.

TEST PLAN
  If someone knows how to change the other 6 connect, please, please
  do it.
  I've tried lots of combinations but I've been unable to find the
  one that compiles.

REPOSITORY
  R241 KIO

BRANCH
  simplejob (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D10124

AFFECTED FILES
  src/core/simplejob.cpp

To: jtamate, #frameworks

D10124: Faster simplejob start

[Anthony Fieroni]

anthonyfieroni added a comment.


  Is there any problems to change other connects too?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10124

To: jtamate, #frameworks
Cc: anthonyfieroni

D10124: Faster simplejob start

[Jaime Torres Amate]

jtamate added a comment.


  F5675857: kcioclient5_move_2000_files_old_connect.png 

  
  F5675856: kcioclient5_move_2000_files_partly_connect.png 

  
  From spending 11.65% to 7.06%

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10124

To: jtamate, #frameworks