{"thread":{"id":"10551","subject":"[PATCH] Show total transferred as part of throughput progress display","startedAt":"2007-11-01T03:57:04Z","lastAt":"2007-11-01T14:18:49Z","messageCount":2,"participants":["Shawn O. Pearce","Nicolas Pitre"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"57774","messageId":"20071101035704.GA4518@spearce.org","threadId":"10551","inReplyTo":null,"subject":"[PATCH] Show total transferred as part of throughput progress display","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-11-01T03:57:04Z","receivedAt":"2007-11-01T03:57:04Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Right now it is infeasible to offer to the user a reasonable concept\nof when a clone will be complete as we aren't able to come up with\nthe final pack size until after we have actually transferred the\nentire thing to the client.  However in many cases users can work\nwith a rough rule-of-thumb; for example it is somewhat well known\nthat git.git is about 16 MiB today and that linux-2.6.git is over\n120 MiB.\n\nWe now show the total amount of data we have transferred over\nthe network as part of the throughput meter, organizing it in\n\"human friendly\" terms like `ls -h` would do.  Users can glance at\nthis, see that the total transferred size is about 3 MiB, see the\nthroughput of X KiB/sec, and determine a reasonable figure of about\nwhen the clone will be complete, assuming they know the rough size\nof the source repository or are able to obtain it.\n\nThis is also a helpful indicator that there is progress being made\neven if we stall on a very large object.  The thoughput meter may\nremain relatively constant and the percentage complete and object\ncount won't be changing, but the total transferred will be increasing\nas additional data is received for this object.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n\n This follows on top of Nico's 5 patch series to add the thoughput\n meter.  I think its a useful addition.\n\n progress.c |   32 +++++++++++++++++++++++++++++---\n 1 files changed, 29 insertions(+), 3 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 23ee9f3..5c95091 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -11,7 +11,11 @@ struct throughput {\n \tunsigned int avg_misecs;\n \tunsigned int last_misecs[TP_IDX_MAX];\n \tunsigned int idx;\n-\tchar display[20];\n+\tunsigned int unit_size;\n+\tunsigned int unit_index;\n+\tunsigned int total_units;\n+\tunsigned int curr_bytes;\n+\tchar display[40];\n };\n \n struct progress {\n@@ -24,6 +28,7 @@ struct progress {\n \tstruct throughput *throughput;\n };\n \n+static const char *units[] = {\"bytes\", \"KiB\", \"MiB\", \"GiB\"};\n static volatile sig_atomic_t progress_update;\n \n static void progress_interval(int signum)\n@@ -113,12 +118,27 @@ void display_throughput(struct progress *progress, unsigned long n)\n \n \tif (!tp) {\n \t\tprogress->throughput = tp = calloc(1, sizeof(*tp));\n-\t\tif (tp)\n+\t\tif (tp) {\n \t\t\ttp->prev_tv = tv;\n+\t\t\ttp->unit_size = 1;\n+\t\t}\n \t\treturn;\n \t}\n \n \ttp->count += n;\n+\ttp->curr_bytes += n;\n+\tif (tp->curr_bytes > tp->unit_size) {\n+\t\ttp->total_units += tp->curr_bytes / tp->unit_size;\n+\t\ttp->curr_bytes = tp->curr_bytes % tp->unit_size;\n+\n+\t\twhile (tp->total_units >= 1024\n+\t\t\t&& tp->unit_index < ARRAY_SIZE(units)) {\n+\t\t\ttp->curr_bytes += (tp->total_units % 1024) * tp->unit_size;\n+\t\t\ttp->total_units = tp->total_units / 1024;\n+\t\t\ttp->unit_size *= 1024;\n+\t\t\ttp->unit_index++;\n+\t\t}\n+\t}\n \n \t/*\n \t * We have x = bytes and y = microsecs.  We want z = KiB/s:\n@@ -143,7 +163,13 @@ void display_throughput(struct progress *progress, unsigned long n)\n \t\ttp->avg_bytes += tp->count;\n \t\ttp->avg_misecs += misecs;\n \t\tsnprintf(tp->display, sizeof(tp->display),\n-\t\t\t \", %lu KiB/s\", tp->avg_bytes / tp->avg_misecs);\n+\t\t\t \", %3u.%2.2u %s     %lu KiB/s\",\n+\t\t\t tp->total_units,\n+\t\t\t tp->unit_size > 1\n+\t\t\t\t? tp->curr_bytes / (tp->unit_size / 100)\n+\t\t\t\t: 0,\n+\t\t\t units[tp->unit_index],\n+\t\t\t tp->avg_bytes / tp->avg_misecs);\n \t\ttp->avg_bytes -= tp->last_bytes[tp->idx];\n \t\ttp->avg_misecs -= tp->last_misecs[tp->idx];\n \t\ttp->last_bytes[tp->idx] = tp->count;\n-- \n1.5.3.4.1481.g854da\n"},{"id":"57826","messageId":"alpine.LFD.0.9999.0711010814140.21255@xanadu.home","threadId":"10551","inReplyTo":"20071101035704.GA4518@spearce.org","subject":"Re: [PATCH] Show total transferred as part of throughput progress display","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-11-01T14:18:49Z","receivedAt":"2007-11-01T14:18:49Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 31 Oct 2007, Shawn O. Pearce wrote:\n\n> We now show the total amount of data we have transferred over\n> the network as part of the throughput meter, organizing it in\n> \"human friendly\" terms like `ls -h` would do.  Users can glance at\n> this, see that the total transferred size is about 3 MiB, see the\n> throughput of X KiB/sec, and determine a reasonable figure of about\n> when the clone will be complete, assuming they know the rough size\n> of the source repository or are able to obtain it.\n\nWell, OK.  But...\n\n> This is also a helpful indicator that there is progress being made\n> even if we stall on a very large object.  The thoughput meter may\n> remain relatively constant and the percentage complete and object\n> count won't be changing, but the total transferred will be increasing\n> as additional data is received for this object.\n\nCurrently if the object count is unchanged for an extended period of \ntime, the display_progress() function isn't called at all, so the \nupdated byte count (and rate) won't be displayed either.  This needs a \nfix, probably in a separate patch which I'll send right away.\n\n> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>\n\nWell, your patch has issues. Please see comments below.\n\n> diff --git a/progress.c b/progress.c\n> index 23ee9f3..5c95091 100644\n> --- a/progress.c\n> +++ b/progress.c\n> @@ -11,7 +11,11 @@ struct throughput {\n>  \tunsigned int avg_misecs;\n>  \tunsigned int last_misecs[TP_IDX_MAX];\n>  \tunsigned int idx;\n> -\tchar display[20];\n> +\tunsigned int unit_size;\n> +\tunsigned int unit_index;\n> +\tunsigned int total_units;\n> +\tunsigned int curr_bytes;\n> +\tchar display[40];\n>  };\n>  \n>  struct progress {\n> @@ -24,6 +28,7 @@ struct progress {\n>  \tstruct throughput *throughput;\n>  };\n>  \n> +static const char *units[] = {\"bytes\", \"KiB\", \"MiB\", \"GiB\"};\n>  static volatile sig_atomic_t progress_update;\n>  \n>  static void progress_interval(int signum)\n> @@ -113,12 +118,27 @@ void display_throughput(struct progress *progress, unsigned long n)\n>  \n>  \tif (!tp) {\n>  \t\tprogress->throughput = tp = calloc(1, sizeof(*tp));\n> -\t\tif (tp)\n> +\t\tif (tp) {\n>  \t\t\ttp->prev_tv = tv;\n> +\t\t\ttp->unit_size = 1;\n> +\t\t}\n>  \t\treturn;\n>  \t}\n>  \n>  \ttp->count += n;\n> +\ttp->curr_bytes += n;\n> +\tif (tp->curr_bytes > tp->unit_size) {\n> +\t\ttp->total_units += tp->curr_bytes / tp->unit_size;\n> +\t\ttp->curr_bytes = tp->curr_bytes % tp->unit_size;\n> +\n> +\t\twhile (tp->total_units >= 1024\n> +\t\t\t&& tp->unit_index < ARRAY_SIZE(units)) {\n> +\t\t\ttp->curr_bytes += (tp->total_units % 1024) * tp->unit_size;\n> +\t\t\ttp->total_units = tp->total_units / 1024;\n> +\t\t\ttp->unit_size *= 1024;\n> +\t\t\ttp->unit_index++;\n> +\t\t}\n> +\t}\n\nThis whole block could be moved inside the \"if (misecs > 512)\" \nconditional.  There is no point performing that computation over and \nover when the display isn't updated more than twice a second anyway.\n\n> @@ -143,7 +163,13 @@ void display_throughput(struct progress *progress, unsigned long n)\n>  \t\ttp->avg_bytes += tp->count;\n>  \t\ttp->avg_misecs += misecs;\n>  \t\tsnprintf(tp->display, sizeof(tp->display),\n> -\t\t\t \", %lu KiB/s\", tp->avg_bytes / tp->avg_misecs);\n> +\t\t\t \", %3u.%2.2u %s     %lu KiB/s\",\n\nThe line is becoming quite long already, and I'd prefer if remote \nmessages like the pack-objects summary kept overwriting it entirely \nwhile this line is bumped to the next line on screen for a prettier \ndisplay.  So I'd prefer if there wasn't so many spaces inserted there.  \nWhat about something like \", %u.%2.2u %s | %lu KiB/s\" instead?\n\nAlso I think that displaying fractional bytes, even if it is always 0, \nis a bit weird.\n\n> +\t\t\t tp->total_units,\n> +\t\t\t tp->unit_size > 1\n> +\t\t\t\t? tp->curr_bytes / (tp->unit_size / 100)\n\nThis has integer truncation problems.  Suppose tp->unit_size = 1024 and\ntp->curr_bytes = 1023.  You get 1023 / (1024 / 100) = 102 while the\ndesired result should be 99.\n\nOverall I think your patch is also needlessly too complex.\nthis could be implemented in a much simpler way, such as follows:\n\ndiff --git a/progress.c b/progress.c\nindex 34a5961..1212be8 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -15,13 +15,14 @@\n \n struct throughput {\n \tstruct timeval prev_tv;\n+\tsize_t total;\n \tunsigned long count;\n \tunsigned long avg_bytes;\n \tunsigned long last_bytes[TP_IDX_MAX];\n \tunsigned int avg_misecs;\n \tunsigned int last_misecs[TP_IDX_MAX];\n \tunsigned int idx;\n-\tchar display[20];\n+\tchar display[32];\n };\n \n struct progress {\n@@ -128,6 +129,7 @@ void display_throughput(struct progress *progress, unsigned long n)\n \t\treturn;\n \t}\n \n+\ttp->total += n;\n \ttp->count += n;\n \n \t/*\n@@ -149,11 +151,32 @@ void display_throughput(struct progress *progress, unsigned long n)\n \tmisecs += (int)(tv.tv_usec - tp->prev_tv.tv_usec) / 977;\n \n \tif (misecs > 512) {\n+\t\tunsigned l = sizeof(tp->display);\n \t\ttp->prev_tv = tv;\n \t\ttp->avg_bytes += tp->count;\n \t\ttp->avg_misecs += misecs;\n-\t\tsnprintf(tp->display, sizeof(tp->display),\n-\t\t\t \", %lu KiB/s\", tp->avg_bytes / tp->avg_misecs);\n+\n+\t\tif (tp->total > 1 << 30) {\n+\t\t\tl -= snprintf(tp->display, l, \", %u.%2.2u GiB\",\n+\t\t\t\t      (int)(tp->total >> 30),\n+\t\t\t\t      (int)(tp->total & ((1 << 30) - 1)) / 10737419);\n+\t\t} else if (tp->total > 1 << 20) {\n+\t\t\tl -= snprintf(tp->display, l, \", %u.%2.2u MiB\",\n+\t\t\t\t      (int)(tp->total >> 20),\n+\t\t\t\t      ((int)(tp->total & ((1 << 20) - 1))\n+\t\t\t\t       * 100) >> 20);\n+\t\t} else if (tp->total > 1 << 10) {\n+\t\t\tl -= snprintf(tp->display, l, \", %u.%2.2u KiB\",\n+\t\t\t\t      (int)(tp->total >> 10),\n+\t\t\t\t      ((int)(tp->total & ((1 << 10) - 1))\n+\t\t\t\t       * 100) >> 10);\n+\t\t} else {\n+\t\t\tl -= snprintf(tp->display, l, \", %u bytes\",\n+\t\t\t\t      (int)tp->total);\n+\t\t}\n+\t\tsnprintf(tp->display + sizeof(tp->display) - l, l,\n+\t\t\t \" | %lu KiB/s\", tp->avg_bytes / tp->avg_misecs);\n+\n \t\ttp->avg_bytes -= tp->last_bytes[tp->idx];\n \t\ttp->avg_misecs -= tp->last_misecs[tp->idx];\n \t\ttp->last_bytes[tp->idx] = tp->count;\n\nNicolas\n"}]}