CURLOPT_BUFFERSIZE: support enlarging receive buffer #1222

Closed
wants to merge 2 commits into
from

Projects

None yet

3 participants

@richykim
Contributor

Replace use of fixed macro BUFSIZE to define the size of the receive buffer.
Reappropriate CURLOPT_BUFFERSIZE to allow enlarging receive buffer size.
Upon setting, resize buffer if larger than the current default size up to a
MAX_BUFSIZE (512KB). This can benefit protocols like SFTP.

@richykim richykim CURLOPT_BUFFERSIZE: support enlarging receive buffer
Replace use of fixed macro BUFSIZE to define the size of the receive buffer.
Reappropriate CURLOPT_BUFFERSIZE to include enlarging receive buffer size.
Upon setting, resize buffer if larger than the current default size up to a
MAX_BUFSIZE (512KB). This can benefit protocols like SFTP.
bed37a6
@bagder

The warnings is what causes the CI failures.

docs/libcurl/opts/CURLOPT_BUFFERSIZE.3
+gets called more often and with smaller chunks. Secondly, for some protocols,
+there's a benefit of having a larger buffer for performance. This is just
+treated as a request, not an order. You cannot be guaranteed to actually get
+the given size.
@bagder
bagder Jan 19, 2017 Member

Maybe add here that growing the buffer was added in 7.53.0?

lib/telnet.c
@@ -1309,6 +1309,7 @@ static CURLcode telnet_do(struct connectdata *conn, bool *done)
struct timeval now;
bool keepon = TRUE;
char *buf = data->state.buffer;
+ const size_t buf_size = CURL_BUFSIZE(data->set.buffer_size);
@bagder
bagder Jan 19, 2017 Member

compiler warning:

telnet.c:1312:16: warning: unused variable 'buf_size' [-Wunused-variable]

  const size_t buf_size = CURL_BUFSIZE(data->set.buffer_size);
lib/http.c
@@ -297,7 +297,7 @@ static CURLcode http_output_basic(struct connectdata *conn, bool proxy)
pwd = conn->passwd;
}
- snprintf(data->state.buffer, sizeof(data->state.buffer), "%s:%s", user, pwd);
+ snprintf(data->state.buffer, CURL_BUFSIZE(data->set.buffer_size), "%s:%s", user, pwd);
@bagder
bagder Jan 19, 2017 Member

checksrc warning:

./http.c:300:89: warning: Longer than 79 columns (LONGLINE)

   snprintf(data->state.buffer, CURL_BUFSIZE(data->set.buffer_size), "%s:%s", user, pwd);
lib/url.c
(data->set.buffer_size < 1))
- data->set.buffer_size = 0; /* huge internal default */
+ data->set.buffer_size = MAX_BUFSIZE; /* huge internal default */
@bagder
bagder Jan 19, 2017 Member

Isn't it weird to get 512KB if you ask for < 1 ? Wouldn't the default size be more reasonable then?

@richykim
richykim Jan 19, 2017 Contributor

Sure, default size is reasonable.

lib/url.c
+
+ /* Resize only if larger than default buffer size. */
+ if(data->set.buffer_size > BUFSIZE) {
+ data->state.buffer = realloc(data->state.buffer, data->set.buffer_size + 1);
@bagder
bagder Jan 19, 2017 Member

checksrc warning:

./url.c:2285:83: warning: Longer than 79 columns (LONGLINE)

       data->state.buffer = realloc(data->state.buffer, data->set.buffer_size + 1);
lib/urldata.h
@@ -201,6 +201,8 @@
/* Download buffer size, keep it fairly big for speed reasons */
#undef BUFSIZE
#define BUFSIZE CURL_MAX_WRITE_SIZE
+#define MAX_BUFSIZE 524288
@bagder
bagder Jan 19, 2017 Member

Do you have any thoughts on this specific limit? Why 512KB? For SFTP we've seen that even larger buffers help, mostly since we don't do any buffer sliding in the code so libcurl has to drain the entire buffer first before it can refill it.

Also, the max limit is not documented.

@richykim
richykim Jan 19, 2017 Contributor

I took 512KB as reference from https://daniel.haxx.se/blog/2014/05/14/why-sftp-is-still-slow-in-curl/
And in terms of adhoc testing that's where the performance was equivalent to sftp on my link.
But of course, I can change it to something bigger.

Should I add the max as CURL_MAX_READ_SIZE define here?
include/curl/curl.h
docs/libcurl/symbols-in-versions

@richykim
richykim Jan 19, 2017 Contributor

I tried 1MB buffer, and anecdotally I think it's diminishing returns.

16KB -> 0.6MB/s
512KB -> 9.4MB/s
1024KB -> 9.9MB/s

@bagder
bagder Jan 19, 2017 Member

Out of curiosity, what was the RTT to the server in this test?

@richykim
richykim Jan 23, 2017 Contributor

~55ms

@bagder
bagder Jan 23, 2017 edited Member

Each SFTP packet needs to get requested individually, and sent back. That means that to fill up 512KB it needs 55 milliseconds (if we rule out a lot of other complications). It certainly can't do it any faster at least. (Because of how curl always drains the entire buffer first before it asks for more data.)

MAX = (512*1024) / 0.055 = 9532509 bytes/sec

That seems to match your numbers for the 512KB buffer case, but since you can't go faster with a larger buffer, it might indicate that limit is then not in the client SFTP code and buffer anymore.

But if we say talk with a more remote server at for example 200ms RTT, the max speed with 512KB of data buffer is:

MAX = (512 * 1024)/0.200 = 2621440 bytes/sec

While a doubled buffer size should be able to double the speed:

MAX = (2* 512 * 1024)/0.200 = 5242880/sec
@richykim richykim Update docs and clean-up formatting
- add CURL_MAX_READ_SIZE to defined maximum receive buffer size
- update docs
- clean-up long lines
d8c7e5c
@jay
Member
jay commented Jan 19, 2017 edited

Could a large default buffer size cause a problem with embedded devices?

@richykim
Contributor

I'm sure a large default buffer could be adverse for certain platforms. However, this change doesn't change that; only gives the option of growing the receive buffer via CURLOPT_BUFFERSIZE.

@bagder
bagder approved these changes Jan 19, 2017 View changes

👍

@bagder bagder added a commit that closed this pull request Jan 19, 2017
@richykim @bagder richykim + bagder CURLOPT_BUFFERSIZE: support enlarging receive buffer
Replace use of fixed macro BUFSIZE to define the size of the receive
buffer.  Reappropriate CURLOPT_BUFFERSIZE to include enlarging receive
buffer size.  Upon setting, resize buffer if larger than the current
default size up to a MAX_BUFSIZE (512KB). This can benefit protocols
like SFTP.

Closes #1222
6b76166
@bagder bagder closed this in 6b76166 Jan 19, 2017
@bagder
Member
bagder commented Jan 19, 2017

Thanks a lot!

@bagder bagder added a commit that referenced this pull request Jan 19, 2017
@richykim @bagder richykim + bagder CURLOPT_BUFFERSIZE: support enlarging receive buffer
Replace use of fixed macro BUFSIZE to define the size of the receive
buffer.  Reappropriate CURLOPT_BUFFERSIZE to include enlarging receive
buffer size.  Upon setting, resize buffer if larger than the current
default size up to a MAX_BUFSIZE (512KB). This can benefit protocols
like SFTP.

Closes #1222
e63ffa5
@jkralik jkralik added a commit to jkralik/curl that referenced this pull request Jan 23, 2017
@richykim @jkralik richykim + jkralik CURLOPT_BUFFERSIZE: support enlarging receive buffer
Replace use of fixed macro BUFSIZE to define the size of the receive
buffer.  Reappropriate CURLOPT_BUFFERSIZE to include enlarging receive
buffer size.  Upon setting, resize buffer if larger than the current
default size up to a MAX_BUFSIZE (512KB). This can benefit protocols
like SFTP.

Closes #1222
ce8a3bb
@peterpih peterpih pushed a commit to railsnewbie257/curl that referenced this pull request Jan 24, 2017
@richykim @railsnewbie257 richykim + railsnewbie257 CURLOPT_BUFFERSIZE: support enlarging receive buffer
Replace use of fixed macro BUFSIZE to define the size of the receive
buffer.  Reappropriate CURLOPT_BUFFERSIZE to include enlarging receive
buffer size.  Upon setting, resize buffer if larger than the current
default size up to a MAX_BUFSIZE (512KB). This can benefit protocols
like SFTP.

Closes #1222
22a88a3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment