| Message ID | 3048b4dd2982932fa11ba8393895fa33a00a5b58.1648119652.git.dyroneteng@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | trace2 output for bitmap decision path | expand |
On Thu, Mar 24, 2022 at 07:43:59PM +0800, Teng Long wrote: > diff --git a/pack-bitmap.c b/pack-bitmap.c > index 9c666cdb8b..931219adf0 100644 > --- a/pack-bitmap.c > +++ b/pack-bitmap.c > @@ -494,15 +494,18 @@ static int open_pack_bitmap(struct repository *r, > static int open_midx_bitmap(struct repository *r, > struct bitmap_index *bitmap_git) > { > + int ret = -1; > struct multi_pack_index *midx; > > assert(!bitmap_git->map); > > for (midx = get_multi_pack_index(r); midx; midx = midx->next) { > - if (!open_midx_bitmap_1(bitmap_git, midx)) > - return 0; > + if (!open_midx_bitmap_1(bitmap_git, midx)) { > + ret = 0; > + break; > + } > } > - return -1; > + return ret; This all looks like good clean-up to me, and it indeed preserves the behavior before and after this patch is applied. But thinking about some of my comments on patch 2/3 here, I think that we don't want to break out of that loop until we have visited both the MIDX in our repository, as well as any alternates (along with _their_ alternates, recursively). That _is_ a behavior change with respect to the existing implementation on master, but I think that what's on master is wrong to stop after looking at the first MIDX bitmap. At least, it's wrong in the same sense of: "we will only load _one_ of these MIDX bitmaps, so if there is more than one to choose from, the caller is mistaken". I think instead we'd want to do something like this on top: --- 8< --- diff --git a/pack-bitmap.c b/pack-bitmap.c index 410020c4d3..0c6640b0f6 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -500,10 +500,8 @@ static int open_midx_bitmap(struct repository *r, assert(!bitmap_git->map); for (midx = get_multi_pack_index(r); midx; midx = midx->next) { - if (!open_midx_bitmap_1(bitmap_git, midx)) { + if (!open_midx_bitmap_1(bitmap_git, midx)) ret = 0; - break; - } } return ret; } --- >8 --- Thanks, Taylor
On Thu, 24 Mar 2022 15:11:08 -0400, Taylor Blau wrote: > But thinking about some of my comments on patch 2/3 here, I think that > we don't want to break out of that loop until we have visited both the > MIDX in our repository, as well as any alternates (along with _their_ > alternates, recursively). > > That _is_ a behavior change with respect to the existing implementation > on master, but I think that what's on master is wrong to stop after > looking at the first MIDX bitmap. At least, it's wrong in the same sense > of: "we will only load _one_ of these MIDX bitmaps, so if there is more > than one to choose from, the caller is mistaken". I'm a little wondering that what's the practial meaning for _ do not _ stop after looking at the first MIDX bitmap? Although all MIDX bitmap are scanned, only one of them will eventually work , and there seems to be no guarantee that the last MIDX that works is the most appropriate one? (with the same comfusion applies to non-MIDX bitmap's behavour...) Thanks.
On Mon, Mar 28, 2022 at 03:59:07PM +0800, Teng Long wrote: > Although all MIDX bitmap are scanned, only one of them will eventually work > , and there seems to be no guarantee that the last MIDX that works is the > most appropriate one? (with the same comfusion applies to non-MIDX > bitmap's behavour...) I think your "which is the most appropriate one?" exactly highlights the problem. Since Git can only handle one .bitmap file at a time, we should tell the user when we found more than one candidate. In this case, they may be expecting or depending on a particular choice for performance reasons, and so may be rather surprised or upset if Git chose a different available .bitmap. The one exception to this is when we have both a single- and multi-pack bitmap available, in which case it almost always makes more sense to pick the multi-pack bitmap. So we don't complain in this case (i.e., if open_midx_bitmap() succeeds, we immediately return success from open_bitmap() and proceed to have prepare_bitmap_git() call load_bitmap() on the previous result without even bothering to check for pack bitmaps). Thanks, Taylor
diff --git a/pack-bitmap.c b/pack-bitmap.c index 9c666cdb8b..931219adf0 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -494,15 +494,18 @@ static int open_pack_bitmap(struct repository *r, static int open_midx_bitmap(struct repository *r, struct bitmap_index *bitmap_git) { + int ret = -1; struct multi_pack_index *midx; assert(!bitmap_git->map); for (midx = get_multi_pack_index(r); midx; midx = midx->next) { - if (!open_midx_bitmap_1(bitmap_git, midx)) - return 0; + if (!open_midx_bitmap_1(bitmap_git, midx)) { + ret = 0; + break; + } } - return -1; + return ret; } static int open_bitmap(struct repository *r,
Let's use "ret" value for "return" statement in "open_midx_bitmap()" just as the same way as int "open_pack_bitmap()". Signed-off-by: Teng Long <dyroneteng@gmail.com> --- pack-bitmap.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)