From: Patrick Steinhardt <ps@pks•im>
To: Jeff King <peff@peff•net>
Cc: git@vger•kernel.org
Subject: Re: [PATCH] cat-file: only use bitmaps when filtering
Date: Wed, 7 Jan 2026 09:20:59 +0100 [thread overview]
Message-ID: <aV4Xa9ceY4ahYj2m@pks.im> (raw)
In-Reply-To: <20260106102558.GA68914@coredump.intra.peff.net>
On Tue, Jan 06, 2026 at 05:25:58AM -0500, Jeff King wrote:
> There are some timing tests in 8002e8ee18 that claim the non-filter case
> is not regressed, but it's not clear to me exactly which commands were
> run. Given the size of the repo there and the fact that it's more I/O
> bound, I'd guess it is using an output format that requires looking at
> the packed objects.
I honestly can't remember anymore, either. I really should stick with
the actual command run in the Hyperfine benchmark names.
> You can see the mean user CPU does jump by almost 2 seconds in the
> timings given there. So it may be that the problem was there but drowned
> out by I/O noise. At any rate, the new t/perf tests isolate it better
> and reproduce consistently for me.
Could be, yeah.
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 505ddaa12f..3cb725940d 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -846,12 +846,14 @@ static void batch_each_object(struct batch_options *opt,
> .callback = callback,
> .payload = _payload,
> };
> - struct bitmap_index *bitmap = prepare_bitmap_git(the_repository);
> + struct bitmap_index *bitmap = NULL;
>
> for_each_loose_object(the_repository->objects, batch_one_object_loose, &payload, 0);
>
> - if (bitmap && !for_each_bitmapped_object(bitmap, &opt->objects_filter,
> - batch_one_object_bitmapped, &payload)) {
> + if (opt->objects_filter.choice != LOFC_DISABLED &&
> + (bitmap = prepare_bitmap_git(the_repository)) &&
> + !for_each_bitmapped_object(bitmap, &opt->objects_filter,
> + batch_one_object_bitmapped, &payload)) {
> struct packed_git *pack;
>
> repo_for_each_pack(the_repository, pack) {
Yeah, this seems like a reasonable change to me. I would've preferred to
avoid the assignment in the conditional, but other than that this looks
good to me.
Thanks!
Patrick
next prev parent reply other threads:[~2026-01-07 8:21 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-01-06 10:25 [PATCH] cat-file: only use bitmaps when filtering Jeff King
2026-01-07 8:20 ` Patrick Steinhardt [this message]
2026-01-16 16:45 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aV4Xa9ceY4ahYj2m@pks.im \
--to=ps@pks$(echo .)im \
--cc=git@vger$(echo .)kernel.org \
--cc=peff@peff$(echo .)net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox