{"thread":{"id":"28821","subject":"[PATCH] http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping","startedAt":"2011-11-02T10:45:34Z","lastAt":"2011-11-02T19:34:19Z","messageCount":3,"participants":["Mika Fischer","Junio C Hamano","Daniel Stenberg"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"178681","messageId":"1320230734-5933-1-git-send-email-mika.fischer@zoopnet.de","threadId":"28821","inReplyTo":null,"subject":"[PATCH] http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping","fromName":"Mika Fischer","fromEmail":"mika.fischer@zoopnet.de","sentAt":"2011-11-02T10:45:34Z","receivedAt":"2011-11-02T10:45:34Z","isPatch":true,"sender":{"key":"mika.fischer@zoopnet.de","avatar":"https://avatars.githubusercontent.com/u/426158?v=4"},"body":"Previously, when nothing could be read from the connections curl had\nopen, git would just sleep unconditionally for 50ms. This patch changes\nthis behavior and instead obtains the recommended timeout and the actual\nfile descriptors from curl. This should eliminate time spent sleeping when\ndata could actually be read/written on the socket.\n\nSigned-off-by: Mika Fischer <mika.fischer@zoopnet.de>\n---\n http.c |   21 ++++++++++++++++-----\n 1 files changed, 16 insertions(+), 5 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex a4bc770..12180f3 100644\n--- a/http.c\n+++ b/http.c\n@@ -649,6 +649,7 @@ void run_active_slot(struct active_request_slot *slot)\n \tfd_set excfds;\n \tint max_fd;\n \tstruct timeval select_timeout;\n+\tlong int curl_timeout;\n \tint finished = 0;\n \n \tslot->finished = &finished;\n@@ -664,14 +665,24 @@ void run_active_slot(struct active_request_slot *slot)\n \t\t}\n \n \t\tif (slot->in_use && !data_received) {\n-\t\t\tmax_fd = 0;\n+\t\t\tcurl_multi_timeout(curlm, &curl_timeout);\n+\t\t\tif (curl_timeout == 0) {\n+\t\t\t\tcontinue;\n+\t\t\t} else if (curl_timeout == -1) {\n+\t\t\t\tselect_timeout.tv_sec  = 0;\n+\t\t\t\tselect_timeout.tv_usec = 50000;\n+\t\t\t} else {\n+\t\t\t\tselect_timeout.tv_sec  =  curl_timeout / 1000;\n+\t\t\t\tselect_timeout.tv_usec = (curl_timeout % 1000) * 1000;\n+\t\t\t}\n+\n+\t\t\tmax_fd = -1;\n \t\t\tFD_ZERO(&readfds);\n \t\t\tFD_ZERO(&writefds);\n \t\t\tFD_ZERO(&excfds);\n-\t\t\tselect_timeout.tv_sec = 0;\n-\t\t\tselect_timeout.tv_usec = 50000;\n-\t\t\tselect(max_fd, &readfds, &writefds,\n-\t\t\t       &excfds, &select_timeout);\n+\t\t\tcurl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);\n+\n+\t\t\tselect(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);\n \t\t}\n \t}\n #else\n-- \n1.7.7.1.489.g1fee\n"},{"id":"178707","messageId":"7v62j2js3p.fsf@alter.siamese.dyndns.org","threadId":"28821","inReplyTo":"1320230734-5933-1-git-send-email-mika.fischer@zoopnet.de","subject":"Re: [PATCH] http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-02T18:29:14Z","receivedAt":"2011-11-02T18:29:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mika Fischer <mika.fischer@zoopnet.de> writes:\n\n> Previously, when nothing could be read from the connections curl had\n> open, git would just sleep unconditionally for 50ms. This patch changes\n> this behavior and instead obtains the recommended timeout and the actual\n> file descriptors from curl. This should eliminate time spent sleeping when\n> data could actually be read/written on the socket.\n>\n> Signed-off-by: Mika Fischer <mika.fischer@zoopnet.de>\n> ---\n\nThanks. I added Daniel back to Cc: list as I know he is the area expert\nwhen it comes to the use of libcurl, and also his input helped to polish\nthis patch during the initial round of discussion.\n\n>  http.c |   21 ++++++++++++++++-----\n>  1 files changed, 16 insertions(+), 5 deletions(-)\n>\n> diff --git a/http.c b/http.c\n> index a4bc770..12180f3 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -649,6 +649,7 @@ void run_active_slot(struct active_request_slot *slot)\n>  \tfd_set excfds;\n>  \tint max_fd;\n>  \tstruct timeval select_timeout;\n> +\tlong int curl_timeout;\n\nJust a style nit, but we usually spell this \"long\" not \"long int\" in our\ncodebase.\n\n> @@ -664,14 +665,24 @@ void run_active_slot(struct active_request_slot *slot)\n>  \t\t}\n>  \n>  \t\tif (slot->in_use && !data_received) {\n> -\t\t\tmax_fd = 0;\n> +\t\t\tcurl_multi_timeout(curlm, &curl_timeout);\n\nAccording to http://curl.haxx.se/libcurl/c/curl_multi_timeout.html\nthis was added in 7.15.4 which may be much newer than some of the versions\nthe existing code checks LIBCURL_VERSION_NUM against (grep for it in http.c).\n\nShouldn't you make this conditional?\n\n> +\t\t\tif (curl_timeout == 0) {\n> +\t\t\t\tcontinue;\n> +\t\t\t} else if (curl_timeout == -1) {\n> +\t\t\t\tselect_timeout.tv_sec  = 0;\n> +\t\t\t\tselect_timeout.tv_usec = 50000;\n> +\t\t\t} else {\n> +\t\t\t\tselect_timeout.tv_sec  =  curl_timeout / 1000;\n> +\t\t\t\tselect_timeout.tv_usec = (curl_timeout % 1000) * 1000;\n> +\t\t\t}\n> +\n> +\t\t\tmax_fd = -1;\n>  \t\t\tFD_ZERO(&readfds);\n>  \t\t\tFD_ZERO(&writefds);\n>  \t\t\tFD_ZERO(&excfds);\n> -\t\t\tselect_timeout.tv_sec = 0;\n> -\t\t\tselect_timeout.tv_usec = 50000;\n> -\t\t\tselect(max_fd, &readfds, &writefds,\n> -\t\t\t       &excfds, &select_timeout);\n> +\t\t\tcurl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);\n\nI couldn't find in http://curl.haxx.se/libcurl/c/curl_multi_fdset.html\nwhat the version requirement for using this function is, but the same\ncomment as above applies here.\n\nBy the way, I think I saw Daniel posting a link to a nicely formatted\ntable that lists each and every functions and CURLOPT_* symbol with\nranges of version it is usable, but I seem to be unable to find it.\n\n> +\n> +\t\t\tselect(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);\n>  \t\t}\n>  \t}\n>  #else\n"},{"id":"178711","messageId":"alpine.DEB.2.00.1111022026390.7774@tvnag.unkk.fr","threadId":"28821","inReplyTo":"7v62j2js3p.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping","fromName":"Daniel Stenberg","fromEmail":"daniel@haxx.se","sentAt":"2011-11-02T19:34:19Z","receivedAt":"2011-11-02T19:34:19Z","isPatch":true,"sender":{"key":"daniel@haxx.se","avatar":"https://gravatar.com/avatar/69fdca87edd17cee21ca2e79fc2ff671d644603c3dc27167430f3cd3dbab7ba8?d=mp&s=160"},"body":"On Wed, 2 Nov 2011, Junio C Hamano wrote:\n\nI'm totally fine with the patch's approach with respect to how it uses libcurl \nand it should be an improvement compared to the previous way.\n\n>> + curl_multi_fdset(curlm, &readfds, &writefds, &excfds, &max_fd);\n>\n> I couldn't find in http://curl.haxx.se/libcurl/c/curl_multi_fdset.html\n> what the version requirement for using this function is, but the same\n> comment as above applies here.\n\nThat function has been around for as long as the multi interface has, so it \nshould be safe to use it just as widely as curl_multi_perform().\n\n> By the way, I think I saw Daniel posting a link to a nicely formatted table \n> that lists each and every functions and CURLOPT_* symbol with ranges of \n> version it is usable, but I seem to be unable to find it.\n\nRight, the document is a bit hard to find and I should figure out a more \nprominent place to link to it. But it can be found here:\n\nhttps://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-versions\n\n-- \n\n  / daniel.haxx.se\n"}]}