Skip to content

commit-reach: replace queue_has_nonstale with a counter #2124

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
spkrka wants to merge 3 commits into
base: master
Choose a base branch
from
+43 −17
Open

commit-reach: replace queue_has_nonstale with a counter #2124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1d37515
commit-reach: deduplicate queue entries in paint_down_to_common
spkrka May 24, 2026
4742f5e
commit-reach: optimize queue scan in paint_down_to_common
spkrka May 24, 2026
711a0e2
commit-reach: optimize queue scan in ahead_behind
spkrka May 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 42 additions & 16 deletions commit-reach.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@
#define PARENT2 (1u<<17)
Copy link
Copy Markdown

@gitgitgadget gitgitgadget Bot May 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

"Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> diff --git a/commit-reach.c b/commit-reach.c
> index d3a9b3ed6f..c16d4b061c 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -17,8 +17,9 @@
>  #define PARENT2		(1u<<17)
>  #define STALE		(1u<<18)
>  #define RESULT		(1u<<19)
> +#define ENQUEUED	(1u<<20)
>  
> -static const unsigned all_flags = (PARENT1 | PARENT2 | STALE | RESULT);
> +static const unsigned all_flags = (PARENT1 | PARENT2 | STALE | RESULT | ENQUEUED);
> ...
> diff --git a/object.h b/object.h
> index d814647ebe..05cbf728e9 100644
> --- a/object.h
> +++ b/object.h
> @@ -74,7 +74,7 @@ void object_array_init(struct object_array *array);
>   * bundle.c:                                        16
>   * http-push.c:                          11-----14
>   * commit-graph.c:                                15
> - * commit-reach.c:                                  16-----19
> + * commit-reach.c:                                  16-------20
>   * builtin/last-modified.c:                         1617
>   * sha1-name.c:                                              20
>   * list-objects-filter.c:                                      21

Not directly the fault of this series, but we'd need to audit and
update this table of bit assignment to match more recent reality.

For example, there no longer exists sha1-name.c but the table claims
that bit 20 is in use for its own purpose, and it being stale makes
it harder to audit and ensure that this new use would not crash with
these existing uses (note. there are other uses of bit 20 in other
subsystems).

FWIW, object-name.c, which was formerly known as sha1-name.c, uses
the bit 20 as ONELINE_SEEN bit, which is used to turn textual object
names like :/string (i.e., commit with that string in its message)
into raw object name, and bit 20 is cleared from all the objects
involved in the search before the helper function returns.
Presumably, once commit-reach.c starts queueing commits and reuses
this bit for its own purpose, we will never try to parse a textual
commit object name to clobber what we thought is ENQUEUED bit,
breaking the code introduced here, so we are probably safe against
its use.

I didn't check all other uses of bit 20, though.

Copy link
Copy Markdown

@gitgitgadget gitgitgadget Bot May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Derrick Stolee wrote on the Git mailing list (how to reply to this email):

On 5/24/26 7:40 PM, Junio C Hamano wrote:
> "Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> >> diff --git a/commit-reach.c b/commit-reach.c
>> index d3a9b3ed6f..c16d4b061c 100644
>> --- a/commit-reach.c
>> +++ b/commit-reach.c
>> @@ -17,8 +17,9 @@
>>   #define PARENT2		(1u<<17)
>>   #define STALE		(1u<<18)
>>   #define RESULT		(1u<<19)
>> +#define ENQUEUED	(1u<<20)
>>   >> -static const unsigned all_flags = (PARENT1 | PARENT2 | STALE | RESULT);
>> +static const unsigned all_flags = (PARENT1 | PARENT2 | STALE | RESULT | ENQUEUED);
>> ...
>> diff --git a/object.h b/object.h
>> index d814647ebe..05cbf728e9 100644
>> --- a/object.h
>> +++ b/object.h
>> @@ -74,7 +74,7 @@ void object_array_init(struct object_array *array);
>>    * bundle.c:                                        16
>>    * http-push.c:                          11-----14
>>    * commit-graph.c:                                15
>> - * commit-reach.c:                                  16-----19
>> + * commit-reach.c:                                  16-------20
>>    * builtin/last-modified.c:                         1617
>>    * sha1-name.c:                                              20
>>    * list-objects-filter.c:                                      21
> > Not directly the fault of this series, but we'd need to audit and
> update this table of bit assignment to match more recent reality.
> > For example, there no longer exists sha1-name.c but the table claims
> that bit 20 is in use for its own purpose, and it being stale makes
> it harder to audit and ensure that this new use would not crash with
> these existing uses (note. there are other uses of bit 20 in other
> subsystems).

It would be worth adding an update patch before this patch, that
only makes these adjustments

> FWIW, object-name.c, which was formerly known as sha1-name.c, uses
> the bit 20 as ONELINE_SEEN bit, which is used to turn textual object
> names like :/string (i.e., commit with that string in its message)
> into raw object name, and bit 20 is cleared from all the objects
> involved in the search before the helper function returns.

This appears to me like the only interaction that _could_ have
overlap with paint_down_to_common().

> Presumably, once commit-reach.c starts queueing commits and reuses
> this bit for its own purpose, we will never try to parse a textual
> commit object name to clobber what we thought is ENQUEUED bit,
> breaking the code introduced here, so we are probably safe against
> its use.
> > I didn't check all other uses of bit 20, though.

FLAG_LINK in builtin/index-pack.c and FLAG_OPEN in
builtin/unpack-objects.c both seem to be completely independent from
this use in commit-reach.c.

Thanks,
-Stolee

#define STALE (1u<<18)
#define RESULT (1u<<19)
#define ENQUEUED (1u<<20)

static const unsigned all_flags = (PARENT1 | PARENT2 | STALE | RESULT);
static const unsigned all_flags = (PARENT1 | PARENT2 | STALE | RESULT | ENQUEUED);

static int compare_commits_by_gen(const void *_a, const void *_b)
{
Expand All @@ -39,14 +40,25 @@ static int compare_commits_by_gen(const void *_a, const void *_b)
return 0;
Copy link
Copy Markdown

@gitgitgadget gitgitgadget Bot May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Derrick Stolee wrote on the Git mailing list (how to reply to this email):

On 5/24/26 1:42 PM, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <krka@spotify.com>
> > paint_down_to_common() terminates when every commit remaining in its
> priority queue is STALE. This was checked by queue_has_nonstale(),
> which performed an O(n) linear scan of the entire queue on every
> iteration, resulting in O(n*m) total overhead where n is the queue
> size and m is the number of commits processed.
> > Replace this with an O(1) nonstale_count that tracks the number of
> non-stale commits currently in the queue. The counter is incremented
> by maybe_enqueue() and decremented on dequeue and by mark_stale()
> when a commit transitions to STALE while still in the queue. Since
> each commit appears at most once (guaranteed by the ENQUEUED flag
> from the previous commit), the counter is exact.

This idea has a lot of merit, but I'm a bit concerned about the
organization of data. My ideas of how to improve things may also
impact patch 1's use of ENQUEUED.

> -static void maybe_enqueue(struct prio_queue *queue, struct commit *c)
> +static void maybe_enqueue(struct prio_queue *queue, struct commit *c,
> +			  int *nonstale_count)
>   {
>   	if (c->object.flags & ENQUEUED)
>   		return;
>   	c->object.flags |= ENQUEUED;
>   	prio_queue_put(queue, c);
> +	if (!(c->object.flags & STALE))
> +		(*nonstale_count)++;
> +}
> +
> +static void mark_stale(struct commit *c, unsigned queued_flag,
> +		       int *nonstale_count)
> +{
> +	if (!(c->object.flags & STALE)) {
> +		if (c->object.flags & queued_flag)
> +			(*nonstale_count)--;
> +		c->object.flags |= STALE;
> +	}
>   }

These two methods have some concerns on my end:

1. We need to store the nonstale count somewhere other than the
   priority queue, even though it's necessarily representing a
   subset of the commits within the queue.

2. mark_stale() needs a queued_flag. (I need to check to see if
   this is indeed changing in multiple callers or should always
   be ENQUEUED).

>   static int queue_has_nonstale(struct prio_queue *queue)
> @@ -68,6 +81,7 @@ static int paint_down_to_common(struct repository *r,
>   {
>   	struct prio_queue queue = { compare_commits_by_gen_then_commit_date };
>   	int i;
> +	int nonstale_count = 0;

My preference would be to create a new struct that contains a
prio_queue as a member _and_ a nonstale_count. It could initialize
with compare_commits_by_gen_then_commit_date by default.

The important thing is that consumers of such a "stale-tracking"
queue would not be setting the STALE or ENQUEUED bits themselves,
but instead the queue would be responsible for that.

This could allow us to simplify callers by always assuming we can
"add" an element to the queue and the queue will use its ENQUEUED
bit to prevent duplicates from reaching its internal prio_queue.

Such a data structure could be private to commit-reach.c for now,
since all the methods that would use it seem to be colocated there.

This is a big ask, but I'm interested to see if such an approach
would simplify things here.

Here's a potential breakdown of how to build such a thing in
"small" patches:

1. Create the data structure and update paint_down_to_common and
   ahead_behind to use that structure, but still use the existing
   prio_queue methods on its internal member.

2. Add the ENQUEUED bit and methods on the new struct that add
   that bit as it adds commits to the inner prio_queue. It would
   also ignore commits that already have that bit. (Should it
   also remove the bit as commits are removed from the queue?)

3. Now add the nonstale_count (or stale count?) to the struct and
   have it control the STALE bit modifications, with increasing
   the stale count when ENQUEUED is live, and decreasing the stale
   count as such a STALE object is dequeued.

I like the idea of this being encapsulated within the struct and
its helper methods. But the proof will be in the implementation.

Thanks,
-Stolee

}

static int queue_has_nonstale(struct prio_queue *queue)
static void maybe_enqueue(struct prio_queue *queue, struct commit *c,
int *nonstale_count)
{
for (size_t i = 0; i < queue->nr; i++) {
struct commit *commit = queue->array[i].data;
if (!(commit->object.flags & STALE))
return 1;
if (c->object.flags & ENQUEUED)
return;
c->object.flags |= ENQUEUED;
prio_queue_put(queue, c);
if (!(c->object.flags & STALE))
(*nonstale_count)++;
}

static void mark_stale(struct commit *c, unsigned queued_flag,
int *nonstale_count)
{
if (!(c->object.flags & STALE)) {
if (c->object.flags & queued_flag)
(*nonstale_count)--;
c->object.flags |= STALE;
}
return 0;
}

/* all input commits in one and twos[] must have been parsed! */
Expand All @@ -59,6 +71,7 @@ static int paint_down_to_common(struct repository *r,
{
struct prio_queue queue = { compare_commits_by_gen_then_commit_date };
int i;
int nonstale_count = 0;
timestamp_t last_gen = GENERATION_NUMBER_INFINITY;
struct commit_list **tail = result;

Expand All @@ -70,19 +83,23 @@ static int paint_down_to_common(struct repository *r,
commit_list_append(one, result);
return 0;
}
prio_queue_put(&queue, one);
maybe_enqueue(&queue, one, &nonstale_count);

for (i = 0; i < n; i++) {
twos[i]->object.flags |= PARENT2;
prio_queue_put(&queue, twos[i]);
maybe_enqueue(&queue, twos[i], &nonstale_count);
}

while (queue_has_nonstale(&queue)) {
while (nonstale_count > 0) {
struct commit *commit = prio_queue_get(&queue);
struct commit_list *parents;
int flags;
timestamp_t generation = commit_graph_generation(commit);

commit->object.flags &= ~ENQUEUED;
if (!(commit->object.flags & STALE))
nonstale_count--;

if (min_generation && generation > last_gen)
BUG("bad generation skip %"PRItime" > %"PRItime" at %s",
generation, last_gen,
Expand Down Expand Up @@ -123,8 +140,10 @@ static int paint_down_to_common(struct repository *r,
return error(_("could not parse commit %s"),
oid_to_hex(&p->object.oid));
}
if (flags & STALE)
mark_stale(p, ENQUEUED, &nonstale_count);
p->object.flags |= flags;
prio_queue_put(&queue, p);
maybe_enqueue(&queue, p, &nonstale_count);
}
}

Expand Down Expand Up @@ -1022,12 +1041,15 @@ struct commit_list *get_reachable_subset(struct commit **from, size_t nr_from,
define_commit_slab(bit_arrays, struct bitmap *);
static struct bit_arrays bit_arrays;

static void insert_no_dup(struct prio_queue *queue, struct commit *c)
static void insert_no_dup(struct prio_queue *queue, struct commit *c,
int *nonstale_count)
{
if (c->object.flags & PARENT2)
return;
prio_queue_put(queue, c);
c->object.flags |= PARENT2;
if (!(c->object.flags & STALE))
(*nonstale_count)++;
}

static struct bitmap *get_bit_array(struct commit *c, int width)
Expand All @@ -1053,6 +1075,7 @@ void ahead_behind(struct repository *r,
{
struct prio_queue queue = { .compare = compare_commits_by_gen_then_commit_date };
size_t width = DIV_ROUND_UP(commits_nr, BITS_IN_EWORD);
int nonstale_count = 0;

if (!commits_nr || !counts_nr)
return;
Expand All @@ -1071,14 +1094,17 @@ void ahead_behind(struct repository *r,
struct bitmap *bitmap = get_bit_array(c, width);

bitmap_set(bitmap, i);
insert_no_dup(&queue, c);
insert_no_dup(&queue, c, &nonstale_count);
}

while (queue_has_nonstale(&queue)) {
while (nonstale_count > 0) {
struct commit *c = prio_queue_get(&queue);
struct commit_list *p;
struct bitmap *bitmap_c = get_bit_array(c, width);

if (!(c->object.flags & STALE))
nonstale_count--;

for (size_t i = 0; i < counts_nr; i++) {
int reach_from_tip = !!bitmap_get(bitmap_c, counts[i].tip_index);
int reach_from_base = !!bitmap_get(bitmap_c, counts[i].base_index);
Expand Down Expand Up @@ -1107,9 +1133,9 @@ void ahead_behind(struct repository *r,
* queue is STALE.
*/
if (bitmap_popcount(bitmap_p) == commits_nr)
p->item->object.flags |= STALE;
mark_stale(p->item, PARENT2, &nonstale_count);

insert_no_dup(&queue, p->item);
insert_no_dup(&queue, p->item, &nonstale_count);
}

free_bit_array(c);
Expand Down
2 changes: 1 addition & 1 deletion object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ void object_array_init(struct object_array *array);
* bundle.c: 16
* http-push.c: 11-----14
* commit-graph.c: 15
* commit-reach.c: 16-----19
* commit-reach.c: 16-------20
* builtin/last-modified.c: 1617
* sha1-name.c: 20
* list-objects-filter.c: 21
Expand Down
Loading