Re: [PATCH 2/2] read-cache: fix incorrect count and progress bar stalling

From: René Scharfe <l.s.r@web.de>
Date: Jun 20, 2021, 12:53 UTC
Message-ID: <0a30d7f3-1718-f2c5-39ab-75acfd754d9a@web.de>
In-Reply-To: <877divystm.fsf@evledraar.gmail.com>

Am 15.06.21 um 18:46 schrieb Ævar Arnfjörð Bjarmason:

> We disagree, and I for one think I understand what you mean, perhaps you
> don't understand what I mean, but let's try to move on.

Perhaps. You seem to think of progress as being represented by a real number. We show integers, though, and you want to round up.

The progress display's function is to inform the user that work is being done and how much there is still to do. It allows them to decide whether to keep the program running.

A progress of "100%" being shown for an extended duration would lead me to the conclusion that the program hangs and I'd cancel it. Rounding down (truncating) prevents that.

Showing an estimated time of completion as well would be even nicer, but is only practical if the time taken for each work item is roughly the same.

But let's move on indeed. The part of your patch that moves the display_progress() call to the top of the loop to avoid stalling is a good idea and worth splitting out into its own patch (keeping the "i").

In general it seems that changes described with "Let's also ..." or "While at it ..." almost always deserve their own patch. I need to follow that insight more myself..

Show 6 quoted lines

> I think it would be better if you replied specifically to the comments I
> had later about throughput progress bars, i.e.:
>
>     How does the idea that we show "has been done" make sense when you
>     combine the progress.c API with the display_throughput(). I.e. output
>     like[...]

Junio already replied to that, but since you ask, here are my thoughts:

Progress and throughput are separate metrics. Adding one doesn't change the other. The throughput value is not specific to the currently processed item.

Say we download a number of files of different sizes and want to show our progress. Then from time to time we display the number of processed files and how many bytes we got since the last update, divided by the time passed since then. The reported bytes could belong to multiple files. Or we could process lots of zero-sized files, which would keep throughput low.

> Anyway, in this case I understood you to mean that you thought the
> off-by-one wasn't a big deal in practice most of the time, I don't think
> so either for e.g. counting objects in pack files.

Not exactly. While I think a difference of one isn't a big deal most of the time, also think there is a correct way, i.e. to show the number of completed items. You have found ways to use an off-by-one error, and my point was that this usage is not reliable. Perhaps that's a weak and convoluted argument.

> I do think it's useful to be consistent though, and for e.g. cases of
> downloading 5 files it makes sense to show 1/5 if we are currently in
> the process of downloading files 1 out of 5, not 0/5 or whatever.

I agree that we should be consistent. If we have downloaded 70% of the first of five files then we have 0.7 files, which is not yet 1 file, so we have to say 0/5.

But let's move on, for real.

René