BUG: 384561
This code is used to pretty print the amount of data copied.
Replace this duplicated code by calling formatByteSize from KFormat.
Details
- Reviewers
mwolff - Group Reviewers
Frameworks - Commits
- R288:240a9c1f01d0: Code de-duplication in byteSize(double size)
Diff Detail
- Repository
- R288 KJobWidgets
- Branch
- performance (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage
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 ↗ | (On Diff #25121) | how about making that list static and turning it into an initializer list? static const auto s_units { QCoreApplication::translate(...), ... } |
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?
- 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
src/kjobtrackerformatters.cpp | ||
---|---|---|
26 ↗ | (On Diff #25114) | Isn't KFormat from a different framework? Adjust the include to use <CamelCase> style, and check if CMakeLists.txt needs to be adjusted. |
- 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.
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?
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%.
- 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.