D17528: Refactor SlaveInterface::calcSpeed

[Chinmoy Ranjan Pradhan]

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, dfaure, fvogt
Cc: fvogt, davidedmundson, broulik, bruns, kde-frameworks-devel, michaelh, 
ngraham

D17528: Refactor SlaveInterface::calcSpeed

[Stefan Brüns]

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 slaveinterface_p.h:62
> Is there any reason to use a qvector instead of a std::array/c array?
> It'll only introduce more overhead.

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()).

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17528

To: chinmoyr, dfaure, fvogt
Cc: fvogt, davidedmundson, broulik, bruns, kde-frameworks-devel, michaelh, 
ngraham

D17528: Refactor SlaveInterface::calcSpeed

[Fabian Vogt]

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 8. Delta is calculated
> +// using first and last item from the list.

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

> slaveinterface.cpp:111
>  
> +const SlaveInterfacePrivate::TransferInfo first = 
> d->transfer_details.first();
> +const SlaveInterfacePrivate::TransferInfo last = {elapsed_time, 
> (d->filesize - d->offset)};

This is undefined behaviour if the vector is empty.

> chinmoyr wrote in slaveinterface.cpp:113
> 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.

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

> slaveinterface.cpp:114
> +KIO::filesize_t lspeed = 1000 * (last.size - first.size) / 
> (last.time - first.time);
>  if (!lspeed) {
> +d->transfer_details.clear();

What does this actually test for?

> slaveinterface_p.h:62
> +};
> +QVector transfer_details;
> +QElapsedTimer elapsed_timer;

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

> dfaure wrote in slaveinterface_p.h:39
> global namespace pollution, better keep this within 
> KIO::SlaveInterfacePrivate.

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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17528

To: chinmoyr, dfaure, fvogt
Cc: fvogt, davidedmundson, broulik, bruns, kde-frameworks-devel, michaelh, 
ngraham

D17528: Refactor SlaveInterface::calcSpeed

[David Edmundson]

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

[Chinmoy Ranjan Pradhan]

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 >= 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.

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.

> broulik wrote in slaveinterface.cpp:113
> We likely get a division by zero here for some reason

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.

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

[David Edmundson]

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:

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.

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

[Kai Uwe Broulik]

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.

INLINE COMMENTS

> slaveinterface.cpp:113
> +const SlaveInterfacePrivate::TransferInfo last = {elapsed_time, 
> (d->filesize - d->offset)};
> +KIO::filesize_t lspeed = 1000 * (last.size - first.size) / 
> (last.time - first.time);
>  if (!lspeed) {

We likely get a division by zero here for some reason

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17528

To: chinmoyr, dfaure
Cc: broulik, bruns, kde-frameworks-devel, michaelh, ngraham

D17528: Refactor SlaveInterface::calcSpeed

[Chinmoy Ranjan Pradhan]

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&id=48003

REVISION DETAIL
  https://phabricator.kde.org/D17528

AFFECTED FILES
  src/core/slaveinterface.cpp
  src/core/slaveinterface_p.h

To: chinmoyr, dfaure
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D17528: Refactor SlaveInterface::calcSpeed

[David Faure]

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

[Chinmoy Ranjan Pradhan]

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&id=47950

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D17528

AFFECTED FILES
  src/core/slaveinterface.cpp
  src/core/slaveinterface_p.h

To: chinmoyr, dfaure
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D17528: Refactor SlaveInterface::calcSpeed

[David Faure]

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();
> +d->transfer_details.append({elapsed_time, (d->filesize - 
> d->offset)});
>  }

.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);

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17528

To: chinmoyr, dfaure
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D17528: Refactor SlaveInterface::calcSpeed

[Chinmoy Ranjan Pradhan]

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
  https://phabricator.kde.org/D17528?vs=47567&id=47726

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D17528

AFFECTED FILES
  src/core/slaveinterface.cpp
  src/core/slaveinterface_p.h

To: chinmoyr, dfaure
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D17528: Refactor SlaveInterface::calcSpeed

[Stefan Brüns]

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 = d->offset;
> -d->sizes[0] = d->filesize - d->offset;
> -d->times[0] = 0;
> -d->nums = 1;
> +d->transfer_details.append({0, (d->filesize - d->offset)});
>  d->speed_timer.start(1000);

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

> dfaure wrote in slaveinterface_p.h:48
> 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

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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17528

To: chinmoyr, dfaure
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D17528: Refactor SlaveInterface::calcSpeed

[David Faure]

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();
> +KIO::filesize_t lspeed = 1000 * (last.size - first.size) / 
> (last.time - first.time);

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

> slaveinterface_p.h:39
> +
> +struct TransferInfo {
> +qint64 time;

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

> slaveinterface_p.h:48
>  SlaveInterfacePrivate()
> -: connection(nullptr), filesize(0), offset(0), last_time(0), 
> start_time(0),
> -  nums(0), slave_calcs_speed(false)
> +: connection(nullptr), transfer_details(max_count), filesize(0), 
> offset(0),
> +  slave_calcs_speed(false)

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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17528

To: chinmoyr, dfaure
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D17528: Refactor SlaveInterface::calcSpeed

[Chinmoy Ranjan Pradhan]

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&id=47567

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D17528

AFFECTED FILES
  src/core/slaveinterface.cpp
  src/core/slaveinterface_p.h

To: chinmoyr, dfaure
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D17528: Refactor SlaveInterface::calcSpeed

[Stefan Brüns]

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 transferSpeed;

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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17528

To: chinmoyr, dfaure
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D17528: Refactor SlaveInterface::calcSpeed

[Chinmoy Ranjan Pradhan]

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 lines as comment. #
  4. If you intended to create a new revision, use:
  5. $ arc diff --create
  
  Used QVector

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17528?vs=47427&id=47518

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D17528

AFFECTED FILES
  src/core/slaveinterface.cpp
  src/core/slaveinterface_p.h

To: chinmoyr, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17528: Refactor SlaveInterface::calcSpeed

[Chinmoy Ranjan Pradhan]

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 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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17528

To: chinmoyr, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17528: Refactor SlaveInterface::calcSpeed

[David Faure]

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: kde-frameworks-devel, michaelh, ngraham, bruns

D17528: Refactor SlaveInterface::calcSpeed

[Chinmoy Ranjan Pradhan]

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 QDateTime for elapsed time.

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

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D17528

AFFECTED FILES
  src/core/slaveinterface.cpp
  src/core/slaveinterface_p.h

To: chinmoyr, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns