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

Re: [PATCH 2/5] pack-bitmap: tag bitmapped packs with their corresponding MIDX

From
Jeff King <peff@peff.net>
Date
Sep 5, 2024, 09:00 UTC
Message-ID
<20240905090024.GA2556395@coredump.intra.peff.net>
In-Reply-To
<ZtDEy/teamtXMAbn@nand.local>
On Thu, Aug 29, 2024 at 02:58:19PM -0400, Taylor Blau wrote:
Show 27 quoted lines
> On Tue, Aug 27, 2024 at 05:14:14PM -0700, Junio C Hamano wrote:
> > > diff --git a/midx.c b/midx.c
> > > index ca98bfd7c6..67e0d64004 100644
> > > --- a/midx.c
> > > +++ b/midx.c
> > > @@ -496,6 +496,7 @@ int nth_bitmapped_pack(struct repository *r, struct multi_pack_index *m,
> > >  				 MIDX_CHUNK_BITMAPPED_PACKS_WIDTH * local_pack_int_id +
> > >  				 sizeof(uint32_t));
> > >  	bp->pack_int_id = pack_int_id;
> > > +	bp->from_midx = m;
> >
> > Do multi_pack_index objects live as long as bitmapped_pack objects
> > that point at them live?  If m later goes away without letting the
> > bitmapped_pack know about it, the borrowed pointer in from_midx
> > would become dangling, which is not what we want to see.
> >
> > "None of these objects are released or relocated while we are
> > running pack-objects, so once the .from_midx member is assigned
> > here, it will always be pointing at a valid multi_pack_index object"
> > is a satisfactory answer, I guess.
> 
> Good question, and good answer ;-).
> 
> This is only relevant in a read-only path where we're generating a new
> pack from existing packs and not altering those pack or rewriting /
> deleting the MIDX attached to them. So I think we're OK here and don't
> have any lifetime/scope issues.

Do we ever close/reopen the midx? For example, if a simultaneous process wrote a new one and we triggered reprepare_packed_git()?

I think the answer is "no"; we might read a new midx, but we never ditch the old one (just like we do for packed_git structs). Which I suspect is needed even before this patch, since various other parts of the bitmap code (and probably others) rely on the struct staying in place across as we read many objects.

-Peff
Previous: Taylor BlauNext: Taylor Blau
Message 6 of 17 in “pack-objects: brown-paper-bag fixes for multi-pack reuse”
  1. 0/5 pack-objects: brown-paper-bag fixes for multi-pack reuseTaylor Blau, Aug 27, 2024
  2. 1/5 t/t5332-multi-pack-reuse.sh: verify pack generation with --strictTaylor Blau, Aug 27, 2024
  3. 2/5 pack-bitmap: tag bitmapped packs with their corresponding MIDXTaylor Blau, Aug 27, 2024
  4. Junio C HamanoAug 28, 2024
  5. Taylor BlauAug 29, 2024
  6. Jeff KingSep 5, 2024
  7. Taylor BlauSep 17, 2024
  8. 3/5 builtin/pack-objects.c: translate bit positions during pack-reuseTaylor Blau, Aug 27, 2024
  9. Junio C HamanoSep 4, 2024
  10. 4/5 pack-bitmap.c: avoid repeated `pack_pos_to_offset()` during reuseTaylor Blau, Aug 27, 2024
  11. Junio C HamanoSep 4, 2024
  12. Taylor BlauSep 4, 2024
  13. 5/5 builtin/pack-objects.c: do not open-code `MAX_PACK_OBJECT_HEADER`Taylor Blau, Aug 27, 2024
  14. Junio C HamanoSep 4, 2024
  15. Taylor BlauSep 4, 2024
  16. Jeff KingSep 5, 2024
  17. Junio C HamanoSep 5, 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.