[PATCH] http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping

[PATCH] http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping

[Mika Fischer]

Previously, when nothing could be read from the connections curl had
open, git would just sleep unconditionally for 50ms. This patch changes
this behavior and instead obtains the recommended timeout and the actual
file descriptors from curl. This should eliminate time spent sleeping when
data could actually be read/written on the socket.

Signed-off-by: Mika Fischer <mika.fischer@zoopnet.de>
---
 http.c |   21 ++++++++++++++++-----
 1 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/http.c b/http.c
index a4bc770..12180f3 100644
--- a/http.c
+++ b/http.c
@@ -649,6 +649,7 @@ void run_active_slot(struct active_request_slot *slot)
 	fd_set excfds;
 	int max_fd;
 	struct timeval select_timeout;
+	long int curl_timeout;
 	int finished = 0;
 
 	slot->finished = &finished;
@@ -664,14 +665,24 @@ void run_active_slot(struct active_request_slot *slot)
 		}
 
 		if (slot->in_use && !data_received) {
-			max_fd = 0;
+			curl_multi_timeout(curlm, &curl_timeout);
+			if (curl_timeout == 0) {
+				continue;
+			} else if (curl_timeout == -1) {
+				select_timeout.tv_sec  = 0;
+				select_timeout.tv_usec = 50000;
+			} else {
+				select_timeout.tv_sec  =  curl_timeout / 1000;
+				select_timeout.tv_usec = (curl_timeout % 1000) * 1000;
+			}
+
+			max_fd = -1;
 			FD_ZERO(&readfds);
 			FD_ZERO(&writefds);
 			FD_ZERO(&excfds);
-			select_timeout.tv_sec = 0;
-			select_timeout.tv_usec = 50000;
-			select(max_fd, &readfds, &writefds,
-			       &excfds, &select_timeout);
+			curl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);
+
+			select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
 		}
 	}
 #else
-- 
1.7.7.1.489.g1fee

Re: [PATCH] http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping

[Daniel Stenberg]

On Sat, 29 Oct 2011, Mika Fischer wrote:

> Previously, when nothing could be read from the connections curl had open, 
> git would just sleep unconditionally for 50ms. This patch changes this 
> behavior and instead obtains the recommended timeout and the actual file 
> descriptors from curl. This should eliminate time spent sleeping when data 
> could actually be read/written on the socket.

It looks fine to me, from a libcurl perspective. I only have one comment about 
this:

> +			curl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);
> +
> +			select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);

At times, curl_multi_fdset() might return -1 in max_fd, as when there's no 
internal socket around to provide to the application to wait for.

Calling select() with max_fd+1 (== 0) will then not be appreciated by all 
implementations of select() so that case should probably also be covered by 
the 50ms sleep approach...

-- 

  / daniel.haxx.se

Re: [PATCH] http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping

[Mika Fischer]

On Sat, Oct 29, 2011 at 22:33, Daniel Stenberg <daniel@haxx.se> wrote:
>> +                       curl_multi_fdset(curlm, &readfds, &writefds,
>> &excfds, &max_fd);
>> +
>> +                       select(max_fd+1, &readfds, &writefds, &excfds,
>> &select_timeout);
>
> At times, curl_multi_fdset() might return -1 in max_fd, as when there's no
> internal socket around to provide to the application to wait for.
>
> Calling select() with max_fd+1 (== 0) will then not be appreciated by all
> implementations of select() so that case should probably also be covered by
> the 50ms sleep approach...

Actually, the 50ms sleep was also implemented using select(0, ...)
before the patch. I tried to keep the previous behavior when curl does
not give us any information.
I assumed that the select(0, ...) was some portable way to sleep with
microsecond granularity.
Is there some other way to tell select not to check any fds, or should
I just call select(1, ...)?

Best,
 Mika

Re: [PATCH] http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping

[Mika Fischer]

>> Calling select() with max_fd+1 (== 0) will then not be appreciated by all
>> implementations of select() so that case should probably also be covered by
>> the 50ms sleep approach...
>
> Actually, the 50ms sleep was also implemented using select(0, ...)
> before the patch. I tried to keep the previous behavior when curl does
> not give us any information.
> I assumed that the select(0, ...) was some portable way to sleep with
> microsecond granularity.

Upon a bit of research, it seems that select(0, ...) is indeed quite
commonly used. So I'd just keep it as it was unless you know of a
problem it causes.

Since I'm new here, I don't really know what the next steps are for
the patch, should I just wait? Or send it directly to someone?

Best,
 Mika

Re: [PATCH] http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping

[Junio C Hamano]

Mika Fischer <mika.fischer@zoopnet.de> writes:

> Since I'm new here, I don't really know what the next steps are for
> the patch, should I just wait? Or send it directly to someone?

Resend to the list for re-evaluation, and then we can take it from there.

Thanks.

[PATCH] http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping

[Mika Fischer]

Previously, when nothing could be read from the connections curl had
open, git would just sleep unconditionally for 50ms. This patch changes
this behavior and instead obtains the recommended timeout and the actual
file descriptors from curl. This should eliminate time spent sleeping when
data could actually be read/written on the socket.

Signed-off-by: Mika Fischer <mika.fischer@zoopnet.de>
---
 http.c |   21 ++++++++++++++++-----
 1 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/http.c b/http.c
index a4bc770..12180f3 100644
--- a/http.c
+++ b/http.c
@@ -649,6 +649,7 @@ void run_active_slot(struct active_request_slot *slot)
 	fd_set excfds;
 	int max_fd;
 	struct timeval select_timeout;
+	long int curl_timeout;
 	int finished = 0;
 
 	slot->finished = &finished;
@@ -664,14 +665,24 @@ void run_active_slot(struct active_request_slot *slot)
 		}
 
 		if (slot->in_use && !data_received) {
-			max_fd = 0;
+			curl_multi_timeout(curlm, &curl_timeout);
+			if (curl_timeout == 0) {
+				continue;
+			} else if (curl_timeout == -1) {
+				select_timeout.tv_sec  = 0;
+				select_timeout.tv_usec = 50000;
+			} else {
+				select_timeout.tv_sec  =  curl_timeout / 1000;
+				select_timeout.tv_usec = (curl_timeout % 1000) * 1000;
+			}
+
+			max_fd = -1;
 			FD_ZERO(&readfds);
 			FD_ZERO(&writefds);
 			FD_ZERO(&excfds);
-			select_timeout.tv_sec = 0;
-			select_timeout.tv_usec = 50000;
-			select(max_fd, &readfds, &writefds,
-			       &excfds, &select_timeout);
+			curl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);
+
+			select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
 		}
 	}
 #else
-- 
1.7.7.1.489.g1fee

Re: [PATCH] http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping

[Junio C Hamano]

Mika Fischer <mika.fischer@zoopnet.de> writes:

> Previously, when nothing could be read from the connections curl had
> open, git would just sleep unconditionally for 50ms. This patch changes
> this behavior and instead obtains the recommended timeout and the actual
> file descriptors from curl. This should eliminate time spent sleeping when
> data could actually be read/written on the socket.
>
> Signed-off-by: Mika Fischer <mika.fischer@zoopnet.de>
> ---

Thanks. I added Daniel back to Cc: list as I know he is the area expert
when it comes to the use of libcurl, and also his input helped to polish
this patch during the initial round of discussion.

>  http.c |   21 ++++++++++++++++-----
>  1 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/http.c b/http.c
> index a4bc770..12180f3 100644
> --- a/http.c
> +++ b/http.c
> @@ -649,6 +649,7 @@ void run_active_slot(struct active_request_slot *slot)
>  	fd_set excfds;
>  	int max_fd;
>  	struct timeval select_timeout;
> +	long int curl_timeout;

Just a style nit, but we usually spell this "long" not "long int" in our
codebase.

> @@ -664,14 +665,24 @@ void run_active_slot(struct active_request_slot *slot)
>  		}
>  
>  		if (slot->in_use && !data_received) {
> -			max_fd = 0;
> +			curl_multi_timeout(curlm, &curl_timeout);

According to http://curl.haxx.se/libcurl/c/curl_multi_timeout.html
this was added in 7.15.4 which may be much newer than some of the versions
the existing code checks LIBCURL_VERSION_NUM against (grep for it in http.c).

Shouldn't you make this conditional?

> +			if (curl_timeout == 0) {
> +				continue;
> +			} else if (curl_timeout == -1) {
> +				select_timeout.tv_sec  = 0;
> +				select_timeout.tv_usec = 50000;
> +			} else {
> +				select_timeout.tv_sec  =  curl_timeout / 1000;
> +				select_timeout.tv_usec = (curl_timeout % 1000) * 1000;
> +			}
> +
> +			max_fd = -1;
>  			FD_ZERO(&readfds);
>  			FD_ZERO(&writefds);
>  			FD_ZERO(&excfds);
> -			select_timeout.tv_sec = 0;
> -			select_timeout.tv_usec = 50000;
> -			select(max_fd, &readfds, &writefds,
> -			       &excfds, &select_timeout);
> +			curl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);

I couldn't find in http://curl.haxx.se/libcurl/c/curl_multi_fdset.html
what the version requirement for using this function is, but the same
comment as above applies here.

By the way, I think I saw Daniel posting a link to a nicely formatted
table that lists each and every functions and CURLOPT_* symbol with
ranges of version it is usable, but I seem to be unable to find it.

> +
> +			select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
>  		}
>  	}
>  #else

Re: [PATCH] http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping

[Daniel Stenberg]

On Wed, 2 Nov 2011, Junio C Hamano wrote:

I'm totally fine with the patch's approach with respect to how it uses libcurl 
and it should be an improvement compared to the previous way.

>> + curl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);
>
> I couldn't find in http://curl.haxx.se/libcurl/c/curl_multi_fdset.html
> what the version requirement for using this function is, but the same
> comment as above applies here.

That function has been around for as long as the multi interface has, so it 
should be safe to use it just as widely as curl_multi_perform().

> By the way, I think I saw Daniel posting a link to a nicely formatted table 
> that lists each and every functions and CURLOPT_* symbol with ranges of 
> version it is usable, but I seem to be unable to find it.

Right, the document is a bit hard to find and I should figure out a more 
prominent place to link to it. But it can be found here:

https://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-versions

-- 

  / daniel.haxx.se