Message ID | patch-2.2-042f598826-20210607T144206Z-avarab@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | trivial progress.c API usage fixes | expand |
On 6/7/2021 10:43 AM, Ævar Arnfjörð Bjarmason wrote: > Fix a potential incorrect display of the number of items (off by one) > and stalling of the progress bar in refresh_index(). > > The off-by-one error is minor, we should say we're processing the 1st > item, not the 0th. This along with the next change also allows us to > remove the last display_progress() call outside the loop, as we'll > always have reached 100% now. This "pre-announce the progress" seems correct and is unlikely to have a user sitting at "100%" while the loop is actually doing work on that last cache entry. Thanks, -Stolee
On Mon, Jun 07 2021, Derrick Stolee wrote: > On 6/7/2021 10:43 AM, Ævar Arnfjörð Bjarmason wrote: >> Fix a potential incorrect display of the number of items (off by one) >> and stalling of the progress bar in refresh_index(). >> >> The off-by-one error is minor, we should say we're processing the 1st >> item, not the 0th. This along with the next change also allows us to >> remove the last display_progress() call outside the loop, as we'll >> always have reached 100% now. > > This "pre-announce the progress" seems correct and is unlikely > to have a user sitting at "100%" while the loop is actually doing > work on that last cache entry. I guess pre-announce v.s. post-announce is a matter of some philosophy, for O(n) when can we be said to be doing work on n[0]? We entered the for-loop and are doing work on that istate->cache[i] item, so I'd like to think of it more as post-announce :) In any case, I'm changing this to the established pattern we use in most other places in the codebase, this one was an odd one out. Thanks for the review of this.
Am 07.06.21 um 17:58 schrieb Ævar Arnfjörð Bjarmason: > > On Mon, Jun 07 2021, Derrick Stolee wrote: > >> On 6/7/2021 10:43 AM, Ævar Arnfjörð Bjarmason wrote: >>> Fix a potential incorrect display of the number of items (off by one) >>> and stalling of the progress bar in refresh_index(). >>> >>> The off-by-one error is minor, we should say we're processing the 1st >>> item, not the 0th. This along with the next change also allows us to >>> remove the last display_progress() call outside the loop, as we'll >>> always have reached 100% now. >> >> This "pre-announce the progress" seems correct and is unlikely >> to have a user sitting at "100%" while the loop is actually doing >> work on that last cache entry. > > I guess pre-announce v.s. post-announce is a matter of some philosophy, > for O(n) when can we be said to be doing work on n[0]? We entered the > for-loop and are doing work on that istate->cache[i] item, so I'd like > to think of it more as post-announce :) Say you have a single item to process and it takes a minute. The original code shows 0% for a minute, then 100% at the end. 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!?". 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); } > In any case, I'm changing this to the established pattern we use in most > other places in the codebase, this one was an odd one out. Consistency is a good thing, but perhaps some of these other places should be changed. It doesn't matter much because most items git deals with are processed quickly, so an off-by-one error should barely be noticeable, but still it would be nice to get it right. It's hard to test, though. René
On Mon, Jun 07 2021, René Scharfe wrote: > Am 07.06.21 um 17:58 schrieb Ævar Arnfjörð Bjarmason: >> >> On Mon, Jun 07 2021, Derrick Stolee wrote: >> >>> On 6/7/2021 10:43 AM, Ævar Arnfjörð Bjarmason wrote: >>>> Fix a potential incorrect display of the number of items (off by one) >>>> and stalling of the progress bar in refresh_index(). >>>> >>>> The off-by-one error is minor, we should say we're processing the 1st >>>> item, not the 0th. This along with the next change also allows us to >>>> remove the last display_progress() call outside the loop, as we'll >>>> always have reached 100% now. >>> >>> This "pre-announce the progress" seems correct and is unlikely >>> to have a user sitting at "100%" while the loop is actually doing >>> work on that last cache entry. >> >> I guess pre-announce v.s. post-announce is a matter of some philosophy, >> for O(n) when can we be said to be doing work on n[0]? We entered the >> for-loop and are doing work on that istate->cache[i] item, so I'd like >> to think of it more as post-announce :) > > Say you have a single item to process and it takes a minute. The > original code shows 0% for a minute, then 100% at the end. 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!?". Perhaps if we said "100% and Reticulating splines[...]" :) > 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); > } But yes, I agree with the issue in theory, but I think in practice we don't need to worry about these 100% cases. We usually only display this anyway with a really big O(n), or (if we correctly use the API) one where each item isn't that expensive, we just do a lot of work in the aggregate. So having a display_progress() at the top of the for-loop with "i + 1" avoids needing two of them, or worrying about "continue" statements etc, or (as in this case) where the data we're processing can be 10k items with the first 8k being items we skip, so we'd be seen to hang, or "jump" from 10% to 50%, then smoothly update 50%..60%, and jump again etc.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> 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.
On Tue, Jun 08 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >>> 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
Am 08.06.21 um 12:58 schrieb Ævar Arnfjörð Bjarmason: > > On Tue, Jun 08 2021, Junio C Hamano wrote: > >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >>>> 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é
On Tue, Jun 08 2021, René Scharfe wrote: > Am 08.06.21 um 12:58 schrieb Ævar Arnfjörð Bjarmason: >> >> On Tue, Jun 08 2021, Junio C Hamano wrote: >> >>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >>> >>>>> 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 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.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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* <eaf2b6b0-4202-d5ea-87a2-b828fdbc60a1@web.de> > 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); > }
Am 10.06.21 um 07:30 schrieb Junio C Hamano: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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*]. 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é diff --git a/t/helper/test-progress.c b/t/helper/test-progress.c index 5d05cbe789..b6589f3878 100644 --- a/t/helper/test-progress.c +++ b/t/helper/test-progress.c @@ -65,6 +65,8 @@ int cmd__progress(int argc, const char **argv) display_throughput(progress, byte_count); } else if (!strcmp(line.buf, "update")) progress_test_force_update(); + else if (skip_prefix(line.buf, "echo ", (const char **) &end)) + fprintf(stderr, "%s\n", end); else die("invalid input: '%s'\n", line.buf); }
Am 09.06.21 um 00:12 schrieb Ævar Arnfjörð Bjarmason: > > On Tue, Jun 08 2021, René Scharfe wrote: > >> 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é
On Thu, Jun 10 2021, René Scharfe wrote: > Am 09.06.21 um 00:12 schrieb Ævar Arnfjörð Bjarmason: >> >> On Tue, Jun 08 2021, René Scharfe wrote: >> >>> 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. >> 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.
Am 14.06.21 um 13:07 schrieb Ævar Arnfjörð Bjarmason: > > On Thu, Jun 10 2021, René Scharfe wrote: > >> Am 09.06.21 um 00:12 schrieb Ævar Arnfjörð Bjarmason: >>> >>> On Tue, Jun 08 2021, René Scharfe wrote: >>> >>>> 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. >> 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. >>> 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. >> 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é
On Mon, Jun 14 2021, René Scharfe wrote: > Am 14.06.21 um 13:07 schrieb Ævar Arnfjörð Bjarmason: >> >> On Thu, Jun 10 2021, René Scharfe wrote: >> >>> Am 09.06.21 um 00:12 schrieb Ævar Arnfjörð Bjarmason: >>>> >>>> On Tue, Jun 08 2021, René Scharfe wrote: >>>> >>>>> 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. > >>> 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. >>>> 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 <8735tt4fhx.fsf@evledraar.gmail.com>. 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. >>> 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. > 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. 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...
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > 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. > ... > 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. Sorry, but I do not understand your argument here at all. If you show "has been completed", when such a thing starts, I'd see for a minute this: Downloading (0/2) ... X MiB | Y kiB/s and then it will switch to Downloading (1/2) ... progress ... and after staring at it for another minute, I'd see Downloading (2/2) ... done. And such an output makes sense to me. It is obvious, at least to me, that the output during the first minute is telling me that it hasn't finished but working hard to turn that "0" into "1" at the pace the throughput thing is showing.
Am 14.06.21 um 21:08 schrieb Ævar Arnfjörð Bjarmason: > > On Mon, Jun 14 2021, René Scharfe wrote: > >> Am 14.06.21 um 13:07 schrieb Ævar Arnfjörð Bjarmason: >>> >>> On Thu, Jun 10 2021, René Scharfe wrote: >>> >>>> Am 09.06.21 um 00:12 schrieb Ævar Arnfjörð Bjarmason: >>>>> >>>>> On Tue, Jun 08 2021, René Scharfe wrote: >>>>> >>>>>> 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. >> >>>> 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. >>>>> 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 <8735tt4fhx.fsf@evledraar.gmail.com>. 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é
On Tue, Jun 15 2021, René Scharfe wrote: > Am 14.06.21 um 21:08 schrieb Ævar Arnfjörð Bjarmason: >> >> On Mon, Jun 14 2021, René Scharfe wrote: >> >>> Am 14.06.21 um 13:07 schrieb Ævar Arnfjörð Bjarmason: >>>> >>>> On Thu, Jun 10 2021, René Scharfe wrote: >>>> >>>>> Am 09.06.21 um 00:12 schrieb Ævar Arnfjörð Bjarmason: >>>>>> >>>>>> On Tue, Jun 08 2021, René Scharfe wrote: >>>>>> >>>>>>> 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. >>> >>>>> 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. >>>>>> 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 <8735tt4fhx.fsf@evledraar.gmail.com>. 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.
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é
diff --git a/read-cache.c b/read-cache.c index 470f800855..8b0073a839 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1594,6 +1594,8 @@ int refresh_index(struct index_state *istate, unsigned int flags, int t2_did_lstat = 0; int t2_did_scan = 0; + display_progress(progress, i + 1); + ce = istate->cache[i]; if (ignore_submodules && S_ISGITLINK(ce->ce_mode)) continue; @@ -1627,7 +1629,6 @@ int refresh_index(struct index_state *istate, unsigned int flags, t2_sum_scan += t2_did_scan; if (new_entry == ce) continue; - display_progress(progress, i); if (!new_entry) { const char *fmt; @@ -1662,7 +1663,6 @@ int refresh_index(struct index_state *istate, unsigned int flags, trace2_data_intmax("index", NULL, "refresh/sum_lstat", t2_sum_lstat); trace2_data_intmax("index", NULL, "refresh/sum_scan", t2_sum_scan); trace2_region_leave("index", "refresh", NULL); - display_progress(progress, istate->cache_nr); stop_progress(&progress); trace_performance_leave("refresh index"); return has_errors;
Fix a potential incorrect display of the number of items (off by one) and stalling of the progress bar in refresh_index(). The off-by-one error is minor, we should say we're processing the 1st item, not the 0th. This along with the next change also allows us to remove the last display_progress() call outside the loop, as we'll always have reached 100% now. Let's also move the display_progress() call to the very start of the loop refresh_index() loop. In the loop we first check whether e.g. we ignore submodules and the entry we're processing is a submodule, whether we ignore certain paths etc.. Thus we could have a pathological case where we have a huge index consisting of such ignored entries, and we'd stall on the progress bar. See ae9af12287 (status: show progress bar if refreshing the index takes too long, 2018-09-15) for the initial addition of this progress bar to refresh_index(). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- read-cache.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)