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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 4559 bytes --]

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.

[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.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]