Re: [PATCH 2/2] receive-pack: use advertised reference tips to inform connectivity check

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Oct 31 2022, Patrick Steinhardt wrote:

> [[PGP Signed Part:Undecided]]
> On Fri, Oct 28, 2022 at 05:01:58PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> On Fri, Oct 28 2022, Patrick Steinhardt wrote:
> [sinp]
>> > Unfortunately, this change comes with a performance hit when refs are
>> > not hidden. Executed in the same repository:
>> >
>> >     Benchmark 1: main
>> >       Time (mean ± σ):     45.780 s ±  0.507 s    [User: 46.908 s, System: 4.838 s]
>> >       Range (min … max):   45.453 s … 46.364 s    3 runs
>> >
>> >     Benchmark 2: pks-connectivity-check-hide-refs
>> >       Time (mean ± σ):     49.886 s ±  0.282 s    [User: 51.168 s, System: 5.015 s]
>> >       Range (min … max):   49.589 s … 50.149 s    3 runs
>> >
>> >     Summary
>> >       'main' ran
>> >         1.09 ± 0.01 times faster than 'pks-connectivity-check-hide-refs'
>> >
>> > This is probably caused by the overhead of reachable tips being passed
>> > in via git-rev-list(1)'s standard input, which seems to be slower than
>> > reading the references from disk.
>> >
>> > It is debatable what to do about this. If this were only about improving
>> > performance then it would be trivial to make the new logic depend on
>> > whether or not `transfer.hideRefs` has been configured in the repo. But
>> > as explained this is also about correctness, even though this can be
>> > considered an edge case. Furthermore, this slowdown is really only
>> > noticeable in outliers like the above repository with an unreasonable
>> > amount of refs. The same benchmark in linux-stable.git with about
>> > 4500 references shows no measurable difference:
>> 
>> Do we have a test that would start failing if we changed the behavior?
>> Perhaps such a test is peeking too much behind the curtain, but if it's
>> easy come up with one I think it would be most welcome to have it
>> alongside this.  to have exposes
>
> We have tests that verify that we indeed detect missing objects in
> t5504. But what we're lacking is tests that verify that we stop walking
> at the boundary of preexisting objects, and I honestly wouldn't quite
> know how to do that as there is no functional difference, but really
> only a performance issue if we overwalked.
>
>> > -static void write_head_info(void)
>> > +static void write_head_info(struct oidset *announced_objects)
>> >  {
>> > -	static struct oidset seen = OIDSET_INIT;
>> > -
>> > -	for_each_ref(show_ref_cb, &seen);
>> > -	for_each_alternate_ref(show_one_alternate_ref, &seen);
>> > -	oidset_clear(&seen);
>> > +	for_each_ref(show_ref_cb, announced_objects);
>> > +	for_each_alternate_ref(show_one_alternate_ref, announced_objects);
>> >  	if (!sent_capabilities)
>> >  		show_ref("capabilities^{}", null_oid());
>> 
>> Nit: The variable rename stands out slightly,
>> i.e. s/&seen/announced_objects/ not s/&seen/seen/, especially as:
>> 
>> >  static void execute_commands(struct command *commands,
>> >  			     const char *unpacker_error,
>> >  			     struct shallow_info *si,
>> > -			     const struct string_list *push_options)
>> > +			     const struct string_list *push_options,
>> > +			     struct oidset *announced_oids)
>> 
>> Here we have the same variable, but now it's *_oids, not *objects.
>
> Hm. I think that `announced_oids` is easier to understand compared to
> `seen`, so I'd prefer to keep the rename. But I'll definitely make this
> consistent so we use `announced_oids` in both places.

Sounds good, we'll need to look at the diff lines in either case (as
we're converting it to a pointer), so changing the name while at it is
fine...

> [snip]
>> > +static const struct object_id *iterate_announced_oids(void *cb_data)
>> > +{
>> > +	struct oidset_iter *iter = cb_data;
>> > +	return oidset_iter_next(iter);
>> > +}
>> > +
>> 
>> Is just used as (from 1/2):
>> 
>> > +	if (opt->reachable_oids_fn) {
>> > +		const struct object_id *reachable_oid;
>> > +		while ((reachable_oid = opt->reachable_oids_fn(opt->reachable_oids_data)) != NULL)
>> > +			if (fprintf(rev_list_in, "^%s\n", oid_to_hex(reachable_oid)) < 0)
>> > +				break;
>> > +	}
>> 
>> After doing above:
>> 
>> > +	if (oidset_size(announced_oids) != 0) {
>> > +		oidset_iter_init(announced_oids, &announced_oids_iter);
>> > +		opt.reachable_oids_fn = iterate_announced_oids;
>> > +		opt.reachable_oids_data = &announced_oids_iter;
>> > +	}
>> 
>> But I don't see the reason for the indirection, but maybe I'm missing
>> something obvious.
>> 
>> Why not just pass the oidset itself and have connected.c iterate through
>> it, rather than going thorugh this callback / data indirection?
>
> This is done to stay consistent with the way new tips are passed in via
> the `oid_iterate_fn`. I'm happy to change callers to just directly pass
> a `struct oidset *` though.

*nod*, makes sense, no need to change it. Just wondering...