Re: [PATCH] hashmap: add API to disable item counting when threaded

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 6, 2017, 01:24 UTC
Message-ID: <xmqqo9qoaab4.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170830185922.10107-2-git@jeffhostetler.com>

Jeff Hostetler <git@jeffhostetler.com> writes:

Show 25 quoted lines

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> This is to address concerns raised by ThreadSanitizer on the
> mailing list about threaded unprotected R/W access to map.size with my previous
> "disallow rehash" change (0607e10009ee4e37cb49b4cec8d28a9dda1656a4).  See:
> https://public-inbox.org/git/adb37b70139fd1e2bac18bfd22c8b96683ae18eb.1502780344.git.martin.agren@gmail.com/
>
> Add API to hashmap to disable item counting and to disable automatic rehashing.  
> Also include APIs to re-enable item counting and automatica rehashing.
>
> When item counting is disabled, the map.size field is invalid.  So to
> prevent accidents, the field has been renamed and an accessor function
> hashmap_get_size() has been added.  All direct references to this
> field have been been updated.  And the name of the field changed
> to map.private_size to communicate thie.
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  attr.c                  | 14 ++++---
>  builtin/describe.c      |  2 +-
>  hashmap.c               | 31 +++++++++++-----
>  hashmap.h               | 98 ++++++++++++++++++++++++++++++++++++++-----------
>  name-hash.c             |  6 ++-
>  t/helper/test-hashmap.c |  2 +-
>  6 files changed, 113 insertions(+), 40 deletions(-)

I feel somewhat stupid to say this, especially after seeing many people applaud this patch, but I do not seem to be able to even build Git with this patch. I am getting:

    common-main.o: In function `hashmap_get_size':
    /home/gitster/w/git.git/hashmap.h:260: multiple definition of `hashmap_get_size'
    fast-import.o:/home/gitster/w/git.git/hashmap.h:260: first defined here
    libgit.a(argv-array.o): In function `hashmap_get_size':
    /home/gitster/w/git.git/hashmap.h:260: multiple definition of `hashmap_get_size'
    fast-import.o:/home/gitster/w/git.git/hashmap.h:260: first defined here
    libgit.a(attr.o): In function `hashmap_get_size':
    ...

and wonder if others are building with different options or something..

Show 11 quoted lines

> diff --git a/hashmap.h b/hashmap.h
> index 7a8fa7f..7b8e6f4 100644
> --- a/hashmap.h
> +++ b/hashmap.h
> @@ -253,6 +253,19 @@ static inline void hashmap_entry_init(void *entry, unsigned int hash)
>  }
>  
>  /*
> + * Return the number of items in the map.
> + */
> +inline unsigned int hashmap_get_size(struct hashmap *map)

I think this must become static inline like everybody else in this file, at least.

I also wonder if this header is excessively inlining too many functions without measuring first, but that is a different story.