Re: [PATCH v2 4/5] bitmap: add trace2 outputs during open "bitmap" file
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Apr 21, 2022, 15:51 UTC
- Message-ID
- <220421.86k0bi77mb.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <2016ef2e342c2ec6517afa8ec3e57035021fb965.1650547400.git.dyroneteng@gmail.com>
On Thu, Apr 21 2022, Teng Long wrote:
Thanks for following up..
> 19:38:43.007840 common-main.c:49 | d0 | main | version | | | | | 2.35.1.582.g8e9092487a > 19:38:43.007874 common-main.c:50 | d0 | main | start | | 0.000305 | | | /opt/git/master/bin/git rev-list --test-bitmap HEAD
It's really helpful to have these full examples in the commit message. Thanks.
Show 15 quoted lines
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -312,9 +312,12 @@ char *pack_bitmap_filename(struct packed_git *p)
> static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
> struct multi_pack_index *midx)
> {
> + int fd;
> struct stat st;
> char *bitmap_name = midx_bitmap_filename(midx);
> - int fd = git_open(bitmap_name);
> + trace2_data_string("midx", the_repository, "try to open bitmap",
> + bitmap_name);
> + fd = git_open(bitmap_name);
>
> free(bitmap_name);Hrm, so re my comment on 5/5 are you trying to use the "try to open" as a timer to see what our start time is?
I think probably not, in which case this whole variable flip-around is something we won't need.
But if we do actually need it perhaps a sub-region for the timing?
Show 14 quoted lines
> @@ -511,11 +530,18 @@ static int open_midx_bitmap(struct repository *r,
> static int open_bitmap(struct repository *r,
> struct bitmap_index *bitmap_git)
> {
> - assert(!bitmap_git->map);
> + int ret = -1;
>
> - if (!open_midx_bitmap(r, bitmap_git))
> - return 0;
> - return open_pack_bitmap(r, bitmap_git);
> + assert(!bitmap_git->map);
> + trace2_region_enter("pack-bitmap", "open_bitmap", r);
> + if (!open_midx_bitmap(r, bitmap_git)) {
> + ret = 0;Nit: I think these "goto" patterns are best if your "int ret = N" picks an "N" which is the default that you'll "goto", i.e. if you pick "ret = 0" you'll just need "goto done" here....
> + goto done; > + } > + ret = open_pack_bitmap(r, bitmap_git);
...which we may then override here.
Just saves you the assignment and the {}, but it tends to add up in longer functions.
> +done:
> + trace2_region_leave("pack-bitmap", "open_bitmap", r);
> + return ret;
> }Looks good, aside from the 5/5 comments that much of the data string logging looks like it becomes redundant in the end due to error() giving us the same thing.
Show 27 quoted lines
> struct bitmap_index *prepare_bitmap_git(struct repository *r)
> diff --git a/repo-settings.c b/repo-settings.c
> index b4fbd16cdc..5bc7a97a6d 100644
> --- a/repo-settings.c
> +++ b/repo-settings.c
> @@ -8,6 +8,7 @@ static void repo_cfg_bool(struct repository *r, const char *key, int *dest,
> {
> if (repo_config_get_bool(r, key, dest))
> *dest = def;
> + trace2_data_string("config", r, key, *dest ? "true" : "false");
> }
>
> void prepare_repo_settings(struct repository *r)
> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> index eb63b71852..664cb88b0b 100755
> --- a/t/t5310-pack-bitmaps.sh
> +++ b/t/t5310-pack-bitmaps.sh
> @@ -421,8 +421,8 @@ test_expect_success 'complains about multiple pack bitmaps' '
> test_line_count = 2 bitmaps &&
>
> git rev-list --use-bitmap-index HEAD 2>err &&
> - grep "a bitmap has been opened" err &&
> - grep "ignoring extra bitmap file" err
> + grep "warning: a normal or midx bitmap already has been opened" err &&
> + grep "warning: ignoring extra bitmap file" err
> )
> 'I haven't tested but part of this test change looks like it'll break under bisect, i.e. you changed 1/2 of these strings in 3/5. Did you try with "git rebase -i -x 'make test T=t*bitmap*" or similar?