Re: [PATCH 15/17] khash: rename oid helper functions

["René Scharfe" <l.s.r@web.de>]

Am 20.06.19 um 20:27 schrieb Jeff King:
> On Thu, Jun 20, 2019 at 10:44:17AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>>> For use in object_id hash tables, we have oid_hash() and oid_equal().
>>> But these are confusingly similar to the existing oideq() and the
>>> oidhash() we plan to add to replace sha1hash().
>>>
>>> The big difference from those functions is that rather than accepting a
>>> const pointer to the "struct object_id", we take the arguments by value
>>> (which is a khash internal convention). So let's make that obvious by
>>> calling them oidhash_by_value() and oideq_by_value().
>>>
>>> Those names are fairly horrendous to type, but we rarely need to do so;
>>> they are passed to the khash implementation macro and then only used
>>> internally. Callers get to use the nice kh_put_oid_map(), etc.
>>
>> The pass-by-value interface feels a bit unforunate but hopefully
>> "static inline" would help us avoid actually copying the struct left
>> and right as we make calls to them X-<.
>
> Yeah, exactly. I think it should end up quite fast, though if anybody
> (René?) wants to try tweaking khash and timing it, be my guest.
>
> I do think if it took the more usual pass-by-const-pointer we'd probably
> still end up with the exact same code from an optimizing compiler, since
> all of the references and dereferences would cancel out once it was
> inlined.

I suspect it depends on the compiler and the exact details.  Here's a
simple experiment: https://godbolt.org/z/kuv4NE.  It has a comparison
function for call by value and one for call by reference as well as a
wrapper for each with the opposite interface.

An enlightened compiler would emit the same code for functions with the
same interface, but none of the current ones do in all cases.  Clang
and MSVC do emit the same code for the two call by value functions, so
there's hope.  And moving to call by reference might make matters worse.
GCC adds some 128-bit moves to both wrappers for some reason.

Perhaps it doesn't matter much anyway e.g. due to caching, especially
for the branch-free variants.  A definite answer would require
measurements.  Your cleanup would make the necessary surgery on khash.h
a bit easier by reducing the number of functions and definitions.

René