Refactor SlaveInterface::calcSpeed
Needs RevisionPublic

Authored by chinmoyr on Dec 12 2018, 9:21 AM.

Details

Summary

Use QLinkedList instead of a static array.
Use QElapsedTimer instead of QDateTime for elapsed time.

Test Plan

Copied several large files(5-20Gb). The difference between current and previous speed calculation was within 0-300Kb.

Diff Detail

Repository
R241 KIO
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 6109
Build 6127: arc lint + arc unit
chinmoyr created this revision.Dec 12 2018, 9:21 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptDec 12 2018, 9:21 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
chinmoyr requested review of this revision.Dec 12 2018, 9:21 AM

I'm surprised, how can a QLinkedList (with nodes allocated all over the memory) be better than a static array (which fits into the same memory cache) ?

I'm surprised, how can a QLinkedList (with nodes allocated all over the memory) be better than a static array (which fits into the same memory cache) ?

IMO It is better only in terms of how the code looks. I saw the for loop shifting elements and immediately felt the urge to use QLinkedList. I never thought of the memory allocation.

chinmoyr updated this revision to Diff 47518.EditedDec 13 2018, 2:28 PM

Used QVector

bruns added a subscriber: bruns.Dec 13 2018, 7:14 PM
bruns added inline comments.
src/core/slaveinterface_p.h
59

You could use a struct for both, e.g.:

struct SpeedSample {
    KIO::filesize_t remainder;
    qint64 elapsedTime;
}
QVector<SpeedSample> transferSpeed;

This would emphasize the two values always come in pairs, and would even remove some code above.

chinmoyr updated this revision to Diff 47567.Dec 14 2018, 2:51 PM

Used a structure to group size and time.

dfaure requested changes to this revision.Dec 14 2018, 3:37 PM
dfaure added inline comments.
src/core/slaveinterface.cpp
112 ↗(On Diff #47567)

why not just call "last" the (currently unnamed) TransferInfo created 2 lines above, rather than extracting it out of the vector just after appending?

src/core/slaveinterface_p.h
39 ↗(On Diff #47567)

global namespace pollution, better keep this within KIO::SlaveInterfacePrivate.

48 ↗(On Diff #47567)

the old "nums" is now the vector size, right?

nums was initialized to 0, so this should not initialize the vector to max_count items

This revision now requires changes to proceed.Dec 14 2018, 3:37 PM
bruns added inline comments.Dec 15 2018, 12:25 AM
src/core/slaveinterface.cpp
102 ↗(On Diff #47567)

.. at most ...

203 ↗(On Diff #47567)

Strange way of writing {0, 0} ...

src/core/slaveinterface_p.h
48 ↗(On Diff #47567)

The result is the same in this case - after 7 seconds, the initial items are shifted out of the array. During these 7 seconds, the first element is always {0, 0}. It does not matter if last() is the 2nd or 7th element.

chinmoyr updated this revision to Diff 47726.Dec 17 2018, 5:12 PM

Removed vector initialization.
Increased vector capacity in constructor.
Appended {0,0} at first.
Stored time and size before appending to prevent extraction.

dfaure requested changes to this revision.Dec 18 2018, 4:22 PM

endless refactoring, for the better :-)

src/core/slaveinterface.cpp
119

.append(last) would do the same in a more compact way.

... and then this means we could reduce code size even further by calculating lspeed *before* doing the append.

first = ...
last = ...
lspeed = ...
if (!lspeed) {
    d->transfer_details.clear();
}
d->transfer_details.append(last);
This revision now requires changes to proceed.Dec 18 2018, 4:22 PM
dfaure accepted this revision.Dec 21 2018, 1:50 PM
This revision is now accepted and ready to land.Dec 21 2018, 1:50 PM
This revision was automatically updated to reflect the committed changes.
broulik reopened this revision.Jan 10 2019, 12:59 PM
broulik added a subscriber: broulik.

We're getting some SIGFPE crash reports in this area: Bug 402665
I failed to reproduce them, though. Best is probably to revert this patch because Frameworks release is imminent.

src/core/slaveinterface.cpp
117 ↗(On Diff #47567)

We likely get a division by zero here for some reason

This revision is now accepted and ready to land.Jan 10 2019, 12:59 PM
davidedmundson added inline comments.
src/core/slaveinterface.cpp
112 ↗(On Diff #47567)

That wasn't there in the old code because it has this guard:

const qint64 diff = currentTime - d->start_time;

(effectively calculating elapsed)

Then

if (diff - d->last_time >= 900) {

So we known first-last is non-zero

Should the new code be

https://phabricator.kde.org/P287 ?

It seemed to work for me.

chinmoyr added inline comments.Jan 10 2019, 3:40 PM
src/core/slaveinterface.cpp
112 ↗(On Diff #47567)

Indeed that should be the code here.
The old code checked the elapsed time since last "calcSpeed()" call whereas the new code checks the elapsed time since the timer started.

117 ↗(On Diff #47567)

I doubt it will be zero here. elapsed_time stores elapsed time since the timer started so it will continue to grow (?) and the difference will always be > 0.

I opened a review for documentation purposes: https://phabricator.kde.org/D18158

fvogt requested changes to this revision.Jan 14 2019, 12:27 PM
fvogt added a subscriber: fvogt.
fvogt added inline comments.
src/core/slaveinterface.cpp
110 ↗(On Diff #47567)

This comment should refer to max_count instead of hardcoding the number.

115 ↗(On Diff #47567)

This is undefined behaviour if the vector is empty.

115–116 ↗(On Diff #47567)

What does this actually test for?

117 ↗(On Diff #47567)

Doubt is not enough - it is absolutely necessary to special case that. A div / 0 is an instant crash on x86_64.

src/core/slaveinterface_p.h
39 ↗(On Diff #47567)

I absolutely agree here.

There is no reason to have this as a global variable, especially not a "static" one in a header.

Please move it into the class.

67 ↗(On Diff #47567)

Is there any reason to use a qvector instead of a std::array/c array?
It'll only introduce more overhead.

This revision now requires changes to proceed.Jan 14 2019, 12:27 PM
bruns added inline comments.Jan 14 2019, 1:18 PM
src/core/slaveinterface.cpp
115–116 ↗(On Diff #47567)

This results in a fast ramp up when the transfer has stalled for more than ~8 seconds, like at the begin of a transfer. Likely needs a comment.

src/core/slaveinterface_p.h
67 ↗(On Diff #47567)

std::array can not be resized. At first, there is only one element.

This requirement could be avoided around by filling the the array with {0,0} elements, which results in identical behavior.

We would also need a method for shifting the entries. (.removeFirst(), append()).

chinmoyr added inline comments.Jan 14 2019, 1:47 PM
src/core/slaveinterface.cpp
115 ↗(On Diff #47567)

Initial item is set to {0,0} before calcSpeed is called.