Re: [PATCH] oidmap: ensure map is initialized

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Brandon,

On Fri, 22 Dec 2017, Brandon Williams wrote:

> Ensure that an oidmap is initialized before attempting to add, remove,
> or retrieve an entry by simply performing the initialization step
> before accessing the underlying hashmap.
> 
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  oidmap.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/oidmap.c b/oidmap.c
> index 6db4fffcd..d9fb19ba6 100644
> --- a/oidmap.c
> +++ b/oidmap.c
> @@ -33,12 +33,19 @@ void oidmap_free(struct oidmap *map, int free_entries)
>  
>  void *oidmap_get(const struct oidmap *map, const struct object_id *key)
>  {
> +	if (!map->map.cmpfn)
> +		return NULL;
> +

This one is unnecessary: if we always _init() before we add, then
hashmap_get_from_hash() will not have anything to compare, and therefore
not call cmpfn.

You could argue that it is only a teeny-tiny runtime cost, so it'd be
better to be safe than to be sorry.

However, it is a death by a thousand cuts. My colleagues and I try to
shave off a few percent here and a few percent there, in the hope to get
some performance improvements worth writing home about. And then patches
like this one come in that simply add runtime without much in the way of
performance considerations.

And that makes me quite a bit sad.

>  	return hashmap_get_from_hash(&map->map, hash(key), key);
>  }
>  
>  void *oidmap_remove(struct oidmap *map, const struct object_id *key)
>  {
>  	struct hashmap_entry entry;
> +
> +	if (!map->map.cmpfn)
> +		oidmap_init(map, 0);
> +
>  	hashmap_entry_init(&entry, hash(key));
>  	return hashmap_remove(&map->map, &entry, key);
>  }
> @@ -46,6 +53,10 @@ void *oidmap_remove(struct oidmap *map, const struct object_id *key)
>  void *oidmap_put(struct oidmap *map, void *entry)
>  {
>  	struct oidmap_entry *to_put = entry;
> +
> +	if (!map->map.cmpfn)
> +		oidmap_init(map, 0);
> +
>  	hashmap_entry_init(&to_put->internal_entry, hash(&to_put->oid));
>  	return hashmap_put(&map->map, to_put);
>  }

"But it does not add a lot, it's only a couple of microseconds"

Sure. But we could (and do) simply initialize the hashmaps once, and avoid
having to spend unnecessary cycles for every single access.

I *much* prefer my original patch that essentially does not change *any*
code path. Everything stays the same, except that there is now a strong
hint explaining why you need to call oidmap_init() manually instead of
using the OIDMAP_INIT macro and then wonder why your code crashes.

Ciao,
Dscho