{"thread":{"id":"28797","subject":"[PATCH] http.c: Use curl_multi_fdset to select on curl fds instead of just sleeping","startedAt":"2011-10-29T15:20:21Z","lastAt":"2011-11-02T09:17:54Z","messageCount":5,"participants":["Mika Fischer","Daniel Stenberg","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"178512","messageId":"1319901621-482-1-git-send-email-mika.fischer@zoopnet.de","threadId":"28797","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-10-29T15:20:21Z","receivedAt":"2011-10-29T15:20:21Z","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 \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":"178514","messageId":"alpine.DEB.2.00.1110292230500.28196@tvnag.unkk.fr","threadId":"28797","inReplyTo":"1319901621-482-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":"Daniel Stenberg","fromEmail":"daniel@haxx.se","sentAt":"2011-10-29T20:33:44Z","receivedAt":"2011-10-29T20:33:44Z","isPatch":true,"sender":{"key":"daniel@haxx.se","avatar":"https://gravatar.com/avatar/69fdca87edd17cee21ca2e79fc2ff671d644603c3dc27167430f3cd3dbab7ba8?d=mp&s=160"},"body":"On Sat, 29 Oct 2011, Mika Fischer wrote:\n\n> Previously, when nothing could be read from the connections curl had open, \n> git would just sleep unconditionally for 50ms. This patch changes this \n> behavior and instead obtains the recommended timeout and the actual file \n> descriptors from curl. This should eliminate time spent sleeping when data \n> could actually be read/written on the socket.\n\nIt looks fine to me, from a libcurl perspective. I only have one comment about \nthis:\n\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\nAt times, curl_multi_fdset() might return -1 in max_fd, as when there's no \ninternal socket around to provide to the application to wait for.\n\nCalling select() with max_fd+1 (== 0) will then not be appreciated by all \nimplementations of select() so that case should probably also be covered by \nthe 50ms sleep approach...\n\n-- \n\n / daniel.haxx.se\n"},{"id":"178524","messageId":"CAOs=hR+YuF+HP0n0132Ktm3RdeWsnVp0Bgt89LNn+VyT6W0mcw@mail.gmail.com","threadId":"28797","inReplyTo":"alpine.DEB.2.00.1110292230500.28196@tvnag.unkk.fr","subject":"Re: [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-10-30T07:19:23Z","receivedAt":"2011-10-30T07:19:23Z","isPatch":true,"sender":{"key":"mika.fischer@zoopnet.de","avatar":"https://avatars.githubusercontent.com/u/426158?v=4"},"body":"On Sat, Oct 29, 2011 at 22:33, Daniel Stenberg wrote:\n>> +                       curl_multi_fdset(curlm, &readfds, &writefds,\n>> &excfds, &max_fd);\n>> +\n>> +                       select(max_fd+1, &readfds, &writefds, &excfds,\n>> &select_timeout);\n>\n> At times, curl_multi_fdset() might return -1 in max_fd, as when there's no\n> internal socket around to provide to the application to wait for.\n>\n> Calling select() with max_fd+1 (== 0) will then not be appreciated by all\n> implementations of select() so that case should probably also be covered by\n> the 50ms sleep approach...\n\nActually, the 50ms sleep was also implemented using select(0, ...)\nbefore the patch. I tried to keep the previous behavior when curl does\nnot give us any information.\nI assumed that the select(0, ...) was some portable way to sleep with\nmicrosecond granularity.\nIs there some other way to tell select not to check any fds, or should\nI just call select(1, ...)?\n\nBest,\n Mika\n"},{"id":"178670","messageId":"CAOs=hR+u_MrHK4iNFZj4pLVhZ6-_75YpqN7tqWnSjh+di8Lzxw@mail.gmail.com","threadId":"28797","inReplyTo":"CAOs=hR+YuF+HP0n0132Ktm3RdeWsnVp0Bgt89LNn+VyT6W0mcw@mail.gmail.com","subject":"Re: [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-02T08:21:37Z","receivedAt":"2011-11-02T08:21:37Z","isPatch":true,"sender":{"key":"mika.fischer@zoopnet.de","avatar":"https://avatars.githubusercontent.com/u/426158?v=4"},"body":">> Calling select() with max_fd+1 (== 0) will then not be appreciated by all\n>> implementations of select() so that case should probably also be covered by\n>> the 50ms sleep approach...\n>\n> Actually, the 50ms sleep was also implemented using select(0, ...)\n> before the patch. I tried to keep the previous behavior when curl does\n> not give us any information.\n> I assumed that the select(0, ...) was some portable way to sleep with\n> microsecond granularity.\n\nUpon a bit of research, it seems that select(0, ...) is indeed quite\ncommonly used. So I'd just keep it as it was unless you know of a\nproblem it causes.\n\nSince I'm new here, I don't really know what the next steps are for\nthe patch, should I just wait? Or send it directly to someone?\n\nBest,\n Mika\n"},{"id":"178674","messageId":"7v39e6lw71.fsf@alter.siamese.dyndns.org","threadId":"28797","inReplyTo":"CAOs=hR+u_MrHK4iNFZj4pLVhZ6-_75YpqN7tqWnSjh+di8Lzxw@mail.gmail.com","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-02T09:17:54Z","receivedAt":"2011-11-02T09:17:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mika Fischer writes:\n\n> Since I'm new here, I don't really know what the next steps are for\n> the patch, should I just wait? Or send it directly to someone?\n\nResend to the list for re-evaluation, and then we can take it from there.\n\nThanks.\n"}]}