Re: [PATCH v2 01/20] pack-revindex: introduce a new API

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 14, 2021, 20:47 UTC
Message-ID: <xmqq35z3xn30.fsf@gitster.c.googlers.com>
In-Reply-To: <YACZLHm4NtrM3POZ@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 25 quoted lines

> On Thu, Jan 14, 2021 at 12:06:20PM -0500, Taylor Blau wrote:
>
>> > > + * This function runs in time O(log N) with the number of objects in the pack.
>> >
>> > Is it a good idea to commit to such performance characteristics as a
>> > promise to callers like this (the comment applies to all three
>> > functions)?
>> >
>> > It depends on how a developer is helped by this comment when
>> > deciding whether to use this function, or find other ways, to
>> > implement what s/he wants to do.
>> 
>> I don't mind it. If they all had the same performance characteristics, I
>> wouldn't be for it, but since they don't, I think that it's good to
>> know. Peff suggested this back in [1].
>
> Yeah, I asked for this. As somebody who has frequently worked on the
> code which accesses the revindex (mostly bitmap stuff), I found it
> useful to understand how expensive the operations were.  However, I also
> know what their runtimes are at this point, and it is not like somebody
> interested cannot look at the implementation. So it may not be that
> important.
>
> So I do still think it is useful, but if somebody feels strongly against
> it, I don't mind it being removed.

That won't be me. It's not like you'd use pack_pos_to_index() combined with pack_pos_to_offset() instead of offset_to_pack_pos() because the latter is more expensive than using the other two functions; the comment does not help those who want to know relative performance of these functions for such a purpose.

I was just curious who the comments were meant to help, that's all.

Thanks.