Re: [PATCH] oidmap: map with OID as key

From: Jonathan Tan <jonathantanmy@google.com>
Date: Sep 28, 2017, 17:38 UTC
Message-ID: <20170928103850.449bd743d5f10f2c948b0c83@google.com>
In-Reply-To: <xmqqr2ur348z.fsf@gitster.mtv.corp.google.com>

On Thu, 28 Sep 2017 12:13:00 +0900 Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> > This is similar to using the hashmap in hashmap.c, but with an
> > easier-to-use API. In particular, custom entry comparisons no longer
> > need to be written, and lookups can be done without constructing a
> > temporary entry structure.
> 
> A naïve question is why this needs to duplicate so much code, just
> to build something similar in spirit to hashmap but unlike hashmap
> that can take caller-defined keys, limited to using oid as the keys,
> instead of just being a thin API wrapper that uses hashmap as its
> internal implementation detail.  
> 
> Is the way hashmap API is structured so hard to use it in such a
> way, or something?

Another reason that I probably should have mentioned is the opportunity to save 4 bytes. I didn't mention it in the commit message at first because I felt that the benefit was very specific and, as far as I know, wouldn't be seen on architectures with 8-byte-aligned pointers until we change the hash function. But I should have probably written something like this in the commit message:

    Using oidmap also saves 4 bytes per entry, compared to hashmap,
    because the hash does not need to be stored separately as an int.
    (However, alignment considerations might mean that we will not
    observe these savings until we change the hash function, since
    currently 20-byte hashes are being used.)

If we decide that the 4 bytes are not important, right now at least, I can change it so that this is a thin API wrapper over hashmap. We could always change it later.