From: Jeff King <peff@peff.net>
To: Taylor Blau <me@ttaylorr.com>
Cc: git@vger.kernel.org, Elijah Newren <newren@gmail.com>,
Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v2 09/23] pseudo-merge: implement support for selecting pseudo-merge commits
Date: Thu, 16 May 2024 04:07:04 -0400 [thread overview]
Message-ID: <20240516080704.GA85197@coredump.intra.peff.net> (raw)
In-Reply-To: <ZkK3KYkA6+zUxu9A@nand.local>
On Mon, May 13, 2024 at 08:58:17PM -0400, Taylor Blau wrote:
> > I was going to complain that explanatory text like this should probably
> > go into the documentation, not a commit message. But I see you do later
> > add documentation. Which seems to happen when this code is actually
> > wired up to the bitmap-writer. Maybe a moot point now that I figured it
> > out, but I think we'd be better off with the two commits squashed
> > together.
>
> I dunno. This commit is already rather large, and I like the split of
> "here's how we select these things", versus "now we actually start
> selecting/writing them".
>
> But maybe it results in a slightly awkward break in the middle that
> leaves some of the stuff that would otherwise fit well in the EXAMPLES
> section (as you mention below) in a weird limbo state.
It's not the break to me so much as the fact that you end up explaining
the concepts twice. Is it the same material presented in two ways? Or is
there stuff in one spot that is not in the other? I think the answer is
that it's a little bit of both. And as a reviewer (and an author) it's
hard to put yourself in the shoes of a user who is only going to see
what's in the docs.
So I'd rather even see the first patch as "here's some config; don't
worry about what it does too much, as we'll explain it in the next
commit" and then in the second patch say "go look at the config added by
this patch". And then we know we're looking at the same thing a user
will.
> There's a good amount of information already in
> Documentation/technical/bitmap-format.txt, though perhaps some of the
> pieces mentioned here could be added there. Let me know if you think one
> is missing something the other has (or if we could move significant
> portions of the latter into the former).
I don't think we should expect most users to read anything in
Documentation/technical. Now I don't expect most users to fiddle with
this feature at all. But reading over the config docs added by the
subsequent patch, it's not at all clear to me when I would want to tweak
the knobs or why.
I think there might need to be an "advanced packing concepts"
user-directed manual. Either as part of git-pack-objects(1), or maybe
broken out into its own page ("gitpacking(7)" or similar). Specifically
for this feature, I think it would want to cover:
- what is this thing, and why would I want it. You cover this in the
format doc, but I think it makes more sense in a user-directed doc
(and to leave the format doc strictly as a technical reference).
- what you wrote in "use cases" there is still, IMHO, introducing
things in the wrong order for a regular user. They're going to come
to the documentation either with a specific problem, or with an idea
to browse around for things that might help them.
So I feel like it needs to start with the concept and the
motivation. Something like (assuming we're following a section on
bitmaps in the "advanced packing" page, something I recognize also
does not yet exist):
Reachability bitmaps are most efficient when we have on-disk
stored bitmaps for one or more of the starting points of a
traversal. For this reason, Git prefers storing bitmaps for
commits at the tips of refs, because traversals tend to start with
those points.
But if you have a large number of refs, it's not feasible to store
a bitmap for _every_ ref tip. It takes up space, and just OR-ing
all of those bitmaps together is expensive.
One way we can deal with that is to create bitmaps that represent
_groups_ of refs. When a traversal asks about the entire group,
then we can use this single bitmap instead of considering each ref
individually. Because these bitmaps represent the set of objects
which would be reachable in a hypothetical merge of all of the
commits, we call them pseudo-merge bitmaps.
I don't think this is saying anything that your technical doc
doesn't, but I think it's more important what it _isn't_ saying.
We don't need to talk about commit bitmaps and merge bitmaps at all.
We just want the user to have the concept of grouping refs. And then
that would hopefully lead naturally into "OK, so how do we group
them".
- OK, so how do we group them? ;) I think there are two concerns here.
One is that traversals can only use a pseudo-merge bitmap if _every_
commit in its group is included in the traversal. So we want to
group the refs along logical boundaries (e.g., tags vs branches vs
remotes). Or in the case of shared-object repositories (like
GitHub's), by boundaries which only the user knows about.
And two is that we want groups that don't become invalidated when a
ref changes or is removed. So you want a bunch of old, stable tags
together, and probably don't want recent branches grouped at all.
And then when we describe the config knobs you can turn, it should
be in the user's mind how they can use them to influence those two
things. For "logical boundaries" part, I think the commit message
for patch 9 does some of that with the "refs/virtual" example. But
that's something I don't see as clearly in the config documentation
added in patch 10.
The knobs for handling age are more complicated and harder to
explain. ;) You do mention the power-law decay thing in patch 10,
but it's in the technical format docs. I think it should be
somewhere more user-accessible.
So hopefully that kind of lays out how I'm thinking about it. Both where
the docs go, but what I think are the useful ways to be thinking about
the feature. And not just for users, but as we see if the design is
doing a good job of fulfilling those needs.
I think the name/pattern config you introduce does cover the logical
boundaries in a clean and easy-to-understand way. The "age" stuff is
much fuzzier in my mind. Your power-law decay makes sense to me, though
it does have a lot of knobs, and I don't think we'll really understand
how it performs without real-world experimentation.
I do wonder if something stupidly simple like "just make a single group
including tags older than 3 months, and ignore everything else" (where
"single" is "single per logical boundary defined by the user") would
perform OK in practice. The point is to help "--all" and "--not --all"
when you have a bunch of crufty old refs. So really, the challenge is
mostly just identifying crufty old refs. ;)
But I do think your power-law stuff should be a functional superset of
that. And while it's complicated to reason about where the knobs should
be set, I don't think the code is very complex. And the fact that it
_has_ knobs gives something to tweak and gather data with.
All of which is to say, I guess, that I think the code is going in a
reasonable direction. It's hard to say much more without spending a
bunch of time benchmarking real repositories, their traversal queries,
and so on.
-Peff
next prev parent reply other threads:[~2024-05-16 8:07 UTC|newest]
Thread overview: 157+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-20 22:04 [PATCH 00/24] pack-bitmap: pseudo-merge reachability bitmaps Taylor Blau
2024-03-20 22:05 ` [PATCH 01/24] Documentation/technical: describe pseudo-merge bitmaps format Taylor Blau
2024-03-21 21:24 ` Junio C Hamano
2024-03-21 22:13 ` Taylor Blau
2024-03-21 22:22 ` Junio C Hamano
2024-03-20 22:05 ` [PATCH 02/24] config: repo_config_get_expiry() Taylor Blau
2024-04-10 17:54 ` Jeff King
2024-04-29 19:39 ` Taylor Blau
2024-03-20 22:05 ` [PATCH 03/24] ewah: implement `ewah_bitmap_is_subset()` Taylor Blau
2024-04-10 18:05 ` Jeff King
2024-04-29 19:47 ` Taylor Blau
2024-03-20 22:05 ` [PATCH 04/24] pack-bitmap: drop unused `max_bitmaps` parameter Taylor Blau
2024-04-10 18:06 ` Jeff King
2024-03-20 22:05 ` [PATCH 05/24] pack-bitmap: move some initialization to `bitmap_writer_init()` Taylor Blau
2024-04-10 18:10 ` Jeff King
2024-03-20 22:05 ` [PATCH 06/24] pseudo-merge.ch: initial commit Taylor Blau
2024-03-20 22:05 ` [PATCH 07/24] pack-bitmap-write: support storing pseudo-merge commits Taylor Blau
2024-03-20 22:05 ` [PATCH 08/24] pack-bitmap: implement `bitmap_writer_has_bitmapped_object_id()` Taylor Blau
2024-03-20 22:05 ` [PATCH 09/24] pack-bitmap: make `bitmap_writer_push_bitmapped_commit()` public Taylor Blau
2024-03-20 22:05 ` [PATCH 10/24] pseudo-merge: implement support for selecting pseudo-merge commits Taylor Blau
2024-03-20 22:05 ` [PATCH 11/24] pack-bitmap-write.c: select " Taylor Blau
2024-03-20 22:05 ` [PATCH 12/24] pack-bitmap-write.c: write pseudo-merge table Taylor Blau
2024-03-20 22:05 ` [PATCH 13/24] pack-bitmap: extract `read_bitmap()` function Taylor Blau
2024-03-20 22:05 ` [PATCH 14/24] pseudo-merge: scaffolding for reads Taylor Blau
2024-03-20 22:05 ` [PATCH 15/24] pack-bitmap.c: read pseudo-merge extension Taylor Blau
2024-03-20 22:05 ` [PATCH 16/24] pseudo-merge: implement support for reading pseudo-merge commits Taylor Blau
2024-03-20 22:05 ` [PATCH 17/24] ewah: implement `ewah_bitmap_popcount()` Taylor Blau
2024-03-20 22:05 ` [PATCH 18/24] pack-bitmap: implement test helpers for pseudo-merge Taylor Blau
2024-03-20 22:05 ` [PATCH 19/24] t/test-lib-functions.sh: support `--date` in `test_commit_bulk()` Taylor Blau
2024-03-20 22:05 ` [PATCH 20/24] pack-bitmap.c: use pseudo-merges during traversal Taylor Blau
2024-03-20 22:06 ` [PATCH 21/24] pack-bitmap: extra trace2 information Taylor Blau
2024-03-20 22:06 ` [PATCH 22/24] ewah: `bitmap_equals_ewah()` Taylor Blau
2024-03-20 22:06 ` [PATCH 23/24] pseudo-merge: implement support for finding existing merges Taylor Blau
2024-03-20 22:06 ` [PATCH 24/24] t/perf: implement performace tests for pseudo-merge bitmaps Taylor Blau
2024-03-21 19:50 ` [PATCH 00/24] pack-bitmap: pseudo-merge reachability bitmaps Junio C Hamano
2024-04-29 20:42 ` [PATCH v2 00/23] " Taylor Blau
2024-04-29 20:42 ` [PATCH v2 01/23] Documentation/technical: describe pseudo-merge bitmaps format Taylor Blau
2024-05-06 11:52 ` Patrick Steinhardt
2024-05-06 16:37 ` Taylor Blau
2024-05-10 11:46 ` Patrick Steinhardt
2024-05-13 19:47 ` Taylor Blau
2024-05-14 6:33 ` Patrick Steinhardt
2024-04-29 20:43 ` [PATCH v2 02/23] ewah: implement `ewah_bitmap_is_subset()` Taylor Blau
2024-04-29 20:43 ` [PATCH v2 03/23] pack-bitmap: drop unused `max_bitmaps` parameter Taylor Blau
2024-04-29 20:43 ` [PATCH v2 04/23] pack-bitmap: move some initialization to `bitmap_writer_init()` Taylor Blau
2024-05-06 11:52 ` Patrick Steinhardt
2024-05-06 18:24 ` Taylor Blau
2024-04-29 20:43 ` [PATCH v2 05/23] pseudo-merge.ch: initial commit Taylor Blau
2024-04-29 20:43 ` [PATCH v2 06/23] pack-bitmap-write: support storing pseudo-merge commits Taylor Blau
2024-05-06 11:52 ` Patrick Steinhardt
2024-05-06 18:48 ` Taylor Blau
2024-05-10 11:47 ` Patrick Steinhardt
2024-05-13 18:42 ` Jeff King
2024-05-13 20:19 ` Taylor Blau
2024-04-29 20:43 ` [PATCH v2 07/23] pack-bitmap: implement `bitmap_writer_has_bitmapped_object_id()` Taylor Blau
2024-04-29 20:43 ` [PATCH v2 08/23] pack-bitmap: make `bitmap_writer_push_bitmapped_commit()` public Taylor Blau
2024-05-13 18:50 ` Jeff King
2024-05-14 0:54 ` Taylor Blau
2024-04-29 20:43 ` [PATCH v2 09/23] pseudo-merge: implement support for selecting pseudo-merge commits Taylor Blau
2024-05-06 11:53 ` Patrick Steinhardt
2024-05-06 19:58 ` Taylor Blau
2024-05-13 19:03 ` Jeff King
2024-05-14 0:58 ` Taylor Blau
2024-05-16 8:07 ` Jeff King [this message]
2024-05-16 22:43 ` Junio C Hamano
2024-04-29 20:43 ` [PATCH v2 10/23] pack-bitmap-write.c: select " Taylor Blau
2024-05-06 11:53 ` Patrick Steinhardt
2024-05-06 20:05 ` Taylor Blau
2024-05-10 11:47 ` Patrick Steinhardt
2024-04-29 20:43 ` [PATCH v2 11/23] pack-bitmap-write.c: write pseudo-merge table Taylor Blau
2024-04-29 20:43 ` [PATCH v2 12/23] pack-bitmap: extract `read_bitmap()` function Taylor Blau
2024-04-29 20:43 ` [PATCH v2 13/23] pseudo-merge: scaffolding for reads Taylor Blau
2024-04-29 20:43 ` [PATCH v2 14/23] pack-bitmap.c: read pseudo-merge extension Taylor Blau
2024-04-29 20:44 ` [PATCH v2 15/23] pseudo-merge: implement support for reading pseudo-merge commits Taylor Blau
2024-04-29 20:44 ` [PATCH v2 16/23] ewah: implement `ewah_bitmap_popcount()` Taylor Blau
2024-04-29 20:44 ` [PATCH v2 17/23] pack-bitmap: implement test helpers for pseudo-merge Taylor Blau
2024-04-29 20:44 ` [PATCH v2 18/23] t/test-lib-functions.sh: support `--date` in `test_commit_bulk()` Taylor Blau
2024-04-29 20:44 ` [PATCH v2 19/23] pack-bitmap.c: use pseudo-merges during traversal Taylor Blau
2024-04-29 20:44 ` [PATCH v2 20/23] pack-bitmap: extra trace2 information Taylor Blau
2024-04-29 20:44 ` [PATCH v2 21/23] ewah: `bitmap_equals_ewah()` Taylor Blau
2024-04-29 20:44 ` [PATCH v2 22/23] pseudo-merge: implement support for finding existing merges Taylor Blau
2024-04-29 20:44 ` [PATCH v2 23/23] t/perf: implement performace tests for pseudo-merge bitmaps Taylor Blau
2024-04-30 20:03 ` [PATCH v2 00/23] pack-bitmap: pseudo-merge reachability bitmaps Junio C Hamano
2024-05-01 14:40 ` Taylor Blau
2024-05-21 19:01 ` [PATCH v3 00/30] " Taylor Blau
2024-05-21 19:01 ` [PATCH v3 01/30] object.h: add flags allocated by pack-bitmap.h Taylor Blau
2024-05-21 19:06 ` Taylor Blau
2024-05-21 19:01 ` [PATCH v3 07/30] Documentation/gitpacking.txt: initial commit Taylor Blau
2024-05-21 19:02 ` [PATCH v3 08/30] Documentation/gitpacking.txt: describe pseudo-merge bitmaps Taylor Blau
2024-05-21 19:02 ` [PATCH v3 09/30] Documentation/technical: describe pseudo-merge bitmaps format Taylor Blau
2024-05-21 19:02 ` [PATCH v3 10/30] ewah: implement `ewah_bitmap_is_subset()` Taylor Blau
2024-05-21 19:02 ` [PATCH v3 11/30] pack-bitmap: move some initialization to `bitmap_writer_init()` Taylor Blau
2024-05-21 19:02 ` [PATCH v3 12/30] pseudo-merge.ch: initial commit Taylor Blau
2024-05-21 19:02 ` [PATCH v3 13/30] pack-bitmap-write: support storing pseudo-merge commits Taylor Blau
2024-05-21 19:02 ` [PATCH v3 14/30] pack-bitmap: implement `bitmap_writer_has_bitmapped_object_id()` Taylor Blau
2024-05-21 19:02 ` [PATCH v3 15/30] pack-bitmap: make `bitmap_writer_push_bitmapped_commit()` public Taylor Blau
2024-05-21 19:02 ` [PATCH v3 16/30] config: introduce git_config_float() Taylor Blau
2024-05-23 10:02 ` Jeff King
2024-05-23 17:51 ` Taylor Blau
2024-05-21 19:02 ` [PATCH v3 17/30] pseudo-merge: implement support for selecting pseudo-merge commits Taylor Blau
2024-05-23 10:12 ` Jeff King
2024-05-23 17:56 ` Taylor Blau
2024-05-21 19:02 ` [PATCH v3 18/30] pack-bitmap-write.c: write pseudo-merge table Taylor Blau
2024-05-21 19:02 ` [PATCH v3 19/30] pack-bitmap: extract `read_bitmap()` function Taylor Blau
2024-05-21 19:02 ` [PATCH v3 20/30] pseudo-merge: scaffolding for reads Taylor Blau
2024-05-21 19:02 ` [PATCH v3 21/30] pack-bitmap.c: read pseudo-merge extension Taylor Blau
2024-05-21 19:02 ` [PATCH v3 22/30] pseudo-merge: implement support for reading pseudo-merge commits Taylor Blau
2024-05-23 10:40 ` Jeff King
2024-05-23 18:09 ` Taylor Blau
2024-05-21 19:02 ` [PATCH v3 23/30] ewah: implement `ewah_bitmap_popcount()` Taylor Blau
2024-05-21 19:02 ` [PATCH v3 24/30] pack-bitmap: implement test helpers for pseudo-merge Taylor Blau
2024-05-21 19:02 ` [PATCH v3 25/30] t/test-lib-functions.sh: support `--date` in `test_commit_bulk()` Taylor Blau
2024-05-23 10:42 ` Jeff King
2024-05-23 15:45 ` Junio C Hamano
2024-05-23 18:23 ` Taylor Blau
2024-05-21 19:03 ` [PATCH v3 26/30] pack-bitmap.c: use pseudo-merges during traversal Taylor Blau
2024-05-23 10:48 ` Jeff King
2024-05-23 18:23 ` Taylor Blau
2024-05-21 19:03 ` [PATCH v3 27/30] pack-bitmap: extra trace2 information Taylor Blau
2024-05-21 19:03 ` [PATCH v3 28/30] ewah: `bitmap_equals_ewah()` Taylor Blau
2024-05-21 19:03 ` [PATCH v3 29/30] pseudo-merge: implement support for finding existing merges Taylor Blau
2024-05-21 19:03 ` [PATCH v3 30/30] t/perf: implement performace tests for pseudo-merge bitmaps Taylor Blau
2024-05-23 10:54 ` Jeff King
2024-05-23 19:53 ` Taylor Blau
2024-05-25 3:13 ` Jeff King
2024-05-23 11:05 ` [PATCH v3 00/30] pack-bitmap: pseudo-merge reachability bitmaps Jeff King
2024-05-23 20:04 ` Taylor Blau
2024-05-25 3:15 ` Jeff King
2024-05-23 20:42 ` Taylor Blau
2024-05-23 21:26 ` [PATCH v4 00/24] " Taylor Blau
2024-05-23 21:26 ` [PATCH v4 01/24] Documentation/gitpacking.txt: initial commit Taylor Blau
2024-05-23 21:26 ` [PATCH v4 02/24] Documentation/gitpacking.txt: describe pseudo-merge bitmaps Taylor Blau
2024-05-23 21:26 ` [PATCH v4 03/24] Documentation/technical: describe pseudo-merge bitmaps format Taylor Blau
2024-05-23 21:26 ` [PATCH v4 04/24] ewah: implement `ewah_bitmap_is_subset()` Taylor Blau
2024-05-23 21:26 ` [PATCH v4 05/24] pack-bitmap: move some initialization to `bitmap_writer_init()` Taylor Blau
2024-05-23 21:26 ` [PATCH v4 06/24] pseudo-merge.ch: initial commit Taylor Blau
2024-05-23 21:26 ` [PATCH v4 07/24] pack-bitmap-write: support storing pseudo-merge commits Taylor Blau
2024-05-23 21:26 ` [PATCH v4 08/24] pack-bitmap: implement `bitmap_writer_has_bitmapped_object_id()` Taylor Blau
2024-05-23 21:26 ` [PATCH v4 09/24] pack-bitmap: make `bitmap_writer_push_bitmapped_commit()` public Taylor Blau
2024-05-23 21:26 ` [PATCH v4 10/24] config: introduce `git_config_double()` Taylor Blau
2024-05-23 21:26 ` [PATCH v4 11/24] pseudo-merge: implement support for selecting pseudo-merge commits Taylor Blau
2024-05-25 3:22 ` Jeff King
2024-05-23 21:26 ` [PATCH v4 12/24] pack-bitmap-write.c: write pseudo-merge table Taylor Blau
2024-05-23 21:26 ` [PATCH v4 13/24] pack-bitmap: extract `read_bitmap()` function Taylor Blau
2024-05-23 21:26 ` [PATCH v4 14/24] pseudo-merge: scaffolding for reads Taylor Blau
2024-05-23 21:26 ` [PATCH v4 15/24] pack-bitmap.c: read pseudo-merge extension Taylor Blau
2024-05-23 21:26 ` [PATCH v4 16/24] pseudo-merge: implement support for reading pseudo-merge commits Taylor Blau
2024-05-23 21:27 ` [PATCH v4 17/24] ewah: implement `ewah_bitmap_popcount()` Taylor Blau
2024-05-23 21:27 ` [PATCH v4 18/24] pack-bitmap: implement test helpers for pseudo-merge Taylor Blau
2024-05-23 21:27 ` [PATCH v4 19/24] t/test-lib-functions.sh: support `--notick` in `test_commit_bulk()` Taylor Blau
2024-05-25 3:25 ` Jeff King
2024-05-23 21:27 ` [PATCH v4 20/24] pack-bitmap.c: use pseudo-merges during traversal Taylor Blau
2024-05-23 21:27 ` [PATCH v4 21/24] pack-bitmap: extra trace2 information Taylor Blau
2024-05-23 21:27 ` [PATCH v4 22/24] ewah: `bitmap_equals_ewah()` Taylor Blau
2024-05-23 21:27 ` [PATCH v4 23/24] pseudo-merge: implement support for finding existing merges Taylor Blau
2024-05-23 21:27 ` [PATCH v4 24/24] t/perf: implement performance tests for pseudo-merge bitmaps Taylor Blau
2024-05-25 3:26 ` [PATCH v4 00/24] pack-bitmap: pseudo-merge reachability bitmaps 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=20240516080704.GA85197@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=me@ttaylorr.com \
--cc=newren@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
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;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).