Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

limit-rate: When the download speed is limited to 15M per second, it cannot be limited! #2386

Closed
liupeidong0620 opened this issue Mar 15, 2018 · 4 comments
Closed

limit-rate: When the download speed is limited to 15M per second, it cannot be limited! #2386

liupeidong0620 opened this issue Mar 15, 2018 · 4 comments

Comments

@liupeidong0620
Copy link

liupeidong0620 commented Mar 15, 2018 •
edited

I did this (Just for download speed test)

Download file size:

[root@california src]# du -sh dyninst-testsuite-9.3.1-1.el7.x86_64.rpm
17M	dyninst-testsuite-9.3.1-1.el7.x86_64.rpm

This post-submission test(#2371)

[root@california src]# ./curl -V
curl 7.59.0-DEV (x86_64-pc-linux-gnu) libcurl/7.59.0-DEV OpenSSL/1.0.2h
Release-Date: [unreleased]
Protocols: dict file ftp ftps gopher http https imap imaps pop3 pop3s rtsp smb smbs smtp smtps telnet tftp
Features: AsynchDNS IPv6 Largefile NTLM NTLM_WB SSL TLS-SRP UnixSockets HTTPS-proxy

[root@california src]# time ./curl http://linux.mirrors.es.net/centos/7.4.1708/os/x86_64/Packages/dyninst-testsuite-9.3.1-1.el7.x86_64.rpm -O  --limit-rate 15M
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 16.2M  100 16.2M    0     0  48.5M      0 --:--:-- --:--:-- --:--:-- 48.5M

real	0m0.384s
user	0m0.046s
sys	0m0.117s

Real download time: real 0m0.384s(Test a few more times you will find!)

I expected the following

Correct performance

[root@california src]# curl -V
curl 7.35.0 (i686-pc-linux-gnu) libcurl/7.35.0 OpenSSL/1.0.2m zlib/1.2.7 c-ares/1.12.0 libidn/0.6.5
Protocols: dict file ftp ftps gopher http https imap imaps pop3 pop3s rtsp smtp smtps telnet tftp
Features: AsynchDNS IDN Largefile NTLM NTLM_WB SSL libz TLS-SRP

[root@california src]# time curl http://linux.mirrors.es.net/centos/7.4.1708/os/x86_64/Packages/dyninst-testsuite-9.3.1-1.el7.x86_64.rpm -O  --limit-rate 15M
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 16.2M  100 16.2M    0     0  14.9M      0  0:00:01  0:00:01 --:--:-- 14.9M

real	0m1.123s
user	0m0.010s
sys	0m0.032s

Real download time: real 0m1.123s

Modify the code:

void Curl_pgrsSetDownloadCounter(struct Curl_easy *data, curl_off_t size)
{
  struct curltime now = Curl_now();

  data->progress.downloaded = size;

  /* download speed limit */
  if((data->set.max_recv_speed > 0) &&
     (Curl_pgrsLimitWaitTime(data->progress.downloaded,
                             data->progress.dl_limit_size,
                             data->set.max_recv_speed,
                             data->progress.dl_limit_start,
                             now) == 0)) {
    /* ====================== Added code  ==================*/
    if((size > 0) && \
       ((size - data->progress.dl_limit_size) < data->set.max_recv_speed)) {
       return;
    }
    /* ====================================================*/
    data->progress.dl_limit_start = now;
    data->progress.dl_limit_size = size;
  }
}

Limit frequent update parameters.what do you think?

（void Curl_pgrsSetUploadCounter(struct Curl_easy *data, curl_off_t size)
Upload speed limit code should also be modified.）

execute the program:

[root@california src]# ./curl -V
curl 7.59.0-DEV (x86_64-pc-linux-gnu) libcurl/7.59.0-DEV OpenSSL/1.0.2h
Release-Date: [unreleased]
Protocols: dict file ftp ftps gopher http https imap imaps pop3 pop3s rtsp smb smbs smtp smtps telnet tftp
Features: AsynchDNS IPv6 Largefile NTLM NTLM_WB SSL TLS-SRP UnixSockets HTTPS-proxy


[root@california src]# time ./curl http://linux.mirrors.es.net/centos/7.4.1708/os/x86_64/Packages/dyninst-testsuite-9.3.1-1.el7.x86_64.rpm -O  --limit-rate 15M
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 16.2M  100 16.2M    0     0  14.9M      0  0:00:01  0:00:01 --:--:-- 14.9M

real	0m1.136s
user	0m0.011s
sys	0m0.034s

Real download time: real 0m1.136s

operating system

Linux california 4.13.2-1.el7.elrepo.x86_64 #1 SMP Wed Sep 13 18:48:00 EDT 2017 x86_64 x86_64 x86_64 GNU/Linux
@bagder
Copy link
Member

bagder commented Mar 15, 2018

I cannot reproduce with current master (7.59.0 release basically, in particular it includes commit 72a0f62) using a 17MB or 8GB file, downloaded from localhost.

@liupeidong0620
Copy link
Author

Repeat the test several times:

There is such a situation:

[root@california src]# ./curl -V
curl 7.59.0 (x86_64-pc-linux-gnu) libcurl/7.59.0 OpenSSL/1.0.2h
Release-Date: 2018-03-14
Protocols: dict file ftp ftps gopher http https imap imaps pop3 pop3s rtsp smb smbs smtp smtps telnet tftp
Features: AsynchDNS IPv6 Largefile NTLM NTLM_WB SSL TLS-SRP UnixSockets HTTPS-proxy

[root@california src]# time ./curl http://linux.mirrors.es.net/centos/7.4.1708/os/x86_64/Packages/dyninst-testsuite-9.3.1-1.el7.x86_64.rpm -O  --limit-rate 15M
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 16.2M  100 16.2M    0     0  52.4M      0 --:--:-- --:--:-- --:--:-- 52.4M

real	0m0.358s
user	0m0.087s
sys	0m0.063s

@liupeidong0620
Copy link
Author

You said I tested，Really cannot be reproduce. There is indeed this problem on the network

@bagder
Copy link
Member

bagder commented Mar 15, 2018

I think this happens because dl_limit_start is updated too often so it checks very very small pieces of the transfer against the rate and that's not very good. I think we should make sure that we always compare against a slightly longer time period, like perhaps a minimum of 3 seconds. PR coming.

@liupeidong0620 liupeidong0620 mentioned this issue Mar 16, 2018
Closed
@bagder bagder mentioned this issue Mar 16, 2018
Closed
@bagder bagder closed this as completed in f5700ea Mar 16, 2018
greearb pushed a commit to greearb/curl that referenced this issue Mar 19, 2018
@bagder @greearb
rate-limit: use three second window to better handle high speeds
78a4dff 
Due to very frequent updates of the rate limit "window", it could
attempt to rate limit within the same milliseconds and that then made
the calculations wrong, leading to it not behaving correctly on very
fast transfers.

This new logic updates the rate limit "window" to be no shorter than the
last three seconds and only updating the timestamps for this when
switching between the states TOOFAST/PERFORM.

Reported-by: 刘佩东
Fixes curl#2386
Closes curl#2388
@lock lock bot locked as resolved and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@bagder @liupeidong0620