git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 1/8] pack-bitmap: initialize `bitmap_writer_init()` with packing_data

From
Taylor Blau <me@ttaylorr.com>
Date
Aug 29, 2024, 19:00 UTC
Message-ID
<ZtDFRYQRLQoe+CHS@nand.local>
In-Reply-To
<20240817103155.GA551779@coredump.intra.peff.net>
On Sat, Aug 17, 2024 at 06:31:55AM -0400, Jeff King wrote:
Show 23 quoted lines
> On Thu, Aug 15, 2024 at 01:31:00PM -0400, Taylor Blau wrote:
>
> > In order to determine its object order, the pack-bitmap machinery keeps
> > a 'struct packing_data' corresponding to the pack or pseudo-pack (when
> > writing a MIDX bitmap) being written.
> >
> > The to_pack field is provided to the bitmap machinery by callers of
> > bitmap_writer_build() and assigned to the bitmap_writer struct at that
> > point.
> >
> > But a subsequent commit will want to have access to that data earlier on
> > during commit selection. Prepare for that by adding a 'to_pack' argument
> > to 'bitmap_writer_init()', and initializing the field during that
> > function.
> >
> > Subsequent commits will clean up other functions which take
> > now-redundant arguments (like nr_objects, which is equivalent to
> > pdata->objects_nr, or pdata itself).
>
> This (and the next few follow-on commits) seem like a good change to me.
> It simplifies many of the function calls, and I think it expresses the
> domain logic in the API: there is a single set of objects being mapped
> to bits, and many parts of the process will rely on it.

Thanks. Yeah, it was a little surprising to me that it wasn't already this way, especially having worked in this area for so long. I suspect it grew this way organically over time (though haven't actually gone spelunking through the history to confirm).

Show 5 quoted lines
> Even the midx code, which is not generating a pack, uses a "fake"
> packing_data as the way to express that (because inherently the bit
> ordering is all coming from the pack-index nature). If we likewise ever
> wrote code to generate bitmaps from an existing pack, it would probably
> use packing_data, too. :)

I agree for the most part, though there is a lot of weight in packing_data that would be nice to not have to carry around. I know within GitHub's infrastructure we sometimes OOM kill invocations of "git multi-pack-index write --bitmap" because of the memory overhead (a lot of which is dominated by the actual traversal and bitmap generation, but a lot that comes from just the per-object overhead).

I've thought about alternative structures that might be a little more memory efficient, but it's never gotten to the top of my list.

Thanks, Taylor

Previous: Jeff KingNext: Jeff King
Message 4 of 20 in “pseudo-merge: avoid empty and non-closed pseudo-merge commits”
  1. 0/8 pseudo-merge: avoid empty and non-closed pseudo-merge commitsTaylor Blau, Aug 15, 2024
  2. 1/8 pack-bitmap: initialize `bitmap_writer_init()` with packing_dataTaylor Blau, Aug 15, 2024
  3. Jeff KingAug 17, 2024
  4. Taylor BlauAug 29, 2024
  5. Jeff KingAug 29, 2024
  6. 2/8 pack-bitmap: drop redundant args from `bitmap_writer_build_type_index()`Taylor Blau, Aug 15, 2024
  7. 3/8 pack-bitmap: drop redundant args from `bitmap_writer_build()`Taylor Blau, Aug 15, 2024
  8. 4/8 pack-bitmap: drop redundant args from `bitmap_writer_finish()`Taylor Blau, Aug 15, 2024
  9. 5/8 pack-bitmap-write.c: select pseudo-merges even for small bitmapsTaylor Blau, Aug 15, 2024
  10. Jeff KingAug 17, 2024
  11. Junio C HamanoAug 17, 2024
  12. Taylor BlauAug 29, 2024
  13. 6/8 t/t5333-pseudo-merge-bitmaps.sh: demonstrate empty pseudo-merge groupsTaylor Blau, Aug 15, 2024
  14. 7/8 pseudo-merge.c: do not generate empty pseudo-merge commitsTaylor Blau, Aug 15, 2024
  15. Jeff KingAug 17, 2024
  16. Taylor BlauAug 29, 2024
  17. 8/8 pseudo-merge.c: ensure pseudo-merge groups are closedTaylor Blau, Aug 15, 2024
  18. Jeff KingAug 17, 2024
  19. Jeff KingAug 17, 2024
  20. Taylor BlauAug 29, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.