Speed limit in ftp kio slave

Review Request #102267 - Created Aug. 9, 2011 and submitted

Information
Tushar Mehta
kdelibs
Reviewers
kdelibs
- This patch contains the basic code which will put the limit on download speed of the ftp data transfer.
- It is looking for "speed-limit" meta-data for deciding how much speed control is required.
- If this meta-data is not found, code will work as it was before and no speed control related code will come into picture.
- This patch is the most basic one which I have testing on my system and to the extent it is controlling the speed.
- Lets say if speed limit is 30 KBps then mostly will get the avg speed around 30 to 35 KBps.
- I am using QTime for measuring time elapsed between two socket read call and its precision is in millisecond. Looping is taking place in microsecond and thats why I am getting almost all the time 0 as time elapsed in between two calls.
- To solve the above problem usleep is introduced to make it sync with the timer.

  
David Faure

   
kioslave/ftp/ftp.cpp (Diff revision 1)
 
 
The construct "Type foo[variable]" is not portable (to compilers other than gcc). You have to use new[]+delete[], or QVarLengthArray or in the case of char like here, QByteArray. I would recommend QByteArray for this code.
kioslave/ftp/ftp.cpp (Diff revision 1)
 
 
What does the magic number represent?

Ah, 1 second? Would be worth a comment.
  1. When I started writing code, I was not using this wait. But what happened is that, the while loop is taking time in microseconds and the code which is calculating how many bytes to read is expecting elapsed time in millisecond. Because of this almost every time elapsed time is coming as zero.(as time granularity is in millisecond) and equation is return 0. So I thought to have some delay in ftp code.
    
    If we have a timer which is having granularity in microsecond then I can update the equation and can observe how code behaves with out any wait.
    
    I tried to find out about this type of timer and got to now that in Qt 4.7 it got introduced.(I am not sure about the qt version but in the latest one). It will be great if you can suggest me something about this.
    
    I wanted to have a opinion of developers so I put this magic number :) Let me know where I am doing mistake regarding this thought.
kioslave/ftp/speedController.h (Diff revision 1)
 
 
This seems to be your own code, the copyright is yours, not mine :)
  1. Hey :) This is my first patch to the KDE and in excitement I forgot to change this :) I will correct it sure :)
kioslave/ftp/speedController.h (Diff revision 1)
 
 
kde_file.h isn't used in this header -> move the #include to the .cpp file.
  1. I have used it for usleep. If I am not including it then it give me this:
    "error: ‘usleep’ was not declared in this scope"
  2. usleep is in <unistd.h>. Include that instead.
kioslave/ftp/speedController.h (Diff revision 1)
 
 
trailing whitespace
  1. acknowledged.
kioslave/ftp/speedController.h (Diff revision 1)
 
 
why not just return the int, like  int bytesToRead()?
  1. acknowledged. With the current design of the code we don't need pass by reference. At initial phase when I was experimenting with the code, I used this behaviour. I will correct this.
kioslave/ftp/speedController.cpp (Diff revision 1)
 
 
Not my code :)
  1. acknowledged.
kioslave/ftp/speedController.cpp (Diff revision 1)
 
 
Make getters const, for good practice.
  1. acknowledged.
kioslave/ftp/speedController.cpp (Diff revision 1)
 
 
This doesn't seem to "add" anything, but to "set". It replaces any existing socket.

Note: the naming is wrong. m_socket looks like a member variable, while "socket" is the actual member variable.

I would suggest to use m_ for the actual member vars, in fact -- and for sure never for function parameters.
  1. acknowledged.
kioslave/ftp/speedController.cpp (Diff revision 1)
 
 
The problem with usleep is that it's not portable (e.g. to Windows).

QThread::usleep would do the job -- but it's protected (for no good reason), so you will have to write

class ThreadWorkaround : public QThread
{
    using QThread::usleep;
}

and then you can write ThreadWorkaround::usleep().
Lukas Appelhans
Patch works fine here by the way.

Use case for this is KGet.
Thomas Zander

   
kioslave/ftp/speedController.h (Diff revision 1)
 
 
I'm wondering if the function name contains a typo;  maybe you meant 'calcHowMuchToRead' (notice the extra c in calc) ?
  1. acknowledged.
Thiago Macieira

   
kioslave/ftp/CMakeLists.txt (Diff revision 1)
 
 
We usually do not use capitals in source code in kdelibs. (there are exceptions, but not in KIO).

Also, it would be better if this were called ratecontroller.cpp, not speed controller.
  1. acknowledged.
kioslave/ftp/ftp.cpp (Diff revision 1)
 
 
As David said, this is a GCC extension and not portable. Please use QByteArray.
kioslave/ftp/ftp.cpp (Diff revision 1)
 
 
No arbitrary numbers in the source code. I also don't like a waiting in the FTP code. I'd rather you moved the waiting to the rate controller and found a suitably-named function.
kioslave/ftp/speedController.h (Diff revision 1)
 
 
Rename to RateController.
  1. acknowledged.
kioslave/ftp/speedController.h (Diff revision 1)
 
 
Suggest renaming to nextReadBlockSize().
  1. acknowledged.
kioslave/ftp/speedController.h (Diff revision 1)
 
 
Use QElapserTimer, not QTime.
  1. acknowledged.
kioslave/ftp/speedController.h (Diff revision 1)
 
 
This will not work for kio_http. You need to limit the transfer rate from the ioslave to the application, in KIO::SlaveBase::data(), or in KIO::SlaveInterface::data().

You don't get access to the QTcpSocket that KIO::TCPSlaveBase uses.
  1. That is not quite correct. You do have access to the socket. See KIO::TCpSlaveBase::socket. Also controlling the data at SlaveBase::data level is not quite the same as controlling it at the socket level, is it ? Granted the socket will stop reading from the pipe when its read buffer is full.
  2. In support to Dawit:
    What I thought is when we control rate at socket level we are actually telling server not to send data at full rate. As we are not reading data from socket buffer at full rate, this will keep advertisement window in control and server will send data accordingly.
    
    In support to thaigo:
    Suppose you are not controlling at socket level, instead of it let say we are controlling one level above i.e. KIO::SlaveBase::data() then in that case also eventually after stabilization phase we will get our rate and server will send data at a rate at which we reading at data() level.
    
    As this was my first try, I thought let me see if my logic works in one small module like ftp slave. If it works at this level, I can try now with same logic at data() level. Now If we are trying at data() level then I need to control how many bytes to pass from within data() function.
    
    What you all say?
Christoph Feck
Tushar, did you have time yet to update the code to reflect the review comments?
  1. ya all are closed. I actually opened new request for it. As I was doing it first time, instead of updating this I added new request.
Loading...