From: Junio C Hamano <hidden> Date: 2021-06-07 23:41:22
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted
So I think this pattern works:
for (i = 0; i < nr; i++) {
display_progress(p, i);
/* work work work */
}
display_progress(p, nr);
Alternatively, if the work part doesn't contain continue statements:
for (i = 0; i < nr; i++) {
/* work work work */
display_progress(p, i + 1);
}
But yes, I agree with the issue in theory, but I think in practice we
don't need to worry about these 100% cases.
Hmph, but in practice we do need to worry, don't we? Otherwise you
wouldn't have started this thread and René wouldn't have responded.
I agree with the issue and I think we should count what we have
finished.
So I think this pattern works:
for (i = 0; i < nr; i++) {
display_progress(p, i);
/* work work work */
}
display_progress(p, nr);
Alternatively, if the work part doesn't contain continue statements:
for (i = 0; i < nr; i++) {
/* work work work */
display_progress(p, i + 1);
}
But yes, I agree with the issue in theory, but I think in practice we
don't need to worry about these 100% cases.
Hmph, but in practice we do need to worry, don't we? Otherwise you
wouldn't have started this thread and René wouldn't have responded.
I started this thread because of:
for (i = 0; i < large_number; i++) {
if (maybe_branch_here())
continue;
/* work work work */
display_progress(p, i);
}
display_progress(p, large_number);
Mainly because it's a special snowflake in how the process.c API is
used, with most other callsites doing:
for (i = 0; i < large_number; i++) {
display_progress(p, i + 1);
/* work work work */
}
Which yes, as René points out *could* hang on 100%, but I think in
practice isn't an issue here, and changing the code per my patch here
solves the practical issue with us always taking the maybe_branch_here()
(or enough that the progress bar hangs).
I agree with the issue and I think we should count what we have
finished.
Fair enough, but in the meantime can we take this patch? I think fixing
that (IMO in practice hypothetical issue) is much easier when we
consistently use that "i + 1" pattern above (which we mostly do
already). We can just search-replace "++i" to "i++" and "i + 1" to "i"
and have stop_progress() be what bumps it to 100%.
I have some unsent patches queued on top of this which has some general
fixes to edge cases in the progress.c API making that & more easier...q
From: René Scharfe <hidden> Date: 2021-06-08 16:14:52
Am 08.06.21 um 12:58 schrieb Ævar Arnfjörð Bjarmason:
On Tue, Jun 08 2021, Junio C Hamano wrote:
quoted
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted
quoted
So I think this pattern works:
for (i = 0; i < nr; i++) {
display_progress(p, i);
/* work work work */
}
display_progress(p, nr);
Alternatively, if the work part doesn't contain continue statements:
for (i = 0; i < nr; i++) {
/* work work work */
display_progress(p, i + 1);
}
But yes, I agree with the issue in theory, but I think in practice we
don't need to worry about these 100% cases.
Hmph, but in practice we do need to worry, don't we? Otherwise you
wouldn't have started this thread and René wouldn't have responded.
I started this thread because of:
for (i = 0; i < large_number; i++) {
if (maybe_branch_here())
continue;
/* work work work */
display_progress(p, i);
}
display_progress(p, large_number);
Mainly because it's a special snowflake in how the process.c API is
used, with most other callsites doing:
for (i = 0; i < large_number; i++) {
display_progress(p, i + 1);
/* work work work */
}
Moving the first call to the top of the loop makes sense. It ensures
all kind of progress -- skipping and actual work -- is reported without
undue delay.
Adding one would introduce an off-by-one error. Removing the call after
the loop would leave the progress report at one short of 100%. I don't
see any benefits of these additional changes, only downsides.
If other callsites have an off-by-one error and we care enough then we
should fix them. Copying their style and spreading the error doesn't
make sense -- correctness trumps consistency.
Fair enough, but in the meantime can we take this patch? I think fixing
that (IMO in practice hypothetical issue) is much easier when we
consistently use that "i + 1" pattern above (which we mostly do
already). We can just search-replace "++i" to "i++" and "i + 1" to "i"
and have stop_progress() be what bumps it to 100%.
This assumes the off-by-one error is consistent. Even if that is the
case you could apply your mechanical fix and leave out read-cache.
This would happen automatically because when keeping i there is no ++i
to be found.
stop_progress() doesn't set the progress to 100%:
$ (echo progress 0; echo update) |
./t/helper/test-tool progress --total 1 test
test: 0% (0/1), done.
I wonder (only in a semi-curious way, though) if we can detect
off-by-one errors by adding an assertion to display_progress() that
requires the first update to have the value 0, and in stop_progress()
one that requires the previous display_progress() call to have a value
equal to the total number of work items. Not sure it'd be worth the
hassle..
René
Am 08.06.21 um 12:58 schrieb Ævar Arnfjörð Bjarmason:
quoted
On Tue, Jun 08 2021, Junio C Hamano wrote:
quoted
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted
quoted
So I think this pattern works:
for (i = 0; i < nr; i++) {
display_progress(p, i);
/* work work work */
}
display_progress(p, nr);
Alternatively, if the work part doesn't contain continue statements:
for (i = 0; i < nr; i++) {
/* work work work */
display_progress(p, i + 1);
}
But yes, I agree with the issue in theory, but I think in practice we
don't need to worry about these 100% cases.
Hmph, but in practice we do need to worry, don't we? Otherwise you
wouldn't have started this thread and René wouldn't have responded.
I started this thread because of:
for (i = 0; i < large_number; i++) {
if (maybe_branch_here())
continue;
/* work work work */
display_progress(p, i);
}
display_progress(p, large_number);
Mainly because it's a special snowflake in how the process.c API is
used, with most other callsites doing:
for (i = 0; i < large_number; i++) {
display_progress(p, i + 1);
/* work work work */
}
Moving the first call to the top of the loop makes sense. It ensures
all kind of progress -- skipping and actual work -- is reported without
undue delay.
Adding one would introduce an off-by-one error. Removing the call after
the loop would leave the progress report at one short of 100%. I don't
see any benefits of these additional changes, only downsides.
If other callsites have an off-by-one error and we care enough then we
should fix them. Copying their style and spreading the error doesn't
make sense -- correctness trumps consistency.
quoted
Fair enough, but in the meantime can we take this patch? I think fixing
that (IMO in practice hypothetical issue) is much easier when we
consistently use that "i + 1" pattern above (which we mostly do
already). We can just search-replace "++i" to "i++" and "i + 1" to "i"
and have stop_progress() be what bumps it to 100%.
This assumes the off-by-one error is consistent. Even if that is the
case you could apply your mechanical fix and leave out read-cache.
This would happen automatically because when keeping i there is no ++i
to be found.
stop_progress() doesn't set the progress to 100%:
$ (echo progress 0; echo update) |
./t/helper/test-tool progress --total 1 test
test: 0% (0/1), done.
I was too quick with that "But yes, I agree with the issue in theory".
Having thought about it some more I think you're wrong, it doesn't make
sense to use the API in the way you're suggesting.
There's an off-by-one error, but it's in the pattern you're
suggesting. Calling "i + 1" on the "first item" doesn't just make for
less verbose code, it's also more correct.
Why? Because:
/* 1. Setup progress */
large_number = 3;
progress = start_progress(title, total);
/* 2. Before we "actually" do anything */
for (i = 0; i < 3; i++) {
/* 3. Now doing O(large_number) work */
display_progress(progress, i + 1);
}
/* 4. Done */
stop_progress(&progress);
As you'll see if you insert a sleep or whatever at "2" we'll signal and
display the progress bar even if we haven't called display_progress()
yet.
Thus calling display_progress with n=0 makes "we are doing the first
item" indistinguishable from "we haven't gotten to the first item
yet". When you're in the loop the first item should be n=1.
This isn't just more correct with this API. I think it also makes sense
not to leak the zero indexed C abstraction to human output. As in:
I sat down at the table with my three apples (stages 1..2 above),
and now I'm eating my first apple (stage 3), I'm not starting to eat
my zeroeth apple. At some point I'm done eating all 3 apples (stage
4).
I think this gets to the crux of the issue for you, per your upthread:
[...]With your change you'd get 100% for a minute. Both would be
annoying, but the latter would have me raging. "If you're done",
I'd yell at the uncaring machine, "what are you still doing!?".
If we said we're done and we're not then yes, that would be a bug.
But that's not what we said. Distinguishing "I'm on 3/3" from "I'm done"
is exactly what the progress.c output does:
$ perl -wE 'for (0..3) { say "update"; say "progress $_" }' |
./helper/test-tool progress --total=3 Apples 2>&1 |
cat -v | perl -pe 's/\^M\K/\n/g'
Apples: 0% (0/3)^M
Apples: 33% (1/3)^M
Apples: 66% (2/3)^M
Apples: 100% (3/3)^M
Apples: 100% (3/3), done.
It's not immediately obvious from that output, but the last line ending
in ", done" isn't added by any display_progress() call, but by calling
stop_progress(). That's when we're done.
Arguably this is too subtle and we should say ", but not done yet!" or
something on that penultimate line. But that's bikeshedding a display
issue; The API use in my patch is correct.
As I noted upthread that's a "matter of some philosophy". I guess you
might report your progress as being at 2/3 apples, even though you're
one bite away from finishing the third apple. Personally I'd say I'm on
the third apple, I'm just not done with it.
But the philosophizing of whether your "progress" is the zeroeth apple
of the day as you're chewing on it aside, I think it's clear that in the
context of the progress.c API using n=0 for the first apple would be a
bug.
You'll conflate "setup" with "work" per the above, and you'll say you're
on the second apple even if you're a bite away from finishing it. We
don't conflate that "100%" state with being "done", so I don't see why
we'd do that.
I wonder (only in a semi-curious way, though) if we can detect
off-by-one errors by adding an assertion to display_progress() that
requires the first update to have the value 0, and in stop_progress()
one that requires the previous display_progress() call to have a value
equal to the total number of work items. Not sure it'd be worth the
hassle..
That's intentional. We started eating 3 apples, got to one, but now our
house is on fire and we're eating no more apples today, even if we
planned to eat 3 when we sat down.
The progress bar reflects this unexpected but recoverable state:
$ perl -wE 'for (0..1) { say "update"; say "progress $_" }' |
./helper/test-tool progress --total=3 Apples 2>&1 |
cat -v | perl -pe 's/\^M\K/\n/g'
Apples: 0% (0/3)^M
Apples: 33% (1/3)^M
Apples: 33% (1/3), done.
We're at 1/3, but we're done. No more apples.
This isn't just some hypothetical, e.g. consider neeing to unlink() or
remove files/directories one at a time in a directory and getting the
estimated number from st_nlink (yeah yeah, unportable, but it was the
first thing I thought of).
We might think we're processing 10 entries, but another other processes
might make our progress bar end at more or less than the 100% we
expected. That's OK, not something we should invoke BUG() about.
Similarly, the n=0 being distinguishable from the first
display_progress() is actually useful in practice. It's something I've
seen git.git emit (not recently, I patched the relevant code to emit
more granular progress).
It's useful to know that we're stalling on the setup code before the
for-loop, not on the first item.
From: Junio C Hamano <hidden> Date: 2021-06-10 05:30:51
Ævar Arnfjörð Bjarmason [off-list ref] writes:
I was too quick with that "But yes, I agree with the issue in theory".
Having thought about it some more I think you're wrong, it doesn't make
sense to use the API in the way you're suggesting.
Sorry, I've read the message I am responding to a few times, but I
couldn't tell what you are arguing against in the suggestion given
earlier by René, which offered two possibile ways to consistently
call display() with the number of things that we have DONE
processing (not the number of things that we are about to touch) [*1*].
Why? Because:
/* 1. Setup progress */
large_number = 3;
progress = start_progress(title, total);
/* 2. Before we "actually" do anything */
for (i = 0; i < 3; i++) {
/* 3. Now doing O(large_number) work */
display_progress(progress, i + 1);
}
/* 4. Done */
stop_progress(&progress);
As you'll see if you insert a sleep or whatever at "2" we'll signal and
display the progress bar even if we haven't called display_progress()
yet.
That is, the signal start_progress_delay() kicking in? But
progress_test_force_update() is a curiosity that appears only in
test-tool and in real life programs, you'd have to call display
yourself, so a long delay at "3" will appear as a long silence,
I would think.
In any case, if we somehow managed to cause display_progress() to
happen somewhere near "2", we haven't finished even a single item
yet at that point, so "we are giving progress bar, and we have
finished 0%" that is an output from such a call would make sense.
As we finish one item, we'd increment it to 1 (that happens after we
spent time at "3" during the first iteration, and display_progress()
is called with "i+1").
Thus calling display_progress with n=0 makes "we are doing the first
item" indistinguishable from "we haven't gotten to the first item
yet".
I am guessing that you are talking about "what value should we call
display() if '2' takes a long time and we want to give progress
early even before we enter the loop?"
I do not view such a call as "we haven't gotten to" or "we are
doing"; an extra call we would make with display(n=0) around "2" is
"how much have we finished? we have completed none".
When you're in the loop the first item should be n=1.
Doesn't that depend on where in the loop you do the work?
If you spend cycles and finish processing the first item _before_
calling display_progress(), you are reporting how many you have
finished, so at the end of the first iteration of the loop, you'd
call with n=1.
If on the other hand you somehow report at the beginning (perhaps
because otherwise you'd have tough time structuring the code when
the loop body has a continue etc.) before finishing the processing
for the first item, you'd call with n=0 (and make sure before
calling stop_progress(), you'd call another display() after exiting
the loop).
And I think that is consistent with what Réne suggested earlier, so
I am not sure where your "I am right you are wrong" is coming from.
To me, you two seem to be agreeing.
[Footnote]
*1* [off-list ref]
Showing only the completed items makes sense. That the next one is
being processed is self-understood. Once all of them are done, 100% is
shown and the progress line is finished.
So I think this pattern works:
for (i = 0; i < nr; i++) {
display_progress(p, i);
/* work work work */
}
display_progress(p, nr);
Alternatively, if the work part doesn't contain continue statements:
for (i = 0; i < nr; i++) {
/* work work work */
display_progress(p, i + 1);
}
From: René Scharfe <hidden> Date: 2021-06-10 15:15:01
Am 10.06.21 um 07:30 schrieb Junio C Hamano:
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted
I was too quick with that "But yes, I agree with the issue in theory".
Having thought about it some more I think you're wrong, it doesn't make
sense to use the API in the way you're suggesting.
Sorry, I've read the message I am responding to a few times, but I
couldn't tell what you are arguing against in the suggestion given
earlier by René, which offered two possibile ways to consistently
call display() with the number of things that we have DONE
processing (not the number of things that we are about to touch) [*1*].
Same here.
Perhaps a demo helps. The patch at the bottom adds an echo command to
the test helper. This way we can intersperse the progress lines with
indications when items are processed.
So here's the pattern for calling display_progress at the top of the
loop and again with the number of items after the loop:
$ (
for i in 0 1 2
do
echo progress $i
echo update
echo echo WORK
done
echo progress 3
) | ./t/helper/test-tool progress --total 3 test 2>&1 | tr '\r' '\n'
test: 0% (0/3)
WORK
test: 33% (1/3)
WORK
test: 66% (2/3)
WORK
test: 100% (3/3)
test: 100% (3/3), done.
The progress lines reflect the number of finished items at all times.
Here's the pattern for display_progress at the bottom of the loop:
$ (
for i in 0 1 2
do
echo echo WORK
echo progress $(( $i + 1 ))
echo update
done
) | ./t/helper/test-tool progress --total 3 test 2>&1 | tr '\r' '\n'
WORK
test: 33% (1/3)
WORK
test: 66% (2/3)
WORK
test: 100% (3/3)
test: 100% (3/3), done.
Same here, the progress line shows the correct number of finished items.
Here's the pattern suggested in your patch:
$ (
for i in 0 1 2
do
echo progress $(( $i + 1 ))
echo update
echo echo WORK
done
) | ./t/helper/test-tool progress --total 3 test 2>&1 | tr '\r' '\n'
test: 33% (1/3)
WORK
test: 66% (2/3)
WORK
test: 100% (3/3)
WORK
test: 100% (3/3), done.
It reports one item too many in the intermediate progress lines and
is correct only at the very end.
René
From: René Scharfe <hidden> Date: 2021-06-10 15:15:05
Am 09.06.21 um 00:12 schrieb Ævar Arnfjörð Bjarmason:
On Tue, Jun 08 2021, René Scharfe wrote:
quoted
I wonder (only in a semi-curious way, though) if we can detect
off-by-one errors by adding an assertion to display_progress() that
requires the first update to have the value 0, and in stop_progress()
one that requires the previous display_progress() call to have a value
equal to the total number of work items. Not sure it'd be worth the
hassle..
That's intentional. We started eating 3 apples, got to one, but now our
house is on fire and we're eating no more apples today, even if we
planned to eat 3 when we sat down.
The progress bar reflects this unexpected but recoverable state:
$ perl -wE 'for (0..1) { say "update"; say "progress $_" }' |
./helper/test-tool progress --total=3 Apples 2>&1 |
cat -v | perl -pe 's/\^M\K/\n/g'
Apples: 0% (0/3)^M
Apples: 33% (1/3)^M
Apples: 33% (1/3), done.
We're at 1/3, but we're done. No more apples.
This isn't just some hypothetical, e.g. consider neeing to unlink() or
remove files/directories one at a time in a directory and getting the
estimated number from st_nlink (yeah yeah, unportable, but it was the
first thing I thought of).
We might think we're processing 10 entries, but another other processes
might make our progress bar end at more or less than the 100% we
expected. That's OK, not something we should invoke BUG() about.
It doesn't have to be a BUG; a warning would suffice. And I hope not
finishing the expected number of items due to a catastrophic event is
rare enough that an additional warning wouldn't cause too much pain.
Loops that *regularly* end early are not a good fit for progress
percentages, I think.
Similarly, the n=0 being distinguishable from the first
display_progress() is actually useful in practice. It's something I've
seen git.git emit (not recently, I patched the relevant code to emit
more granular progress).
It's useful to know that we're stalling on the setup code before the
for-loop, not on the first item.
Hmm, preparations that take a noticeable time might deserve their own
progress line.
Anyway, if no guard rails can be built then we have to rely on our math
skills alone. Off-by-one errors may look silly, but are no joke -- they
are surprisingly easy to make.
René
Am 09.06.21 um 00:12 schrieb Ævar Arnfjörð Bjarmason:
quoted
On Tue, Jun 08 2021, René Scharfe wrote:
quoted
I wonder (only in a semi-curious way, though) if we can detect
off-by-one errors by adding an assertion to display_progress() that
requires the first update to have the value 0, and in stop_progress()
one that requires the previous display_progress() call to have a value
equal to the total number of work items. Not sure it'd be worth the
hassle..
That's intentional. We started eating 3 apples, got to one, but now our
house is on fire and we're eating no more apples today, even if we
planned to eat 3 when we sat down.
The progress bar reflects this unexpected but recoverable state:
$ perl -wE 'for (0..1) { say "update"; say "progress $_" }' |
./helper/test-tool progress --total=3 Apples 2>&1 |
cat -v | perl -pe 's/\^M\K/\n/g'
Apples: 0% (0/3)^M
Apples: 33% (1/3)^M
Apples: 33% (1/3), done.
We're at 1/3, but we're done. No more apples.
This isn't just some hypothetical, e.g. consider neeing to unlink() or
remove files/directories one at a time in a directory and getting the
estimated number from st_nlink (yeah yeah, unportable, but it was the
first thing I thought of).
We might think we're processing 10 entries, but another other processes
might make our progress bar end at more or less than the 100% we
expected. That's OK, not something we should invoke BUG() about.
It doesn't have to be a BUG; a warning would suffice. And I hope not
finishing the expected number of items due to a catastrophic event is
rare enough that an additional warning wouldn't cause too much pain.
It's not a catastrophic event, just a run of the mill race condition
we'll expect if we're dealing with the real world.
E.g. you asked to unlink 1000 files, we do so, we find 10 are unlinked
already, or the command is asked to recursively unlink all files in a
directory tree, and new ones have showed up.
In those cases we should just just shrug and move on, no need for a
warning. We just don't always have perfect information about future
state at the start of the loop.
Loops that *regularly* end early are not a good fit for progress
percentages, I think.
Arguably yes, but in these fuzzy cases not providing a "total" means
showing no progress at all, just a counter. Perhaps we should have some
other "provide total, and it may be fuzzy" flag. Not providing it might
run into your proposed BUG(), my point was that the current API
providing this flexibility is intentional.
quoted
Similarly, the n=0 being distinguishable from the first
display_progress() is actually useful in practice. It's something I've
seen git.git emit (not recently, I patched the relevant code to emit
more granular progress).
It's useful to know that we're stalling on the setup code before the
for-loop, not on the first item.
Hmm, preparations that take a noticeable time might deserve their own
progress line.
Sure, and I've split some of those up in the past, but this seems like
ducking/not addressing the point that the API use we disagree on has
your preferred use conflating these conditions, but mine does not...
Anyway, if no guard rails can be built then we have to rely on our math
skills alone. Off-by-one errors may look silly, but are no joke -- they
are surprisingly easy to make.
...which, regardless of whether one views a progress of "1/5 items" has
"finished 1/5" or "working on 1/5", which I think *in general* is an
arbitrary choice, I think the progress.c API we have in git.git clearly
fits the usage I'm describing better.
From: René Scharfe <hidden> Date: 2021-06-14 17:18:15
Am 14.06.21 um 13:07 schrieb Ævar Arnfjörð Bjarmason:
On Thu, Jun 10 2021, René Scharfe wrote:
quoted
Am 09.06.21 um 00:12 schrieb Ævar Arnfjörð Bjarmason:
quoted
On Tue, Jun 08 2021, René Scharfe wrote:
quoted
I wonder (only in a semi-curious way, though) if we can detect
off-by-one errors by adding an assertion to display_progress() that
requires the first update to have the value 0, and in stop_progress()
one that requires the previous display_progress() call to have a value
equal to the total number of work items. Not sure it'd be worth the
hassle..
That's intentional. We started eating 3 apples, got to one, but now our
house is on fire and we're eating no more apples today, even if we
planned to eat 3 when we sat down.
The progress bar reflects this unexpected but recoverable state:
$ perl -wE 'for (0..1) { say "update"; say "progress $_" }' |
./helper/test-tool progress --total=3 Apples 2>&1 |
cat -v | perl -pe 's/\^M\K/\n/g'
Apples: 0% (0/3)^M
Apples: 33% (1/3)^M
Apples: 33% (1/3), done.
We're at 1/3, but we're done. No more apples.
This isn't just some hypothetical, e.g. consider neeing to unlink() or
remove files/directories one at a time in a directory and getting the
estimated number from st_nlink (yeah yeah, unportable, but it was the
first thing I thought of).
We might think we're processing 10 entries, but another other processes
might make our progress bar end at more or less than the 100% we
expected. That's OK, not something we should invoke BUG() about.
It doesn't have to be a BUG; a warning would suffice. And I hope not
finishing the expected number of items due to a catastrophic event is
rare enough that an additional warning wouldn't cause too much pain.
It's not a catastrophic event, just a run of the mill race condition
we'll expect if we're dealing with the real world.
E.g. you asked to unlink 1000 files, we do so, we find 10 are unlinked
already, or the command is asked to recursively unlink all files in a
directory tree, and new ones have showed up.
In those cases we should just just shrug and move on, no need for a
warning. We just don't always have perfect information about future
state at the start of the loop.
If a planned work item is cancelled then it can still be counted as
done. Or the total could be adjusted, but that might look awkward.
quoted
Loops that *regularly* end early are not a good fit for progress
percentages, I think.
Arguably yes, but in these fuzzy cases not providing a "total" means
showing no progress at all, just a counter. Perhaps we should have some
other "provide total, and it may be fuzzy" flag. Not providing it might
run into your proposed BUG(), my point was that the current API
providing this flexibility is intentional.
Your patch turns a loop that doesn't immediately report skipped items
into one with contiguous progress updates. That's a good way to deal
with the imagined restrictions for error detection. Another would be
to make the warnings optional.
quoted
quoted
Similarly, the n=0 being distinguishable from the first
display_progress() is actually useful in practice. It's something I've
seen git.git emit (not recently, I patched the relevant code to emit
more granular progress).
It's useful to know that we're stalling on the setup code before the
for-loop, not on the first item.
Hmm, preparations that take a noticeable time might deserve their own
progress line.
Sure, and I've split some of those up in the past, but this seems like
ducking/not addressing the point that the API use we disagree on has
your preferred use conflating these conditions, but mine does not...
Subtle. If preparation takes a long time and each item less than that
then the progress display is likely to jump from "0/n" to "i/n", where
i > 1, and the meaning of "1/n" becomes moot.
The progress display could show just the title before the first
display_progress() call to make the distinction clear. Would it really
be useful, though? Perhaps a trace2 region started by the first
display_progress() call and ended by the last one (n == total) would
be better.
quoted
Anyway, if no guard rails can be built then we have to rely on our math
skills alone. Off-by-one errors may look silly, but are no joke -- they
are surprisingly easy to make.
...which, regardless of whether one views a progress of "1/5 items" has
"finished 1/5" or "working on 1/5", which I think *in general* is an
arbitrary choice, I think the progress.c API we have in git.git clearly
fits the usage I'm describing better.
How so? start_progress() specifies a title and the total number of
items, display_progress() reports some other number that is shown in
relation to the total, and stop_progress() finishes the progress line.
This API is not documented and thus its meaning is (strictly speaking)
left unspecified.
It can be used to show a classic "percent-done progress indicator", as
https://dl.acm.org/doi/10.1145/1165385.317459 calls it. That's how I
read a growing percentage shown by a program, and "done" I understand
as "has been done" (completed), not as "is being done".
Wikipedia sent me to
https://chrisharrison.net/projects/progressbars/ProgBarHarrison.pdf,
which has some fun ideas on how to warp the perception of time for
users staring at a progress bar, but also states typical ones grow
with the amount of completed work.
René
Am 14.06.21 um 13:07 schrieb Ævar Arnfjörð Bjarmason:
quoted
On Thu, Jun 10 2021, René Scharfe wrote:
quoted
Am 09.06.21 um 00:12 schrieb Ævar Arnfjörð Bjarmason:
quoted
On Tue, Jun 08 2021, René Scharfe wrote:
quoted
I wonder (only in a semi-curious way, though) if we can detect
off-by-one errors by adding an assertion to display_progress() that
requires the first update to have the value 0, and in stop_progress()
one that requires the previous display_progress() call to have a value
equal to the total number of work items. Not sure it'd be worth the
hassle..
That's intentional. We started eating 3 apples, got to one, but now our
house is on fire and we're eating no more apples today, even if we
planned to eat 3 when we sat down.
The progress bar reflects this unexpected but recoverable state:
$ perl -wE 'for (0..1) { say "update"; say "progress $_" }' |
./helper/test-tool progress --total=3 Apples 2>&1 |
cat -v | perl -pe 's/\^M\K/\n/g'
Apples: 0% (0/3)^M
Apples: 33% (1/3)^M
Apples: 33% (1/3), done.
We're at 1/3, but we're done. No more apples.
This isn't just some hypothetical, e.g. consider neeing to unlink() or
remove files/directories one at a time in a directory and getting the
estimated number from st_nlink (yeah yeah, unportable, but it was the
first thing I thought of).
We might think we're processing 10 entries, but another other processes
might make our progress bar end at more or less than the 100% we
expected. That's OK, not something we should invoke BUG() about.
It doesn't have to be a BUG; a warning would suffice. And I hope not
finishing the expected number of items due to a catastrophic event is
rare enough that an additional warning wouldn't cause too much pain.
It's not a catastrophic event, just a run of the mill race condition
we'll expect if we're dealing with the real world.
E.g. you asked to unlink 1000 files, we do so, we find 10 are unlinked
already, or the command is asked to recursively unlink all files in a
directory tree, and new ones have showed up.
In those cases we should just just shrug and move on, no need for a
warning. We just don't always have perfect information about future
state at the start of the loop.
If a planned work item is cancelled then it can still be counted as
done. Or the total could be adjusted, but that might look awkward.
quoted
quoted
Loops that *regularly* end early are not a good fit for progress
percentages, I think.
Arguably yes, but in these fuzzy cases not providing a "total" means
showing no progress at all, just a counter. Perhaps we should have some
other "provide total, and it may be fuzzy" flag. Not providing it might
run into your proposed BUG(), my point was that the current API
providing this flexibility is intentional.
Your patch turns a loop that doesn't immediately report skipped items
into one with contiguous progress updates. That's a good way to deal
with the imagined restrictions for error detection. Another would be
to make the warnings optional.
I don't see how there's anything wrong with the API use, how it needs a
warning etc.
quoted
quoted
quoted
Similarly, the n=0 being distinguishable from the first
display_progress() is actually useful in practice. It's something I've
seen git.git emit (not recently, I patched the relevant code to emit
more granular progress).
It's useful to know that we're stalling on the setup code before the
for-loop, not on the first item.
Hmm, preparations that take a noticeable time might deserve their own
progress line.
Sure, and I've split some of those up in the past, but this seems like
ducking/not addressing the point that the API use we disagree on has
your preferred use conflating these conditions, but mine does not...
Subtle. If preparation takes a long time and each item less than that
then the progress display is likely to jump from "0/n" to "i/n", where
i > 1, and the meaning of "1/n" becomes moot.
In practice we're making humongous jumps all over the place, we don't
write to the terminal for every item processed, and if we did it would
be too fast to be perceptable to the user.
So I don't think this is an issue in the first place, as noted upthread
in [off-list ref]. Regardless of what we think of
the supposed off-by-one issue you seemed to think that it was enough of
an issue to justify complexity at the API use level (needing to think
about "continue" statements in loops, etc.), but now you think it's
moot?
The progress display could show just the title before the first
display_progress() call to make the distinction clear. Would it really
be useful, though? Perhaps a trace2 region started by the first
display_progress() call and ended by the last one (n == total) would
be better.
Yes, it should display 0/N if it's stalled. I have some WIP patches to
do that. I misrecalled that we were updating the progress from the
signal handler, but no, we wait for the first display(), but if we just
call display() from the handler with a "stalled" message we'll clearly
show these cases where we're stalling in the preparation.
quoted
quoted
Anyway, if no guard rails can be built then we have to rely on our math
skills alone. Off-by-one errors may look silly, but are no joke -- they
are surprisingly easy to make.
...which, regardless of whether one views a progress of "1/5 items" has
"finished 1/5" or "working on 1/5", which I think *in general* is an
arbitrary choice, I think the progress.c API we have in git.git clearly
fits the usage I'm describing better.
How so? start_progress() specifies a title and the total number of
items, display_progress() reports some other number that is shown in
relation to the total, and stop_progress() finishes the progress line.
This API is not documented and thus its meaning is (strictly speaking)
left unspecified.
I don't mean that it's clearly documented, I'm making the case that the
API is most sanely used in the way I've laid out. It seems we're
respectfully disagreeing on that, but ...
It can be used to show a classic "percent-done progress indicator", as
https://dl.acm.org/doi/10.1145/1165385.317459 calls it. That's how I
read a growing percentage shown by a program, and "done" I understand
as "has been done" (completed), not as "is being done".
...I think that's pretty moot in the first place, but I also think
thinking about progress.c's idea of the world as "has been done" doesn't
make sense when you look at its API holistically.
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:
+Working hard: 50% (1/2)<CR>
+Working hard: 50% (1/2), 1.91 MiB | 195.00 KiB/s<CR>
+Working hard: 50% (1/2), 2.86 MiB | 146.00 KiB/s<CR>
+Working hard: 50% (1/2), 3.81 MiB | 130.00 KiB/s<CR>
+Working hard: 50% (1/2), 3.81 MiB | 97.00 KiB/s, done.
Given something like (I've locally modified this a bit):
cat >in <<-\EOF &&
start 2
throughput 1 10000
update
progress 1
update
throughput 100000 10000
update
throughput 2000000 20000
update
throughput 3000000 30000
update
throughput 4000000 40000
update
stop
EOF
test-tool progress <in 2>stderr &&
That's another reason I'm maintaining that reporting progress as "is
being done" is the only sane way to look at it, because if you think it's
"has been done" you preclude the API from being used for cases where you
e.g. want to download 2 files, each file takes 1 minute, and you want to
show progress on the item itself.
Our API mostly just stops before the "progress on the item itself", but
if you think that layer should be "has been done" doesn't that mean that
display_throughput() would have to be entirely redesigned?
After all there's no way to feed a N for item number into it, it
optionally *updates* the progress on an existing item, such a thing
can't exist if we only call display_progress() to report items as being
done.
I do think the actual output we show is pretty crappy for that use-case,
but that output can be changed, but not really if we insist on the
invariant you're maintaining that display_progress() must only be called
if the item is done.
Just like showing a "stalled" message if we get a signal before we show
the 1st item doing that is actually pretty easy, i.e. we could showh 1/2
downloaded files, and then as we get signals show a spinner...
From: René Scharfe <hidden> Date: 2021-06-15 15:14:15
Am 14.06.21 um 21:08 schrieb Ævar Arnfjörð Bjarmason:
On Mon, Jun 14 2021, René Scharfe wrote:
quoted
Am 14.06.21 um 13:07 schrieb Ævar Arnfjörð Bjarmason:
quoted
On Thu, Jun 10 2021, René Scharfe wrote:
quoted
Am 09.06.21 um 00:12 schrieb Ævar Arnfjörð Bjarmason:
quoted
On Tue, Jun 08 2021, René Scharfe wrote:
quoted
I wonder (only in a semi-curious way, though) if we can detect
off-by-one errors by adding an assertion to display_progress() that
requires the first update to have the value 0, and in stop_progress()
one that requires the previous display_progress() call to have a value
equal to the total number of work items. Not sure it'd be worth the
hassle..
That's intentional. We started eating 3 apples, got to one, but now our
house is on fire and we're eating no more apples today, even if we
planned to eat 3 when we sat down.
The progress bar reflects this unexpected but recoverable state:
$ perl -wE 'for (0..1) { say "update"; say "progress $_" }' |
./helper/test-tool progress --total=3 Apples 2>&1 |
cat -v | perl -pe 's/\^M\K/\n/g'
Apples: 0% (0/3)^M
Apples: 33% (1/3)^M
Apples: 33% (1/3), done.
We're at 1/3, but we're done. No more apples.
This isn't just some hypothetical, e.g. consider neeing to unlink() or
remove files/directories one at a time in a directory and getting the
estimated number from st_nlink (yeah yeah, unportable, but it was the
first thing I thought of).
We might think we're processing 10 entries, but another other processes
might make our progress bar end at more or less than the 100% we
expected. That's OK, not something we should invoke BUG() about.
It doesn't have to be a BUG; a warning would suffice. And I hope not
finishing the expected number of items due to a catastrophic event is
rare enough that an additional warning wouldn't cause too much pain.
It's not a catastrophic event, just a run of the mill race condition
we'll expect if we're dealing with the real world.
E.g. you asked to unlink 1000 files, we do so, we find 10 are unlinked
already, or the command is asked to recursively unlink all files in a
directory tree, and new ones have showed up.
In those cases we should just just shrug and move on, no need for a
warning. We just don't always have perfect information about future
state at the start of the loop.
If a planned work item is cancelled then it can still be counted as
done. Or the total could be adjusted, but that might look awkward.
quoted
quoted
Loops that *regularly* end early are not a good fit for progress
percentages, I think.
Arguably yes, but in these fuzzy cases not providing a "total" means
showing no progress at all, just a counter. Perhaps we should have some
other "provide total, and it may be fuzzy" flag. Not providing it might
run into your proposed BUG(), my point was that the current API
providing this flexibility is intentional.
Your patch turns a loop that doesn't immediately report skipped items
into one with contiguous progress updates. That's a good way to deal
with the imagined restrictions for error detection. Another would be
to make the warnings optional.
I don't see how there's anything wrong with the API use, how it needs a
warning etc.
You pointed out that many callsites do:
for (i = 0; i < large_number; i++) {
display_progress(p, i + 1);
/* work work work */
}
This is an off-by-one error because a finished item is reported before
work on it starts. Adding a warning can help find these cases reliably.
quoted
quoted
quoted
quoted
Similarly, the n=0 being distinguishable from the first
display_progress() is actually useful in practice. It's something I've
seen git.git emit (not recently, I patched the relevant code to emit
more granular progress).
It's useful to know that we're stalling on the setup code before the
for-loop, not on the first item.
Hmm, preparations that take a noticeable time might deserve their own
progress line.
Sure, and I've split some of those up in the past, but this seems like
ducking/not addressing the point that the API use we disagree on has
your preferred use conflating these conditions, but mine does not...
Subtle. If preparation takes a long time and each item less than that
then the progress display is likely to jump from "0/n" to "i/n", where
i > 1, and the meaning of "1/n" becomes moot.
In practice we're making humongous jumps all over the place, we don't
write to the terminal for every item processed, and if we did it would
be too fast to be perceptable to the user.
So I don't think this is an issue in the first place, as noted upthread
in [off-list ref]. Regardless of what we think of
the supposed off-by-one issue you seemed to think that it was enough of
an issue to justify complexity at the API use level (needing to think
about "continue" statements in loops, etc.), but now you think it's
moot?
I don't understand your question. Let me rephrase what I find moot:
You wrote that the first display_progress() call being made with n>0
would be useful to you to see long-running preparations. If items are
processed quicker than one per second, then whatever number the first
display_progress() call writes to the screen will be overwritten within
a second. So the value of the first update shouldn't actually matter
much for your use case -- unless items takes a long time to process.
René
Am 14.06.21 um 21:08 schrieb Ævar Arnfjörð Bjarmason:
quoted
On Mon, Jun 14 2021, René Scharfe wrote:
quoted
Am 14.06.21 um 13:07 schrieb Ævar Arnfjörð Bjarmason:
quoted
On Thu, Jun 10 2021, René Scharfe wrote:
quoted
Am 09.06.21 um 00:12 schrieb Ævar Arnfjörð Bjarmason:
quoted
On Tue, Jun 08 2021, René Scharfe wrote:
quoted
I wonder (only in a semi-curious way, though) if we can detect
off-by-one errors by adding an assertion to display_progress() that
requires the first update to have the value 0, and in stop_progress()
one that requires the previous display_progress() call to have a value
equal to the total number of work items. Not sure it'd be worth the
hassle..
That's intentional. We started eating 3 apples, got to one, but now our
house is on fire and we're eating no more apples today, even if we
planned to eat 3 when we sat down.
The progress bar reflects this unexpected but recoverable state:
$ perl -wE 'for (0..1) { say "update"; say "progress $_" }' |
./helper/test-tool progress --total=3 Apples 2>&1 |
cat -v | perl -pe 's/\^M\K/\n/g'
Apples: 0% (0/3)^M
Apples: 33% (1/3)^M
Apples: 33% (1/3), done.
We're at 1/3, but we're done. No more apples.
This isn't just some hypothetical, e.g. consider neeing to unlink() or
remove files/directories one at a time in a directory and getting the
estimated number from st_nlink (yeah yeah, unportable, but it was the
first thing I thought of).
We might think we're processing 10 entries, but another other processes
might make our progress bar end at more or less than the 100% we
expected. That's OK, not something we should invoke BUG() about.
It doesn't have to be a BUG; a warning would suffice. And I hope not
finishing the expected number of items due to a catastrophic event is
rare enough that an additional warning wouldn't cause too much pain.
It's not a catastrophic event, just a run of the mill race condition
we'll expect if we're dealing with the real world.
E.g. you asked to unlink 1000 files, we do so, we find 10 are unlinked
already, or the command is asked to recursively unlink all files in a
directory tree, and new ones have showed up.
In those cases we should just just shrug and move on, no need for a
warning. We just don't always have perfect information about future
state at the start of the loop.
If a planned work item is cancelled then it can still be counted as
done. Or the total could be adjusted, but that might look awkward.
quoted
quoted
Loops that *regularly* end early are not a good fit for progress
percentages, I think.
Arguably yes, but in these fuzzy cases not providing a "total" means
showing no progress at all, just a counter. Perhaps we should have some
other "provide total, and it may be fuzzy" flag. Not providing it might
run into your proposed BUG(), my point was that the current API
providing this flexibility is intentional.
Your patch turns a loop that doesn't immediately report skipped items
into one with contiguous progress updates. That's a good way to deal
with the imagined restrictions for error detection. Another would be
to make the warnings optional.
I don't see how there's anything wrong with the API use, how it needs a
warning etc.
You pointed out that many callsites do:
for (i = 0; i < large_number; i++) {
display_progress(p, i + 1);
/* work work work */
}
This is an off-by-one error because a finished item is reported before
work on it starts. Adding a warning can help find these cases reliably.
I understand that we're respectfully disagreeing and that's what you
think, but I really don't think it helps anyone if we just repeat our
respective points.
I don't think it's off-by-one, but you do.
Yes, I understand that you think that progress bars should absolutely
never ever show 1/5 if the first item is not finished. I disagree and
think that's not intuitive, per my "eating an Apple" example
upthread.
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.
quoted
quoted
quoted
quoted
quoted
Similarly, the n=0 being distinguishable from the first
display_progress() is actually useful in practice. It's something I've
seen git.git emit (not recently, I patched the relevant code to emit
more granular progress).
It's useful to know that we're stalling on the setup code before the
for-loop, not on the first item.
Hmm, preparations that take a noticeable time might deserve their own
progress line.
Sure, and I've split some of those up in the past, but this seems like
ducking/not addressing the point that the API use we disagree on has
your preferred use conflating these conditions, but mine does not...
Subtle. If preparation takes a long time and each item less than that
then the progress display is likely to jump from "0/n" to "i/n", where
i > 1, and the meaning of "1/n" becomes moot.
In practice we're making humongous jumps all over the place, we don't
write to the terminal for every item processed, and if we did it would
be too fast to be perceptable to the user.
So I don't think this is an issue in the first place, as noted upthread
in [off-list ref]. Regardless of what we think of
the supposed off-by-one issue you seemed to think that it was enough of
an issue to justify complexity at the API use level (needing to think
about "continue" statements in loops, etc.), but now you think it's
moot?
I don't understand your question. Let me rephrase what I find moot:
You wrote that the first display_progress() call being made with n>0
would be useful to you to see long-running preparations. If items are
processed quicker than one per second, then whatever number the first
display_progress() call writes to the screen will be overwritten within
a second. So the value of the first update shouldn't actually matter
much for your use case -- unless items takes a long time to process.
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[...]
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.
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.
From: René Scharfe <hidden> Date: 2021-06-20 12:54:37
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..
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é