[PATCH 0/1] Improve progress display in kB range.

[PATCH 0/1] Improve progress display in kB range.

[James Cloos]

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 <cloos@jhcloos.com>         OpenPGP: 1024D/ED7DAEA6

[PATCH 1/1] Improve progress display in kB range.

[James Cloos]

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 <cloos@jhcloos.com>
---
 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

Re: [PATCH 1/1] Improve progress display in kB range.

[Nicolas Pitre]

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

Re: [PATCH 1/1] Improve progress display in kB range.

[James Cloos]

>>>>> "Nicolas" == Nicolas Pitre <nico@cam.org> 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 <cloos@jhcloos.com>         OpenPGP: 1024D/ED7DAEA6

Re: [PATCH 1/1] Improve progress display in kB range.

[Nicolas Pitre]

On Tue, 21 Apr 2009, James Cloos wrote:

> >>>>> "Nicolas" == Nicolas Pitre <nico@cam.org> 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

Re: [PATCH 1/1] Improve progress display in kB range.

[James Cloos]

>>>>> "Nicolas" == Nicolas Pitre <nico@cam.org> 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 <cloos@jhcloos.com>         OpenPGP: 1024D/ED7DAEA6

Re: [PATCH 1/1] Improve progress display in kB range.

[James Cloos]

|> 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 <cloos@jhcloos.com>         OpenPGP: 1024D/ED7DAEA6

Re: [PATCH 1/1] Improve progress display in kB range.

[Junio C Hamano]

James Cloos <cloos@jhcloos.com> 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.

Re: [PATCH 1/1] Improve progress display in kB range.

[James Cloos]

>>>>> "Junio" == Junio C Hamano <gitster@pobox.com> 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 <cloos@jhcloos.com>         OpenPGP: 1024D/ED7DAEA6

Re: [PATCH 1/1] Improve progress display in kB range.

[Johannes Sixt]

James Cloos schrieb:
>>>>>> "Junio" == Junio C Hamano <gitster@pobox.com> 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

Re: [PATCH 1/1] Improve progress display in kB range.

[James Cloos]

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 <cloos@jhcloos.com>         OpenPGP: 1024D/ED7DAEA6

Re: [PATCH 1/1] Improve progress display in kB range.

[Nicolas Pitre]

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

Re: [PATCH 1/1] Improve progress display in kB range.

[James Cloos]

>>>>> "Nicolas" == Nicolas Pitre <nico@cam.org> 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 <cloos@jhcloos.com>         OpenPGP: 1024D/ED7DAEA6