# [PATCH 0/1] Improve progress display in kB range. 13 messages from 2009-04-19 to 2009-04-24. Participants: James Cloos, Nicolas Pitre, Junio C Hamano, Johannes Sixt. Thread: https://gitlist.dev/t/18943 ## James Cloos, 2009-04-19 04:32 Subject: [PATCH 1/1] Improve progress display in kB range. Message-ID: URL: https://gitlist.dev/e/d03620ac4d99f3280df31708032a072a4a6cd96e.1240115957.git.cloos%40jhcloos.com In-Reply-To: ``` When progress.c:throughput_string() is called, the variable total invariably has its twelve least significant bits set. Ie, it is always the case that: total & 0xFFF == 0xFFF As such, there is no point in displaying centi KiB. Signed-off-by: James Cloos --- progress.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-) diff --git a/progress.c b/progress.c index 55a8687..e51b834 100644 --- a/progress.c +++ b/progress.c @@ -125,9 +125,8 @@ static void throughput_string(struct throughput *tp, off_t total, (int)(total >> 20), ((int)(total & ((1 << 20) - 1)) * 100) >> 20); } else if (total > 1 << 10) { - l -= snprintf(tp->display, l, ", %u.%2.2u KiB", - (int)(total >> 10), - ((int)(total & ((1 << 10) - 1)) * 100) >> 10); + l -= snprintf(tp->display, l, ", %u KiB", + (int)(total >> 10)); } else { l -= snprintf(tp->display, l, ", %u bytes", (int)total); } -- 1.6.3.rc1.1.g7e8e.dirty ``` ## James Cloos, 2009-04-19 04:38 Subject: [PATCH 0/1] Improve progress display in kB range. Message-ID: URL: https://gitlist.dev/e/cover.1240115957.git.cloos%40jhcloos.com ``` One of the few irritations when using git — note that you’ll need a relatively slow link to notice — is that the progress always shows something .99 when under one Meg. This patch eliminates the constant .99 from the progress display. James Cloos (1): Improve progress display in kB range. progress.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-) -- James Cloos OpenPGP: 1024D/ED7DAEA6 ``` ## Nicolas Pitre, 2009-04-21 04:56 Subject: Re: [PATCH 1/1] Improve progress display in kB range. Message-ID: URL: https://gitlist.dev/e/alpine.LFD.2.00.0904210054190.6741%40xanadu.home In-Reply-To: ``` On Sun, 19 Apr 2009, James Cloos wrote: > When progress.c:throughput_string() is called, the variable total > invariably has its twelve least significant bits set. Ie, it is > always the case that: > > total & 0xFFF == 0xFFF Could you please explain ow you come to that conclusion? Nicolas ``` ## James Cloos, 2009-04-21 17:11 Subject: Re: [PATCH 1/1] Improve progress display in kB range. Message-ID: URL: https://gitlist.dev/e/m3skk2szgv.fsf%40lugabout.jhcloos.org In-Reply-To: ``` >>>>> "Nicolas" == Nicolas Pitre writes: Nicolas> On Sun, 19 Apr 2009, James Cloos wrote: >> When progress.c:throughput_string() is called, the variable total >> invariably has its twelve least significant bits set. Ie, it is >> always the case that: >> >> total & 0xFFF == 0xFFF Nicolas> Could you please explain ow you come to that conclusion? Empirical evidence. Even since the current progress was added, it has always shown nn.99 KiB in that range. I added an extra snprintf(3) to show total in hex and it always ends in FFF. I presume the progress function is getting called just before total hits a page boundry. In any case, the empirical evidence is clear. And only even seeing .99 is annoying. Hense the proposed patch. -JimC -- James Cloos OpenPGP: 1024D/ED7DAEA6 ``` ## Nicolas Pitre, 2009-04-21 17:28 Subject: Re: [PATCH 1/1] Improve progress display in kB range. Message-ID: URL: https://gitlist.dev/e/alpine.LFD.2.00.0904211319570.6741%40xanadu.home In-Reply-To: ``` On Tue, 21 Apr 2009, James Cloos wrote: > >>>>> "Nicolas" == Nicolas Pitre writes: > > Nicolas> On Sun, 19 Apr 2009, James Cloos wrote: > >> When progress.c:throughput_string() is called, the variable total > >> invariably has its twelve least significant bits set. Ie, it is > >> always the case that: > >> > >> total & 0xFFF == 0xFFF > > Nicolas> Could you please explain ow you come to that conclusion? > > Empirical evidence. > > Even since the current progress was added, it has always shown nn.99 KiB > in that range. I added an extra snprintf(3) to show total in hex and it > always ends in FFF. Empirical evidence on my side shows the opposite. I just did a fetch in my kernel repo and got: Receiving objects: 100% (1373/1373), 223.36 KiB, done. > I presume the progress function is getting called just before total hits > a page boundry. In any case, the empirical evidence is clear. And only > even seeing .99 is annoying. Hense the proposed patch. I must NACK your patches. Presumptions are not good enough justification for such a change, especially if results can't be reproduced. That doesn't mean the code is completely bug free of course, but finding the source of the bug affecting you would be a far better course of action than simply turning our back on it. Maybe you can tell us more about your environment? Nicolas ``` ## James Cloos, 2009-04-21 20:16 Subject: Re: [PATCH 1/1] Improve progress display in kB range. Message-ID: URL: https://gitlist.dev/e/m3d4b5oj76.fsf%40lugabout.jhcloos.org In-Reply-To: ``` >>>>> "Nicolas" == Nicolas Pitre writes: Nicolas> Empirical evidence on my side shows the opposite. I just did a fetch in Nicolas> my kernel repo and got: Nicolas> Receiving objects: 100% (1373/1373), 223.36 KiB, done. OK. That does show that my proposed patch is incomplete. The contrary example is from the final output, if the received pack is less than a Meg. The annoyance is in the progress display. In index-pack.c, fill() calls xread() and then display_throughput(). Since xread() is designed to call read(2) and simple continue on any EINTR or EAGAIN, then — even though xread() explicitly does not guarantee that ‘len’ bytes are read even if the data are available — in practice xread() fills its buffer. (At least on 32-bit x86, using Linus’ kernel.) Therefore, in practice — and as I have witnessed several thousand times without ever having seen a contrary example — display_throughput() is called *durring* a download only when total & 0xFFF == 0xFFF. Perhaps, then, display_throughput() should round differently, so that the logical equivilent of: ( ( n << 10) & 0x3FF ) / 1024.0 would be rounded up. Then throughput_string() could elide the ".%2.2u" whenever ((int)(total & ((1 << 10) - 1)) * 100) >> 10) == 0. Or throughput_string() could simply elide the ".%2.2u" whenever total & 0x3FF == 0x3FF. Nicolas> I must NACK your patches. Presumptions are not good enough Nicolas> justification for such a change, especially if results can't Nicolas> be reproduced. Understood. I concentrated on the progress display and ignored the final display. -JimC -- James Cloos OpenPGP: 1024D/ED7DAEA6 ``` ## James Cloos, 2009-04-22 14:33 Subject: Re: [PATCH 1/1] Improve progress display in kB range. Message-ID: URL: https://gitlist.dev/e/m34owgoj08.fsf%40lugabout.jhcloos.org In-Reply-To: ``` |> Therefore, in practice — and as I have witnessed several thousand times |> without ever having seen a contrary example — display_throughput() is |> called *durring* a download only when total & 0xFFF == 0xFFF. Another possibility is an off-by-one error. The relevant part of fill() looks like: ,----< excerpt from index-pack.c:fill() > | do { | ssize_t ret = xread(input_fd, input_buffer + input_len, | sizeof(input_buffer) - input_len); | if (ret <= 0) { | if (!ret) | die("early EOF"); | die("read error on input: %s", strerror(errno)); | } | input_len += ret; | if (from_stdin) | display_throughput(progress, consumed_bytes + input_len); | } while (input_len < min); | return input_buffer; | } `---- if *(input_buffer + ret) is the last read octet rather than the next empty octet, that would also explain what I see. Perhaps that call to display_throughput() should have an extra +1? -JimC -- James Cloos OpenPGP: 1024D/ED7DAEA6 ``` ## Junio C Hamano, 2009-04-22 19:44 Subject: Re: [PATCH 1/1] Improve progress display in kB range. Message-ID: <7vljps324a.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vljps324a.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` James Cloos writes: > |> Therefore, in practice — and as I have witnessed several thousand times > |> without ever having seen a contrary example — display_throughput() is > |> called *durring* a download only when total & 0xFFF == 0xFFF. > > Another possibility is an off-by-one error. The relevant part of fill() > looks like: > > ,----< excerpt from index-pack.c:fill() > > | do { > | ssize_t ret = xread(input_fd, input_buffer + input_len, > | sizeof(input_buffer) - input_len); > | if (ret <= 0) { > | if (!ret) > | die("early EOF"); > | die("read error on input: %s", strerror(errno)); > | } > | input_len += ret; > | if (from_stdin) > | display_throughput(progress, consumed_bytes + input_len); > | } while (input_len < min); > | return input_buffer; > | } > `---- > > if *(input_buffer + ret) is the last read octet rather than the next > empty octet, that would also explain what I see. After checking "ret" from xread(), input_len is incremented by that amount, and the next iteration gives "input_buffer + input_len" to xread(). If input_buffer[ret] _were_ the last octet read, your loop would be discarding that octet when you call more than one xread() to fill the buffer. ``` ## James Cloos, 2009-04-22 22:35 Subject: Re: [PATCH 1/1] Improve progress display in kB range. Message-ID: URL: https://gitlist.dev/e/m3ab68mi3q.fsf%40lugabout.jhcloos.org In-Reply-To: <7vljps324a.fsf@gitster.siamese.dyndns.org> ``` >>>>> "Junio" == Junio C Hamano writes: Junio> If input_buffer[ret] _were_ the last octet read, [the] loop would Junio> be discarding that octet when [it] call[s] more than one xread() Junio> to fill the buffer. That did sem more likely, but I thought I'd throw out the possibility. I just tried instrumenting fill(). In the first call to fill() during a fetch, xread() returns 4096. In the second call to fill(), xread() returns 4095. In all subsequent calls to fill(), xread() returns 4096 again. So, after the second call to xread(), consumed_bytes & 0xFFF == 0xFFF. It always follows the pattern: xread() read 0x1000 from fd 0 at 0x80a0900 ret = 0x1000 consumed_bytes = 0 input_len = 0x1000 xread() read 0xFFF from fd 0 at 0x80a0900 ret = 0xFFF consumed_bytes = 0x1000 input_len = 0xFFF xread() read 0x1000 from fd 0 at 0x80a0900 ret = 0x1000 consumed_bytes = 0x1FFF input_len = 0x1000 with all subsequent calls reading 0x1000 and adding 0x1000 to consumed_bytes, until the final chuck of tha pack is read. Also, all calls to xread() where the status lines are being sent from the remote server also return 0x1000 octets. Only the second chunck of a pack ever returns 0xFFF. I've tested against a number of remote servers and the pattern holds for a wide range of remote server versions. The pattern also holds for clones over ssh. Does anyone have an idea of why the second call to read(2), when receiving a pack from a remote, always leaves the last octet of the buffer free, whereas all other read(2)s fill it? -JimC -- James Cloos OpenPGP: 1024D/ED7DAEA6 ``` ## Johannes Sixt, 2009-04-23 06:23 Subject: Re: [PATCH 1/1] Improve progress display in kB range. Message-ID: <49F0094A.6020609@viscovery.net> URL: https://gitlist.dev/e/49F0094A.6020609%40viscovery.net In-Reply-To: ``` James Cloos schrieb: >>>>>> "Junio" == Junio C Hamano writes: > > Junio> If input_buffer[ret] _were_ the last octet read, [the] loop would > Junio> be discarding that octet when [it] call[s] more than one xread() > Junio> to fill the buffer. > > That did sem more likely, but I thought I'd throw out the possibility. > > I just tried instrumenting fill(). > > In the first call to fill() during a fetch, xread() returns 4096. In > the second call to fill(), xread() returns 4095. In all subsequent > calls to fill(), xread() returns 4096 again. > > So, after the second call to xread(), consumed_bytes & 0xFFF == 0xFFF. > > It always follows the pattern: > > xread() read 0x1000 from fd 0 at 0x80a0900 > ret = 0x1000 > consumed_bytes = 0 > input_len = 0x1000 > > xread() read 0xFFF from fd 0 at 0x80a0900 > ret = 0xFFF > consumed_bytes = 0x1000 > input_len = 0xFFF > > xread() read 0x1000 from fd 0 at 0x80a0900 > ret = 0x1000 > consumed_bytes = 0x1FFF > input_len = 0x1000 > > with all subsequent calls reading 0x1000 and adding 0x1000 to > consumed_bytes, until the final chuck of tha pack is read. > > Also, all calls to xread() where the status lines are being sent from > the remote server also return 0x1000 octets. Only the second chunck of > a pack ever returns 0xFFF. > > I've tested against a number of remote servers and the pattern holds for > a wide range of remote server versions. > > The pattern also holds for clones over ssh. > > Does anyone have an idea of why the second call to read(2), when > receiving a pack from a remote, always leaves the last octet of the > buffer free, whereas all other read(2)s fill it? You could instrument upload-pack.c. There is an xread() around l.237; does it aways return 4096? And send_client_data() at around l.254; when does it send 4096 and when 4095 bytes? Read the comment in this if-branch. upload-pack is run on the server-side; therefore, you can test this only on local repositories (unless you can replace upload-pack on the server, too). -- Hannes ``` ## James Cloos, 2009-04-24 20:15 Subject: Re: [PATCH 1/1] Improve progress display in kB range. Message-ID: URL: https://gitlist.dev/e/m3zle5hkpa.fsf%40lugabout.jhcloos.org In-Reply-To: ``` It turns out that the off by one is intentional. >From upload-pack.c: /* Data ready; we keep the last byte to ourselves * in case we detect broken rev-list, so that we * can leave the stream corrupted. This is * unfortunate -- unpack-objects would happily * accept a valid packdata with trailing garbage, * so appending garbage after we pass all the * pack data is not good enough to signal * breakage to downstream. */ Upload-pack uses a buffer of 8193 octets, which is why it is always the second xread() that returns 0xFFF. It first sends 8191 octets, then n chunks of 8192 and then the final chunk. It seems to only way to fix the progress annoyance -- and it is most annoying -- would be to round correctly in progress.c. (The .99 comes from 1023/1024, which is .999 and therefor ought to round up to 1.00, not down to 0.99.) Will a patch which does round-to-nearest (instead of the current round-to-zero) be accepted? -JimC -- James Cloos OpenPGP: 1024D/ED7DAEA6 ``` ## Nicolas Pitre, 2009-04-24 21:46 Subject: Re: [PATCH 1/1] Improve progress display in kB range. Message-ID: URL: https://gitlist.dev/e/alpine.LFD.2.00.0904241722220.6741%40xanadu.home In-Reply-To: ``` On Fri, 24 Apr 2009, James Cloos wrote: > It turns out that the off by one is intentional. > > >From upload-pack.c: > > /* Data ready; we keep the last byte to ourselves > * in case we detect broken rev-list, so that we > * can leave the stream corrupted. This is > * unfortunate -- unpack-objects would happily > * accept a valid packdata with trailing garbage, > * so appending garbage after we pass all the > * pack data is not good enough to signal > * breakage to downstream. > */ > > Upload-pack uses a buffer of 8193 octets, which is why it is always > the second xread() that returns 0xFFF. It first sends 8191 octets, > then n chunks of 8192 and then the final chunk. > > It seems to only way to fix the progress annoyance -- and it is most > annoying -- would be to round correctly in progress.c. > > (The .99 comes from 1023/1024, which is .999 and therefor ought to > round up to 1.00, not down to 0.99.) > > Will a patch which does round-to-nearest (instead of the current > round-to-zero) be accepted? Sure. What about this (untested): diff --git a/progress.c b/progress.c index 55a8687..621c34e 100644 --- a/progress.c +++ b/progress.c @@ -121,13 +121,13 @@ static void throughput_string(struct throughput *tp, off_t total, (int)(total >> 30), (int)(total & ((1 << 30) - 1)) / 10737419); } else if (total > 1 << 20) { + int x = total + 5243; /* for rounding */ l -= snprintf(tp->display, l, ", %u.%2.2u MiB", - (int)(total >> 20), - ((int)(total & ((1 << 20) - 1)) * 100) >> 20); + x >> 20, ((x & ((1 << 20) - 1)) * 100) >> 20); } else if (total > 1 << 10) { + int x = total + 5; /* for rounding */ l -= snprintf(tp->display, l, ", %u.%2.2u KiB", - (int)(total >> 10), - ((int)(total & ((1 << 10) - 1)) * 100) >> 10); + x >> 10, ((x & ((1 << 10) - 1)) * 100) >> 10); } else { l -= snprintf(tp->display, l, ", %u bytes", (int)total); } Nicolas ``` ## James Cloos, 2009-04-24 22:20 Subject: Re: [PATCH 1/1] Improve progress display in kB range. Message-ID: URL: https://gitlist.dev/e/m3tz4dhewa.fsf%40lugabout.jhcloos.org In-Reply-To: ``` >>>>> "Nicolas" == Nicolas Pitre writes: >> Will a patch which does round-to-nearest (instead of the current >> round-to-zero) be accepted? Nicolas> Sure. What about this (untested): Nicolas> + int x = total + 5243; /* for rounding */ Nicolas> + int x = total + 5; /* for rounding */ That looks correct. -JimC -- James Cloos OpenPGP: 1024D/ED7DAEA6 ```