oidmap.h: strongly discourage using OIDMAP_INIT directly
PATCH 1/1 oidmap.h: strongly discourage using OIDMAP_INIT directly
In Git's source code, we have this convention that quite a few data structures can be initialized using a macro *_INIT while defining an instance (instead of having to call a function to initialize the data structure). You will see that idiom quite a bit, e.g. struct strbuf buf = STRBUF_INIT;
This works for oidsets, too: struct oidset oids = OIDSET_INIT; is perfectly legal and you can use that data structure right away, without having to call oidset_init() first.
That pattern is violated by OIDMAP_INIT, though. The first call to oidmap_put() or oidmap_get() will succeed, but by mistake rather than by
design: The underlying hashmap is not initialized correctly, as the cmpfn function pointer still points to NULL, but since there are no entries to be compared, cmpfn will not be called. Things break down, though, as soon as there is even one entry.
Rather than causing confusion, frustration and needless loss of time due to pointless debugging, let's *rename* OIDMAP_INIT so that developers who have gotten used to the pattern struct xyz a = XYZ_INIT; won't do that with oidmaps.
An alternative would be to introduce a HASHMAP_INIT(cmp_fn) and use that in oidmap.h. However, there are *no* call sites in Git's source code that would benefit from that macro, i.e. this would be premature optimization (and cost a lot more time than this here trivial patch).
---
Published-As: https://github.com/dscho/git/releases/tag/discourage-OIDMAP_INIT-v1
Fetch-It-Via: git fetch https://github.com/dscho/git discourage-OIDMAP_INIT-v1
oidmap.h | 6 +++++- oidset.h | 7 ++++++- 2 files changed, 11 insertions(+), 2 deletions(-)
oidmap.h +5 -1
@@ -21,7 +21,11 @@ struct oidmap {struct hashmap map; }; -#define OIDMAP_INIT { { NULL } } +/* +* This macro initializes the data structure only incompletely, just enough +* to call oidmap_get() on an empty map. Use oidmap_init() instead. +*/ +#define OIDMAP_INIT_INCOMPLETELY { { NULL } }/* * Initializes an oidmap structure.
oidset.h +6 -1
@@ -22,7 +22,12 @@ struct oidset {struct oidmap map; }; -#define OIDSET_INIT { OIDMAP_INIT } +/* +* It is okay to initialize the map incompletely here because oidset_insert() +* will call oidset_init() (which will call oidmap_init()), and +* oidset_contains() works as intended even before oidset_init() was called. +*/ +#define OIDSET_INIT { OIDMAP_INIT_INCOMPLETELY }/** * Returns true iff `set` contains `oid`.
base-commit: 936d1b989416a95f593bf81ccae8ac62cd83f279
Signature · 1 line
2.15.1.windows.2
Quoted 40 lines · Johannes Schindelin
In Git's source code, we have this convention that quite a few data structures can be initialized using a macro
*_INITwhile defining an instance (instead of having to call a function to initialize the data structure). You will see that idiom quite a bit, e.g.struct strbuf buf = STRBUF_INIT;This works for oidsets, too:
struct oidset oids = OIDSET_INIT;is perfectly legal and you can use that data structure right away, without having to calloidset_init()first.That pattern is violated by
OIDMAP_INIT, though. The first call tooidmap_put()oroidmap_get()will succeed, but by mistake rather than by
design: The underlying hashmap is not initialized correctly, as the cmpfn function pointer still points to NULL, but since there are no entries to be compared, cmpfn will not be called. Things break down, though, as soon as there is even one entry.Rather than causing confusion, frustration and needless loss of time due to pointless debugging, let's *rename*
OIDMAP_INITso that developers who have gotten used to the patternstruct xyz a = XYZ_INIT;won't do that with oidmaps.An alternative would be to introduce a
HASHMAP_INIT(cmp_fn)and use that inoidmap.h. However, there are *no* call sites in Git's source code that would benefit from that macro, i.e. this would be premature optimization (and cost a lot more time than this here trivial patch).Signed-off-by Johannes Schindelin <[email protected]>---
Published-As: https://github.com/dscho/git/releases/tag/discourage-OIDMAP_INIT-v1
Fetch-It-Via: git fetch https://github.com/dscho/git discourage-OIDMAP_INIT-v1oidmap.h | 6 +++++- oidset.h | 7 ++++++- 2 files changed, 11 insertions(+), 2 deletions(-)oidmap.h +0
-0
oidmap.h struct oidmap { · line 28
Quoted 9 lines
oidmap.h +5
-1@@ -21,7 +21,11 @@ struct oidmap {struct hashmap map; }; -#define OIDMAP_INIT { { NULL } } +/* +* This macro initializes the data structure only incompletely, just enough +* to call oidmap_get() on an empty map. Use oidmap_init() instead. +*/ +#define OIDMAP_INIT_INCOMPLETELY { { NULL } }
This is one way of approaching the problem. Couldn't we also take the approach like we have with oidset and ensure that when oidmap_get() or _put() are called that if the oidmap isn't initialized, we simply do that then?
Quoted 25 lines
oidmap.h +0
-0/* * Initializes an oidmap structure. oidset.h +6
-1@@ -22,7 +22,12 @@ struct oidset {struct oidmap map; }; -#define OIDSET_INIT { OIDMAP_INIT } +/* +* It is okay to initialize the map incompletely here because oidset_insert() +* will call oidset_init() (which will call oidmap_init()), and +* oidset_contains() works as intended even before oidset_init() was called. +*/ +#define OIDSET_INIT { OIDMAP_INIT_INCOMPLETELY }/** * Returns true iff `set` contains `oid`. base-commit:
936d1b989416a95f593bf81ccae8ac62cd83f279Signature · 1 line
2.15.1.windows.2
Signature · 1 line
Brandon Williams
Quoted 11 lines · Brandon Williams
oidmap.h struct oidmap { · line 28
Quoted 6 lines
oidmap.h +5
-1-#define OIDMAP_INIT { { NULL } } +/* +* This macro initializes the data structure only incompletely, just enough +* to call oidmap_get() on an empty map. Use oidmap_init() instead. +*/ +#define OIDMAP_INIT_INCOMPLETELY { { NULL } }This is one way of approaching the problem. Couldn't we also take the approach like we have with oidset and ensure that when
oidmap_get()or_put()are called that if the oidmap isn't initialized, we simply do that then?
Hmph. Can you show how the alternative code would look like?
PATCH 1/1 oidmap: ensure map is initialized
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.
--- oidmap.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
oidmap.c +11 -0
@@ -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 ; +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); }
Signature · 1 line
2.15.1.620.gb9897f4670-goog
Re: [PATCH] oidmap: ensure map is initialized
Hi Brandon,
Quoted 14 lines · Brandon Williams
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 <[email protected]>--- oidmap.c | 11 +++++++++++ 1 file changed, 11 insertions(+)oidmap.c +0
-0
oidmap.c void oidmap_free(struct oidmap *map, int free_entries) · line 38
Quoted 6 lines
oidmap.c +3
-0@@ -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.
Quoted 14 lines
oidmap.c +4
-0return 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); }
oidmap.c void *oidmap_remove(struct oidmap *map, const struct object_id *key) · line 62
Quoted 10 lines
oidmap.c +4
-0@@ -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
Re: [PATCH] oidmap: ensure map is initialized
In reply to Brandon WilliamsQuoted 6 lines · Brandon Williams
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 <[email protected]>---
Looks sane. Thanks for illustrating the idea with actual code.
Essentially, you are using map->map.cmpfn as a boolean to see "have we initialized this thing? if not, we need to initialize it on demand".
By the way, I am somewhat more sympathetic than usual to Dscho's "make oidmap_get() very aware of the internal implementation detail of hashmap_get_from_hash() to micro-optimize by removing the check from _get()". Such a layering violation is disgusting, and making it deliberately shows an even worse design taste, but in this particular case, because the oidmap API is too thin a layer on top of hashmap, it is understandably a very tempting approach.
Quoted 38 lines
oidmap.c | 11 +++++++++++ 1 file changed, 11 insertions(+)oidmap.c +11
-0@@ -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 ; +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); }
Hi Junio,
Quoted 14 lines · Junio C Hamano
Quoted 11 lines · Brandon Williams
oidmap.h struct oidmap { · line 28
Quoted 6 lines
oidmap.h +5
-1-#define OIDMAP_INIT { { NULL } } +/* +* This macro initializes the data structure only incompletely, just enough +* to call oidmap_get() on an empty map. Use oidmap_init() instead. +*/ +#define OIDMAP_INIT_INCOMPLETELY { { NULL } }This is one way of approaching the problem. Couldn't we also take the approach like we have with oidset and ensure that when
oidmap_get()or_put()are called that if the oidmap isn't initialized, we simply do that then?
Quoted 1 line
Hmph. Can you show how the alternative code would look like?
No, because I refuse to perform pointless work, in particular when I am already pretty booked with work.
But you know how it would look like, right? The cmpfn() function would be exported via oidmap.h, and a HASHMAP_INIT(cmpfn) would be introduced in hashmap.h that would initialize everything zeroed out except for the cmpfn.
But then you would review it and ask if there would be any use in adding cmp_cb_data to the signature of the HASHMAP_INIT() macro, and I would have to implement that, too.
And then nobody would use it, and the macro would very likely get stale/incorrect. And then I would offer another patch reverting that change (because there is no user) and replace it with this here patch.
As I said: pointless,
Dscho
Quoted 1 line · Brandon Williams
On 12/22, Johannes Schindelin wrote:
[...]
Quoted 12 lines
Quoted 12 lines
That pattern is violated by
OIDMAP_INIT, though. The first call tooidmap_put()oroidmap_get()will succeed, but by mistake rather than by
design: The underlying hashmap is not initialized correctly, as the cmpfn function pointer still points to NULL, but since there are no entries to be compared, cmpfn will not be called. Things break down, though, as soon as there is even one entry.Rather than causing confusion, frustration and needless loss of time due to pointless debugging, let's *rename*
OIDMAP_INITso that developers who have gotten used to the patternstruct xyz a = XYZ_INIT;won't do that with oidmaps.
Quoted 6 lines
oidmap.h struct oidmap { · line 28
Quoted 6 lines
oidmap.h +5
-1-#define OIDMAP_INIT { { NULL } } +/* +* This macro initializes the data structure only incompletely, just enough +* to call oidmap_get() on an empty map. Use oidmap_init() instead. +*/ +#define OIDMAP_INIT_INCOMPLETELY { { NULL } }
Alternatively, could we define the macro to an expression that would cause a compiler error? Then any new code written would fail to compile. And we document the issue in a comment above the macro so no one changes the macro to "fix" it.
(I suggest this as opposed to simply removing the macro to prevent someone from re-adding it later, since it is the standard pattern.)
Just a thought,
Jeff