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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 2, 2011, 18:29 UTC
Message-ID: <7v62j2js3p.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1320230734-5933-1-git-send-email-mika.fischer@zoopnet.de>

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

Show 8 quoted lines

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

Show 12 quoted lines

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

Show 6 quoted lines

> @@ -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?

Show 19 quoted lines

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

Show 5 quoted lines

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