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