Differential D9770

Code de-duplication in byteSize(double size)
ClosedPublic
Actions

Authored by jtamate on Jan 9 2018, 6:22 PM.

Details

Reviewers
mwolff
Group Reviewers
Frameworks
Commits
R288:240a9c1f01d0: Code de-duplication in byteSize(double size)
Summary

BUG: 384561
This code is used to pretty print the amount of data copied.
Replace this duplicated code by calling formatByteSize from KFormat.

Test Plan

Diff Detail

Repository
R288 KJobWidgets
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.Jan 9 2018, 6:22 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 9 2018, 6:22 PM
jtamate requested review of this revision.Jan 9 2018, 6:22 PM
broulik added a subscriber: broulik.Jan 9 2018, 6:38 PM

Nice catch.

src/kjobtrackerformatters.cpp
30

These aren't translated anymore. The translate call is also a hint that these strings should be extracted

31

how about making that list static and turning it into an initializer list?

static const auto s_units {
    QCoreApplication::translate(...),
    ...
}
mwolff requested changes to this revision.Jan 10 2018, 7:59 AM
mwolff added a subscriber: mwolff.

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?

This revision now requires changes to proceed.Jan 10 2018, 7:59 AM
jtamate updated this revision to Diff 25114.Jan 10 2018, 6:55 PM
  • 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

cfeck added a subscriber: cfeck.Jan 10 2018, 7:24 PM
cfeck added inline comments.
src/kjobtrackerformatters.cpp
26

Isn't KFormat from a different framework? Adjust the include to use <CamelCase> style, and check if CMakeLists.txt needs to be adjusted.

jtamate updated this revision to Diff 25121.Jan 10 2018, 7:49 PM
  • 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.

mwolff added a comment.Jan 11 2018, 9:35 AM

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?

jtamate added a comment.Jan 11 2018, 5:59 PM
In 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%.

ngraham edited the summary of this revision. (Show Details)Jan 12 2018, 5:00 PM
ngraham added a subscriber: ngraham.
ngraham added a comment.Jan 14 2018, 2:18 PM

Is anything else needed here before we can land this?

mwolff accepted this revision.Jan 15 2018, 9:50 AM
  • 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.

This revision is now accepted and ready to land.Jan 15 2018, 9:50 AM
jtamate retitled this revision from Optimization of byteSize(double size) to Code de-duplication in byteSize(double size).Jan 15 2018, 10:10 AM
jtamate edited the summary of this revision. (Show Details)
jtamate edited the test plan for this revision. (Show Details)
Closed by commit R288:240a9c1f01d0: Code de-duplication in byteSize(double size) (authored by jtamate). · Explain WhyJan 15 2018, 6:39 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Msrc/kjobtrackerformatters.cpp (28 lines)

Diff 25411

View Options

src/kjobtrackerformatters.cpp

Loading...