git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Taylor Blau <me@ttaylorr.com>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org, Elijah Newren <newren@gmail.com>,
	Junio C Hamano <gitster@pobox.com>,
	Patrick Steinhardt <ps@pks.im>
Subject: Re: [PATCH v4 00/13] midx: incremental multi-pack indexes, part two
Date: Wed, 19 Mar 2025 20:18:52 -0400	[thread overview]
Message-ID: <Z9te7BVGeU6VCo8Y@nand.local> (raw)
In-Reply-To: <20250318022134.GD1473033@coredump.intra.peff.net>

On Mon, Mar 17, 2025 at 10:21:34PM -0400, Jeff King wrote:
> On Fri, Mar 14, 2025 at 04:18:12PM -0400, Taylor Blau wrote:
>
> > This is a new round of my series to implement bitmap support for
> > incremental multi-pack indexes (MIDXs). It has been rebased on current
> > 'master', which is 683c54c999 (Git 2.49, 2025-03-14) at the time of
> > writing.
>
> I read over this and didn't find anything objectionable (I left a few
> comments here and there). I think I've said this before with big
> bitmap/midx series: the biggest issue is that it's hard to know what you
> might have missed. Especially in terms of corner cases. So it all looks
> reasonable to me (including the overall design), but ultimately I think
> it's more fruitful to put it through the paces on real-looking data than
> it is to try to go over every inch of the midx code with a fine-tooth
> comb. And I'd guess the eventual fate here is for this code to get
> exercise on GitHub, which would help with that shaking out.

Thanks for reviewing it, and sorry that this series is so dense to begin
with.

I agree that the proof is really in the pudding here, and the best way
to confirm that we squashed everything is by putting real usage through
the new paths and seeing what shakes out.

I think the main thing that I was hoping for here were two things:

  1. That others thought the overall design and approach are sane, and
     that we're not painting ourselves into a corner.

  2. That we are unlikely to regress non-incremental bitmap usage.

> So mainly I tried to look for things that might hurt the non-incremental
> cases, and didn't see anything (modulo one or two questions about
> micro-optimizations, though I expect the answer there is "nothing big
> enough to measure"). So if this can progress towards the "shaking out"
> phase, and has the potential to hurt only people who turn on the new
> feature, that seems like a good path to me.

...and that's exactly what I got :-).

I'll send a small reroll shortly that addresses the comments from you
and Elijah (thanks, Elijah!) and I'll look forward to hearing what you
think of that round.

Thanks,
Taylor

  reply	other threads:[~2025-03-20  0:18 UTC|newest]

Thread overview: 136+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-08-15 21:01 [PATCH 00/13] midx: incremental multi-pack indexes, part two Taylor Blau
2024-08-15 21:01 ` [PATCH 01/13] Documentation: describe incremental MIDX bitmaps Taylor Blau
2024-08-15 21:01 ` [PATCH 02/13] pack-revindex: prepare for " Taylor Blau
2024-08-15 21:01 ` [PATCH 03/13] pack-bitmap.c: open and store incremental bitmap layers Taylor Blau
2024-08-15 21:01 ` [PATCH 04/13] pack-bitmap.c: teach `bitmap_for_commit()` about incremental MIDXs Taylor Blau
2024-08-15 21:01 ` [PATCH 05/13] pack-bitmap.c: teach `show_objects_for_type()` " Taylor Blau
2024-08-15 21:01 ` [PATCH 06/13] pack-bitmap.c: support bitmap pack-reuse with " Taylor Blau
2024-08-15 21:01 ` [PATCH 07/13] pack-bitmap.c: teach `rev-list --test-bitmap` about " Taylor Blau
2024-08-15 21:01 ` [PATCH 08/13] pack-bitmap.c: compute disk-usage with " Taylor Blau
2024-08-15 21:01 ` [PATCH 09/13] pack-bitmap.c: apply pseudo-merge commits " Taylor Blau
2024-08-15 21:01 ` [PATCH 10/13] ewah: implement `struct ewah_or_iterator` Taylor Blau
2024-08-15 21:01 ` [PATCH 11/13] pack-bitmap.c: keep track of each layer's type bitmaps Taylor Blau
2024-08-15 21:01 ` [PATCH 12/13] pack-bitmap.c: use `ewah_or_iterator` for type bitmap iterators Taylor Blau
2024-08-15 21:01 ` [PATCH 13/13] midx: implement writing incremental MIDX bitmaps Taylor Blau
2024-08-15 22:28 ` [PATCH v2 00/13] midx: incremental multi-pack indexes, part two Taylor Blau
2024-08-15 22:28   ` [PATCH v2 01/13] Documentation: describe incremental MIDX bitmaps Taylor Blau
2024-08-15 22:28   ` [PATCH v2 02/13] pack-revindex: prepare for " Taylor Blau
2024-08-15 22:28   ` [PATCH v2 03/13] pack-bitmap.c: open and store incremental bitmap layers Taylor Blau
2024-08-15 22:29   ` [PATCH v2 04/13] pack-bitmap.c: teach `bitmap_for_commit()` about incremental MIDXs Taylor Blau
2024-08-15 22:29   ` [PATCH v2 05/13] pack-bitmap.c: teach `show_objects_for_type()` " Taylor Blau
2024-08-15 22:29   ` [PATCH v2 06/13] pack-bitmap.c: support bitmap pack-reuse with " Taylor Blau
2024-08-15 22:29   ` [PATCH v2 07/13] pack-bitmap.c: teach `rev-list --test-bitmap` about " Taylor Blau
2024-08-15 22:29   ` [PATCH v2 08/13] pack-bitmap.c: compute disk-usage with " Taylor Blau
2024-08-15 22:29   ` [PATCH v2 09/13] pack-bitmap.c: apply pseudo-merge commits " Taylor Blau
2024-08-15 22:29   ` [PATCH v2 10/13] ewah: implement `struct ewah_or_iterator` Taylor Blau
2024-08-15 22:29   ` [PATCH v2 11/13] pack-bitmap.c: keep track of each layer's type bitmaps Taylor Blau
2024-08-15 22:29   ` [PATCH v2 12/13] pack-bitmap.c: use `ewah_or_iterator` for type bitmap iterators Taylor Blau
2024-08-15 22:29   ` [PATCH v2 13/13] midx: implement writing incremental MIDX bitmaps Taylor Blau
2024-08-28 17:55     ` [PATCH] fixup! " Junio C Hamano
2024-08-28 18:33       ` Jeff King
2024-08-29 18:57         ` Taylor Blau
2024-08-29 19:27           ` Jeff King
2024-11-19 20:56             ` Taylor Blau
2024-11-19 22:07 ` [PATCH v3 00/13] midx: incremental multi-pack indexes, part two Taylor Blau
2024-11-19 22:07   ` [PATCH v3 01/13] Documentation: describe incremental MIDX bitmaps Taylor Blau
2025-02-28 10:01     ` Patrick Steinhardt
2025-02-28 23:26       ` Taylor Blau
2025-03-03 10:54         ` Patrick Steinhardt
2024-11-19 22:07   ` [PATCH v3 02/13] pack-revindex: prepare for " Taylor Blau
2025-02-28 10:01     ` Patrick Steinhardt
2025-02-28 23:39       ` Taylor Blau
2024-11-19 22:07   ` [PATCH v3 03/13] pack-bitmap.c: open and store incremental bitmap layers Taylor Blau
2025-02-28 10:01     ` Patrick Steinhardt
2025-02-28 23:49       ` Taylor Blau
2025-03-03 10:55         ` Patrick Steinhardt
2024-11-19 22:07   ` [PATCH v3 04/13] pack-bitmap.c: teach `bitmap_for_commit()` about incremental MIDXs Taylor Blau
2025-02-28 10:01     ` Patrick Steinhardt
2025-03-01  0:12       ` Taylor Blau
2024-11-19 22:07   ` [PATCH v3 05/13] pack-bitmap.c: teach `show_objects_for_type()` " Taylor Blau
2024-11-19 22:07   ` [PATCH v3 06/13] pack-bitmap.c: support bitmap pack-reuse with " Taylor Blau
2025-02-28 10:01     ` Patrick Steinhardt
2025-03-01  0:16       ` Taylor Blau
2024-11-19 22:07   ` [PATCH v3 07/13] pack-bitmap.c: teach `rev-list --test-bitmap` about " Taylor Blau
2025-02-28 10:01     ` Patrick Steinhardt
2025-03-01  0:19       ` Taylor Blau
2024-11-19 22:07   ` [PATCH v3 08/13] pack-bitmap.c: compute disk-usage with " Taylor Blau
2024-11-19 22:07   ` [PATCH v3 09/13] pack-bitmap.c: apply pseudo-merge commits " Taylor Blau
2024-11-19 22:07   ` [PATCH v3 10/13] ewah: implement `struct ewah_or_iterator` Taylor Blau
2025-02-28 10:01     ` Patrick Steinhardt
2025-03-01  0:22       ` Taylor Blau
2024-11-19 22:07   ` [PATCH v3 11/13] pack-bitmap.c: keep track of each layer's type bitmaps Taylor Blau
2025-02-28 10:01     ` Patrick Steinhardt
2025-03-01  0:26       ` Taylor Blau
2024-11-19 22:07   ` [PATCH v3 12/13] pack-bitmap.c: use `ewah_or_iterator` for type bitmap iterators Taylor Blau
2025-02-28 10:01     ` Patrick Steinhardt
2025-03-01  0:28       ` Taylor Blau
2024-11-19 22:07   ` [PATCH v3 13/13] midx: implement writing incremental MIDX bitmaps Taylor Blau
2025-02-28 10:01     ` Patrick Steinhardt
2025-03-01  0:31       ` Taylor Blau
2024-11-20  8:49   ` [PATCH v3 00/13] midx: incremental multi-pack indexes, part two Junio C Hamano
2025-03-14 20:18 ` [PATCH v4 " Taylor Blau
2025-03-14 20:18   ` [PATCH v4 01/13] Documentation: describe incremental MIDX bitmaps Taylor Blau
2025-03-18  1:16     ` Jeff King
2025-03-18 23:11       ` Taylor Blau
2025-03-18  2:42     ` Elijah Newren
2025-03-18 23:19       ` Taylor Blau
2025-03-14 20:18   ` [PATCH v4 02/13] pack-revindex: prepare for " Taylor Blau
2025-03-18  1:27     ` Jeff King
2025-03-19  0:02       ` Taylor Blau
2025-03-19  0:07         ` Taylor Blau
2025-03-26 18:08           ` Jeff King
2025-03-18  2:43     ` Elijah Newren
2025-03-19  0:03       ` Taylor Blau
2025-03-14 20:18   ` [PATCH v4 03/13] pack-bitmap.c: open and store incremental bitmap layers Taylor Blau
2025-03-18  4:13     ` Elijah Newren
2025-03-19  0:08       ` Taylor Blau
2025-03-14 20:18   ` [PATCH v4 04/13] pack-bitmap.c: teach `bitmap_for_commit()` about incremental MIDXs Taylor Blau
2025-03-18  1:38     ` Jeff King
2025-03-19  0:13       ` Taylor Blau
2025-03-14 20:18   ` [PATCH v4 05/13] pack-bitmap.c: teach `show_objects_for_type()` " Taylor Blau
2025-03-14 20:18   ` [PATCH v4 06/13] pack-bitmap.c: support bitmap pack-reuse with " Taylor Blau
2025-03-18  4:13     ` Elijah Newren
2025-03-19  0:17       ` Taylor Blau
2025-03-14 20:18   ` [PATCH v4 07/13] pack-bitmap.c: teach `rev-list --test-bitmap` about " Taylor Blau
2025-03-18  5:31     ` Elijah Newren
2025-03-19  0:30       ` Taylor Blau
2025-03-14 20:18   ` [PATCH v4 08/13] pack-bitmap.c: compute disk-usage with " Taylor Blau
2025-03-18  1:41     ` Jeff King
2025-03-19  0:30       ` Taylor Blau
2025-03-14 20:18   ` [PATCH v4 09/13] pack-bitmap.c: apply pseudo-merge commits " Taylor Blau
2025-03-14 20:18   ` [PATCH v4 10/13] ewah: implement `struct ewah_or_iterator` Taylor Blau
2025-03-18  1:44     ` Jeff King
2025-03-19  0:33       ` Taylor Blau
2025-03-14 20:18   ` [PATCH v4 11/13] pack-bitmap.c: keep track of each layer's type bitmaps Taylor Blau
2025-03-18  2:01     ` Jeff King
2025-03-19  0:38       ` Taylor Blau
2025-03-18  6:43     ` Elijah Newren
2025-03-19  0:39       ` Taylor Blau
2025-03-14 20:18   ` [PATCH v4 12/13] pack-bitmap.c: use `ewah_or_iterator` for type bitmap iterators Taylor Blau
2025-03-18  2:05     ` Jeff King
2025-03-19 23:02       ` Taylor Blau
2025-03-14 20:19   ` [PATCH v4 13/13] midx: implement writing incremental MIDX bitmaps Taylor Blau
2025-03-18  2:16     ` Jeff King
2025-03-20  0:14       ` Taylor Blau
2025-03-18 17:13     ` Elijah Newren
2025-03-20  0:16       ` Taylor Blau
2025-03-18  2:21   ` [PATCH v4 00/13] midx: incremental multi-pack indexes, part two Jeff King
2025-03-20  0:18     ` Taylor Blau [this message]
2025-03-20 17:56 ` [PATCH v5 00/14] " Taylor Blau
2025-03-20 17:56   ` [PATCH v5 01/14] Documentation: remove a "future work" item from the MIDX docs Taylor Blau
2025-03-20 17:56   ` [PATCH v5 02/14] Documentation: describe incremental MIDX bitmaps Taylor Blau
2025-03-20 17:56   ` [PATCH v5 03/14] pack-revindex: prepare for " Taylor Blau
2025-03-20 17:56   ` [PATCH v5 04/14] pack-bitmap.c: open and store incremental bitmap layers Taylor Blau
2025-03-20 17:56   ` [PATCH v5 05/14] pack-bitmap.c: teach `bitmap_for_commit()` about incremental MIDXs Taylor Blau
2025-03-20 17:56   ` [PATCH v5 06/14] pack-bitmap.c: teach `show_objects_for_type()` " Taylor Blau
2025-03-20 17:56   ` [PATCH v5 07/14] pack-bitmap.c: support bitmap pack-reuse with " Taylor Blau
2025-03-20 17:56   ` [PATCH v5 08/14] pack-bitmap.c: teach `rev-list --test-bitmap` about " Taylor Blau
2025-03-20 17:56     ` Taylor Blau
2025-03-20 17:58       ` Taylor Blau
2025-03-20 17:56   ` [PATCH v5 09/14] pack-bitmap.c: compute disk-usage with " Taylor Blau
2025-03-20 17:56   ` [PATCH v5 10/14] pack-bitmap.c: apply pseudo-merge commits " Taylor Blau
2025-03-20 17:56   ` [PATCH v5 11/14] ewah: implement `struct ewah_or_iterator` Taylor Blau
2025-03-20 17:57   ` [PATCH v5 12/14] pack-bitmap.c: keep track of each layer's type bitmaps Taylor Blau
2025-03-20 17:57   ` [PATCH v5 13/14] pack-bitmap.c: use `ewah_or_iterator` for type bitmap iterators Taylor Blau
2025-03-20 17:57   ` [PATCH v5 14/14] midx: implement writing incremental MIDX bitmaps Taylor Blau
2025-03-20 20:00   ` [PATCH v5 00/14] midx: incremental multi-pack indexes, part two Elijah Newren

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=Z9te7BVGeU6VCo8Y@nand.local \
    --to=me@ttaylorr.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=newren@gmail.com \
    --cc=peff@peff.net \
    --cc=ps@pks.im \
    /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 NNTP newsgroup(s).