D9770: Optimization of byteSize(double size)
mwolff accepted this revision. mwolff added a comment. This revision is now accepted and ready to land. - do not ever profile a debug build, the results are completely bogus - do not use callgrind, use perf that said, I'm OK with this patch but the commit message should not talk about performance, but rather about code de-duplication. I doubt this has such a bit effect in a release build. REPOSITORY R288 KJobWidgets BRANCH performance (branched from master) REVISION DETAIL https://phabricator.kde.org/D9770 To: jtamate, #frameworks, mwolff Cc: ngraham, cfeck, mwolff, broulik
D9770: Optimization of byteSize(double size)
ngraham added a comment. Is anything else needed here before we can land this? REPOSITORY R288 KJobWidgets REVISION DETAIL https://phabricator.kde.org/D9770 To: jtamate, #frameworks, mwolff Cc: ngraham, cfeck, mwolff, broulik
D9770: Optimization of byteSize(double size)
ngraham edited the summary of this revision. REPOSITORY R288 KJobWidgets REVISION DETAIL https://phabricator.kde.org/D9770 To: jtamate, #frameworks, mwolff Cc: ngraham, cfeck, mwolff, broulik
D9770: Optimization of byteSize(double size)
jtamate added a comment. In https://phabricator.kde.org/D9770#189242, @mwolff wrote: > 10 calls per second sound fine to me, that shouldn't be a big performance issue at all. Yes, that is what I tough, but as soon as I changed the code, dolphin(file.so) started to copy as fast as cp. > Are you measuring performance of a debug build or of a release build? I'm measuring performance in a debug build (I know, I know). > Can you specify the exact commands you are profiling? valgrind --tool=callgrind /usr/local/kde-build/bin/dolphin > Is the performance better when you are using KFormat here? Yes, the cpu usage with KFormat is around 0.0x%. REPOSITORY R288 KJobWidgets REVISION DETAIL https://phabricator.kde.org/D9770 To: jtamate, #frameworks, mwolff Cc: cfeck, mwolff, broulik
D9770: Optimization of byteSize(double size)
mwolff added a comment. 10 calls per second sound fine to me, that shouldn't be a big performance issue at all. Are you measuring performance of a debug build or of a release build? Can you specify the exact commands you are profiling? Is the performance better when you are using KFormat here? REPOSITORY R288 KJobWidgets REVISION DETAIL https://phabricator.kde.org/D9770 To: jtamate, #frameworks, mwolff Cc: cfeck, mwolff, broulik
D9770: Optimization of byteSize(double size)
jtamate updated this revision to Diff 25121.
jtamate added a comment.
- De-duplication of code in byteSize(double size)
Changed the include.
There is no need to change the CMakeLists.txt as
find_package(KF5CoreAddons ${KF5_DEP_VERSION} REQUIRED)
is already included.
REPOSITORY
R288 KJobWidgets
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D9770?vs=25114=25121
BRANCH
performance (branched from master)
REVISION DETAIL
https://phabricator.kde.org/D9770
AFFECTED FILES
src/kjobtrackerformatters.cpp
To: jtamate, #frameworks, mwolff
Cc: cfeck, mwolff, broulik
D9770: Optimization of byteSize(double size)
cfeck added inline comments. INLINE COMMENTS > kjobtrackerformatters.cpp:26 > #include "kjobtrackerformatters_p.h" > +#include "kformat.h" > Isn't KFormat from a different framework? Adjust the include to use style, and check if CMakeLists.txt needs to be adjusted. REPOSITORY R288 KJobWidgets REVISION DETAIL https://phabricator.kde.org/D9770 To: jtamate, #frameworks, mwolff Cc: cfeck, mwolff, broulik
D9770: Optimization of byteSize(double size)
jtamate updated this revision to Diff 25114. jtamate added a comment. - De-duplication of code in byteSize(double size) In fact Milian Wolff is right. Why so many calls to this function? Copying 20Gb it is called 19.026 times (aproximately 10 times per second) from processedSize() in slavebase.cpp in kio REPOSITORY R288 KJobWidgets CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9770?vs=25034=25114 BRANCH performance (branched from master) REVISION DETAIL https://phabricator.kde.org/D9770 AFFECTED FILES src/kjobtrackerformatters.cpp To: jtamate, #frameworks, mwolff Cc: mwolff, broulik
D9770: Optimization of byteSize(double size)
mwolff requested changes to this revision. mwolff added a comment. This revision now requires changes to proceed. Imo this should be using `KFormat::formatByteSize`. https://api.kde.org/frameworks/kcoreaddons/html/classKFormat.html#ae7412420b70e2ca935d0ebed6770e313 And if that function is slow, add a benchmark and optimize it there so that everyone profits and not only the job tracker. Furthermore: can you show me the callgrind file? I suspect the real issue is that `::byteSize` is called too often here. This is for a progress bar I suspect, right? That should only update itself every X ms at the most, instead of doing it millions of times which I suspect is the case here? REPOSITORY R288 KJobWidgets REVISION DETAIL https://phabricator.kde.org/D9770 To: jtamate, #frameworks, mwolff Cc: mwolff, broulik
D9770: Optimization of byteSize(double size)
broulik added a comment.
Nice catch.
INLINE COMMENTS
> kjobtrackerformatters.cpp:29
>
> +const char* units[] {"%1 B", "%1 KiB", "%1 MiB", "%1 GiB", "%1 TiB", "%1
> PiB", "%1 EiB", "%1 ZiB", "%1 YiB"};
> +const int unitsSize = sizeof(units)/sizeof(const char *);
These aren't translated anymore. The `translate` call is also a hint that these
strings should be extracted
> kjobtrackerformatters.cpp:31
> {
> -QList units;
> -units << QCoreApplication::translate("KJobTrackerFormatters", "%1 B")
how about making that list `static` and turning it into an initializer list?
static const auto s_units {
QCoreApplication::translate(...),
...
}
REPOSITORY
R288 KJobWidgets
REVISION DETAIL
https://phabricator.kde.org/D9770
To: jtamate, #frameworks
Cc: broulik
D9770: Optimization of byteSize(double size)
jtamate created this revision. jtamate added a reviewer: Frameworks. Restricted Application added a project: Frameworks. jtamate requested review of this revision. REVISION SUMMARY Copying 20Gib, using callgrind, before it used 5,2% of the cpu, and after only 0,58%. Applying this patch, the file copy speed is the same as cp/mv but the cpu usage grows from 40% up to 80% spent in io-wait. TEST PLAN callgrind of dolphin moving 4Gb files from ext4 to ntfs as in the bug 384561 REPOSITORY R288 KJobWidgets BRANCH performance (branched from master) REVISION DETAIL https://phabricator.kde.org/D9770 AFFECTED FILES src/kjobtrackerformatters.cpp To: jtamate, #frameworks
