D17528: Refactor SlaveInterface::calcSpeed

chinmoyr added inline comments. INLINE COMMENTS > fvogt wrote in slaveinterface.cpp:111 > This is undefined behaviour if the vector is empty. Initial item is set to {0,0} before calcSpeed is called. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D17528 To: chinmoyr,

D17528: Refactor SlaveInterface::calcSpeed

bruns added inline comments. INLINE COMMENTS > fvogt wrote in slaveinterface.cpp:114 > What does this actually test for? 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. > fvogt wrote in

D17528: Refactor SlaveInterface::calcSpeed

fvogt requested changes to this revision. fvogt added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > slaveinterface.cpp:102 > +// Note for future reference: A list is maintained for sizes and times. > +// Minimum list size is 1 and maximum list size is

D17528: Refactor SlaveInterface::calcSpeed

davidedmundson added a comment. I opened a review for documentation purposes: https://phabricator.kde.org/D18158 REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D17528 To: chinmoyr, dfaure Cc: davidedmundson, broulik, bruns, kde-frameworks-devel, michaelh, ngraham

D17528: Refactor SlaveInterface::calcSpeed

chinmoyr added inline comments. INLINE COMMENTS > davidedmundson wrote in slaveinterface.cpp:106 > 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 >=

D17528: Refactor SlaveInterface::calcSpeed

davidedmundson added inline comments. INLINE COMMENTS > slaveinterface.cpp:106 > +const qint64 elapsed_time = d->elapsed_timer.elapsed(); > +if (elapsed_time >= 900) { > +if (d->transfer_details.count() == max_count) { That wasn't there in the old code because it has this guard:

D17528: Refactor SlaveInterface::calcSpeed

broulik reopened this revision. broulik added a comment. This revision is now accepted and ready to land. 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.

D17528: Refactor SlaveInterface::calcSpeed

This revision was automatically updated to reflect the committed changes. Closed by commit R241:4e2a815b9a10: Refactor SlaveInterface::calcSpeed (authored by chinmoyr). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17528?vs=47950=48003 REVISION DETAIL

D17528: Refactor SlaveInterface::calcSpeed

dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO BRANCH master REVISION DETAIL https://phabricator.kde.org/D17528 To: chinmoyr, dfaure Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D17528: Refactor SlaveInterface::calcSpeed

chinmoyr updated this revision to Diff 47950. chinmoyr added a comment. update REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17528?vs=47726=47950 BRANCH master REVISION DETAIL https://phabricator.kde.org/D17528 AFFECTED FILES

D17528: Refactor SlaveInterface::calcSpeed

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. endless refactoring, for the better :-) INLINE COMMENTS > slaveinterface.cpp:119 > +d->transfer_details.clear(); > +

D17528: Refactor SlaveInterface::calcSpeed

chinmoyr updated this revision to Diff 47726. chinmoyr added a comment. Removed vector initialization. Increased vector capacity in constructor. Appended {0,0} at first. Stored time and size before appending to prevent extraction. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE

D17528: Refactor SlaveInterface::calcSpeed

bruns added inline comments. INLINE COMMENTS > slaveinterface.cpp:102 > +// Minimum list size is 1 and maximum list size is 8. Delta is calculated > +// using first and last item from the list at least every 900 msecs. > + .. at most ... > slaveinterface.cpp:203 > d->filesize

D17528: Refactor SlaveInterface::calcSpeed

dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > slaveinterface.cpp:112 > +const TransferInfo first = d->transfer_details.first(); > +const TransferInfo last = d->transfer_details.last(); > +

D17528: Refactor SlaveInterface::calcSpeed

chinmoyr updated this revision to Diff 47567. chinmoyr added a comment. Used a structure to group size and time. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17528?vs=47518=47567 BRANCH master REVISION DETAIL https://phabricator.kde.org/D17528

D17528: Refactor SlaveInterface::calcSpeed

bruns added inline comments. INLINE COMMENTS > slaveinterface_p.h:59 > +QVector sizes; > +QVector times; > +QElapsedTimer elapsed_timer; You could use a struct for both, e.g.: struct SpeedSample { KIO::filesize_t remainder; qint64 elapsedTime; } QVector

D17528: Refactor SlaveInterface::calcSpeed

chinmoyr updated this revision to Diff 47518. chinmoyr added a comment. 1. Updating D17528 <https://phabricator.kde.org/D17528>: Refactor SlaveInterface::calcSpeed # 2. Enter a brief description of the changes included in this update. 3. The first line is used as subject, next

D17528: Refactor SlaveInterface::calcSpeed

chinmoyr added a comment. In D17528#376349 , @dfaure wrote: > 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

D17528: Refactor SlaveInterface::calcSpeed

dfaure added a comment. 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) ? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D17528 To: chinmoyr, dfaure Cc:

D17528: Refactor SlaveInterface::calcSpeed

chinmoyr created this revision. chinmoyr added a reviewer: dfaure. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. chinmoyr requested review of this revision. REVISION SUMMARY Use QLinkedList instead of a static array. Use QElapsedTimer instead of