git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jonathan Tan <jonathantanmy@google.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, git@jeffhostetler.com
Subject: Re: [PATCH] oidmap: map with OID as key
Date: Thu, 28 Sep 2017 10:38:50 -0700	[thread overview]
Message-ID: <20170928103850.449bd743d5f10f2c948b0c83@google.com> (raw)
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:

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

  reply	other threads:[~2017-09-28 17:38 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-09-27 22:19 [PATCH] oidmap: map with OID as key Jonathan Tan
2017-09-28  0:41 ` Brandon Williams
2017-09-28 17:46   ` Jonathan Tan
2017-09-28 20:05     ` Jeff King
2017-09-29 19:04       ` Jonathan Tan
2017-09-29 19:26         ` Jeff King
2017-09-29 21:43       ` Johannes Schindelin
2017-09-29 23:24         ` Jeff King
2017-09-28  3:13 ` Junio C Hamano
2017-09-28 17:38   ` Jonathan Tan [this message]
2017-09-29 22:54 ` [PATCH v2] " Jonathan Tan
2017-10-02 23:48   ` Brandon Williams
2017-10-03  6:31     ` Jeff King
2017-10-04  0:29       ` Jonathan Tan
2017-10-04  7:45         ` Jeff King
2017-10-04  8:48           ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170928103850.449bd743d5f10f2c948b0c83@google.com \
    --to=jonathantanmy@google.com \
    --cc=git@jeffhostetler.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).