A couple of small fixes to the usage of the progress.c API. I have
some subsequent changes queued up locally, but thought it would be
better to submit this more incrementally, and to focus on smaller
cleanups in the immediate post-release period.
Ævar Arnfjörð Bjarmason (2):
read-cache.c: don't guard calls to progress.c API
read-cache: fix incorrect count and progress bar stalling
read-cache.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
--
2.32.0.rc3.434.gd8aed1f08a7
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 <redacted>
---
read-cache.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Don't guard the calls to the progress.c API with "if (progress)". The
API itself will check this. This doesn't change any behavior, but
makes this code consistent with the rest of the codebase.
See ae9af12287b (status: show progress bar if refreshing the index
takes too long, 2018-09-15) for the commit that added the pattern
we're changing here.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
read-cache.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
On 6/7/2021 10:43 AM, Ævar Arnfjörð Bjarmason wrote:
Don't guard the calls to the progress.c API with "if (progress)". The
API itself will check this. This doesn't change any behavior, but
makes this code consistent with the rest of the codebase.
Since stop_progress() closes a trace2 region, this actually
does make a change in behavior, I think. In a good way.
Thanks,
-Stolee
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 6/7/2021 10:43 AM, Ævar Arnfjörð Bjarmason wrote:
quoted
Don't guard the calls to the progress.c API with "if (progress)". The
API itself will check this. This doesn't change any behavior, but
makes this code consistent with the rest of the codebase.
Since stop_progress() closes a trace2 region, this actually
does make a change in behavior, I think. In a good way.
I don't see the behavior change.
Yes start_delayed_progress() will call start_progress_delay() which
mallocs and enters the trace2 region, and then if you don't call
stop_progress() at all you won't leave it.
But in read-cache.c both before & after my change we only malloc & only
enter the region if we're actually displaying the progress, there's an
isatty() guard on it.
Once we start that progress bar we will leave the trace2 region, via
stop_progress(), but note that stop_progress() will exit early if the
pointer you dereference is NULL and not do that, and since we'll have
"*progress = NULL" in the case of not wanting the progress bar we won't
leave the region we didn't enter in the first place.
The change here is just to remove the needless nano-optimization of
guarding the calls with a NULL check of "progress", which the API itself
does, no?
On 6/7/2021 10:43 AM, Ævar Arnfjörð Bjarmason wrote:
quoted
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.
On 6/7/2021 11:52 AM, Ævar Arnfjörð Bjarmason wrote:
On Mon, Jun 07 2021, Derrick Stolee wrote:
quoted
On 6/7/2021 10:43 AM, Ævar Arnfjörð Bjarmason wrote:
quoted
Don't guard the calls to the progress.c API with "if (progress)". The
API itself will check this. This doesn't change any behavior, but
makes this code consistent with the rest of the codebase.
Since stop_progress() closes a trace2 region, this actually
does make a change in behavior, I think. In a good way.
I don't see the behavior change.
Yes start_delayed_progress() will call start_progress_delay() which
mallocs and enters the trace2 region, and then if you don't call
stop_progress() at all you won't leave it.
But in read-cache.c both before & after my change we only malloc & only
enter the region if we're actually displaying the progress, there's an
isatty() guard on it.
That's right. I misremembered where this trace2 stuff was.
The idea we didn't pursue was to create the progress struct
unconditionally, and just leave it as "quiet" based on an
input parameter. That would keep the trace2 regions consistent.
But I'm wrong and there is no behavior change here.
Thanks,
-Stolee
From: René Scharfe <hidden> Date: 2021-06-07 19:20:56
Am 07.06.21 um 17:58 schrieb Ævar Arnfjörð Bjarmason:
On Mon, Jun 07 2021, Derrick Stolee wrote:
quoted
On 6/7/2021 10:43 AM, Ævar Arnfjörð Bjarmason wrote:
quoted
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é
Am 07.06.21 um 17:58 schrieb Ævar Arnfjörð Bjarmason:
quoted
On Mon, Jun 07 2021, Derrick Stolee wrote:
quoted
On 6/7/2021 10:43 AM, Ævar Arnfjörð Bjarmason wrote:
quoted
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.