Re: [PATCH] commit-reach: simplify cleanup of remaining bitmaps in ahead_behind()
- From
Jeff King <peff@peff.net>
- Date
- Mar 19, 2026, 16:57 UTC
- Message-ID
- <20260319165747.GA3615867@coredump.intra.peff.net>
- In-Reply-To
- <c01eb1e3-d839-4cf6-ba47-5a9edd336ae3@web.de>
On Wed, Mar 18, 2026 at 05:09:37PM +0100, René Scharfe wrote:
Show 10 quoted lines
> > - while (prio_queue_peek(&queue)) {
> > - struct commit *c = prio_queue_get(&queue);
> > - free_bit_array(c);
> > - }
> > - clear_bit_arrays(&bit_arrays);
> > + deep_clear_bit_arrays(&bit_arrays, free_bitmap_pointer);
>
> The prio_queue contains just a few unvisited entries at this point (or
> perhaps even none), while deep_clear_*() will visit all commits that
> ever had a bitmap, even if their bitmap pointer is NULL now.It is potentially even worse than that. A commit-slab must over-allocate because it provides a pseudo-array over _all_ commits in the program. So if the commit with index 123 gets a bitmap, then we will allocate a pointer for the whole chunk, even if 124, 125, etc, never got one.
Looking at ahead_behind(), though, I think it's probably pretty dense. We'll be creating new commits from parent pointers and then immediately queuing them. So the index values we allocate should have high locality.
But it might be something interesting to double-check.
Show 6 quoted lines
> We could still access them in array order, which must be cheaper: > > for (size_t i = 0; i < queue.nr; i++) > free_bit_array(queue.array[i].data); > > Performance is the same for my local Git repo clone, though.
Yeah, I agree that is a reasonable simplification.
-Peff