{"thread":{"id":"47481","subject":"[PATCH] oidmap.h: strongly discourage using OIDMAP_INIT directly","startedAt":"2017-12-22T10:59:37Z","lastAt":"2018-01-02T18:13:37Z","messageCount":8,"participants":["Johannes Schindelin","Brandon Williams","Junio C Hamano","Jeff Hostetler"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"335164","messageId":"940d8daa8eb8d42b9f0a3ebf4c25fb6417e71bc8.1513940345.git.johannes.schindelin@gmx.de","threadId":"47481","inReplyTo":null,"subject":"[PATCH] oidmap.h: strongly discourage using OIDMAP_INIT directly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-12-22T10:59:23Z","receivedAt":"2017-12-22T10:59:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In Git's source code, we have this convention that quite a few data\nstructures can be initialized using a macro *_INIT while defining an\ninstance (instead of having to call a function to initialize the data\nstructure). You will see that idiom quite a bit, e.g. `struct strbuf buf\n= STRBUF_INIT;`\n\nThis works for oidsets, too: `struct oidset oids = OIDSET_INIT;` is\nperfectly legal and you can use that data structure right away, without\nhaving to call `oidset_init()` first.\n\nThat pattern is violated by OIDMAP_INIT, though. The first call to\noidmap_put() or oidmap_get() will succeed, but by mistake rather than by\ndesign: The underlying hashmap is not initialized correctly, as the\ncmpfn function pointer still points to NULL, but since there are no\nentries to be compared, cmpfn will not be called. Things break down,\nthough, as soon as there is even one entry.\n\nRather than causing confusion, frustration and needless loss of time due\nto pointless debugging, let's *rename* OIDMAP_INIT so that developers\nwho have gotten used to the pattern `struct xyz a = XYZ_INIT;` won't do\nthat with oidmaps.\n\nAn alternative would be to introduce a HASHMAP_INIT(cmp_fn) and use that\nin oidmap.h. However, there are *no* call sites in Git's source code\nthat would benefit from that macro, i.e. this would be premature\noptimization (and cost a lot more time than this here trivial patch).\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\nPublished-As: https://github.com/dscho/git/releases/tag/discourage-OIDMAP_INIT-v1\nFetch-It-Via: git fetch https://github.com/dscho/git discourage-OIDMAP_INIT-v1\n oidmap.h | 6 +++++-\n oidset.h | 7 ++++++-\n 2 files changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/oidmap.h b/oidmap.h\nindex 18f54cde143..1a73c392b79 100644\n--- a/oidmap.h\n+++ b/oidmap.h\n@@ -21,7 +21,11 @@ struct oidmap {\n \tstruct hashmap map;\n };\n \n-#define OIDMAP_INIT { { NULL } }\n+/*\n+ * This macro initializes the data structure only incompletely, just enough\n+ * to call oidmap_get() on an empty map. Use oidmap_init() instead.\n+ */\n+#define OIDMAP_INIT_INCOMPLETELY { { NULL } }\n \n /*\n  * Initializes an oidmap structure.\ndiff --git a/oidset.h b/oidset.h\nindex f4c9e0f9c04..a11d88edc1d 100644\n--- a/oidset.h\n+++ b/oidset.h\n@@ -22,7 +22,12 @@ struct oidset {\n \tstruct oidmap map;\n };\n \n-#define OIDSET_INIT { OIDMAP_INIT }\n+/*\n+ * It is okay to initialize the map incompletely here because oidset_insert()\n+ * will call oidset_init() (which will call oidmap_init()), and\n+ * oidset_contains() works as intended even before oidset_init() was called.\n+ */\n+#define OIDSET_INIT { OIDMAP_INIT_INCOMPLETELY }\n \n /**\n  * Returns true iff `set` contains `oid`.\n\nbase-commit: 936d1b989416a95f593bf81ccae8ac62cd83f279\n-- \n2.15.1.windows.2\n"},{"id":"335173","messageId":"20171222171635.GA230194@google.com","threadId":"47481","inReplyTo":"940d8daa8eb8d42b9f0a3ebf4c25fb6417e71bc8.1513940345.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] oidmap.h: strongly discourage using OIDMAP_INIT directly","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-22T17:16:35Z","receivedAt":"2017-12-22T17:16:44Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/22, Johannes Schindelin wrote:\n> In Git's source code, we have this convention that quite a few data\n> structures can be initialized using a macro *_INIT while defining an\n> instance (instead of having to call a function to initialize the data\n> structure). You will see that idiom quite a bit, e.g. `struct strbuf buf\n> = STRBUF_INIT;`\n> \n> This works for oidsets, too: `struct oidset oids = OIDSET_INIT;` is\n> perfectly legal and you can use that data structure right away, without\n> having to call `oidset_init()` first.\n> \n> That pattern is violated by OIDMAP_INIT, though. The first call to\n> oidmap_put() or oidmap_get() will succeed, but by mistake rather than by\n> design: The underlying hashmap is not initialized correctly, as the\n> cmpfn function pointer still points to NULL, but since there are no\n> entries to be compared, cmpfn will not be called. Things break down,\n> though, as soon as there is even one entry.\n> \n> Rather than causing confusion, frustration and needless loss of time due\n> to pointless debugging, let's *rename* OIDMAP_INIT so that developers\n> who have gotten used to the pattern `struct xyz a = XYZ_INIT;` won't do\n> that with oidmaps.\n> \n> An alternative would be to introduce a HASHMAP_INIT(cmp_fn) and use that\n> in oidmap.h. However, there are *no* call sites in Git's source code\n> that would benefit from that macro, i.e. this would be premature\n> optimization (and cost a lot more time than this here trivial patch).\n> \n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n> Published-As: https://github.com/dscho/git/releases/tag/discourage-OIDMAP_INIT-v1\n> Fetch-It-Via: git fetch https://github.com/dscho/git discourage-OIDMAP_INIT-v1\n>  oidmap.h | 6 +++++-\n>  oidset.h | 7 ++++++-\n>  2 files changed, 11 insertions(+), 2 deletions(-)\n> \n> diff --git a/oidmap.h b/oidmap.h\n> index 18f54cde143..1a73c392b79 100644\n> --- a/oidmap.h\n> +++ b/oidmap.h\n> @@ -21,7 +21,11 @@ struct oidmap {\n>  \tstruct hashmap map;\n>  };\n>  \n> -#define OIDMAP_INIT { { NULL } }\n> +/*\n> + * This macro initializes the data structure only incompletely, just enough\n> + * to call oidmap_get() on an empty map. Use oidmap_init() instead.\n> + */\n> +#define OIDMAP_INIT_INCOMPLETELY { { NULL } }\n\nThis is one way of approaching the problem.  Couldn't we also take the\napproach like we have with oidset and ensure that when oidmap_get() or\n_put() are called that if the oidmap isn't initialized, we simply do\nthat then?\n\n>  \n>  /*\n>   * Initializes an oidmap structure.\n> diff --git a/oidset.h b/oidset.h\n> index f4c9e0f9c04..a11d88edc1d 100644\n> --- a/oidset.h\n> +++ b/oidset.h\n> @@ -22,7 +22,12 @@ struct oidset {\n>  \tstruct oidmap map;\n>  };\n>  \n> -#define OIDSET_INIT { OIDMAP_INIT }\n> +/*\n> + * It is okay to initialize the map incompletely here because oidset_insert()\n> + * will call oidset_init() (which will call oidmap_init()), and\n> + * oidset_contains() works as intended even before oidset_init() was called.\n> + */\n> +#define OIDSET_INIT { OIDMAP_INIT_INCOMPLETELY }\n>  \n>  /**\n>   * Returns true iff `set` contains `oid`.\n> \n> base-commit: 936d1b989416a95f593bf81ccae8ac62cd83f279\n> -- \n> 2.15.1.windows.2\n\n-- \nBrandon Williams\n"},{"id":"335189","messageId":"xmqqlghur08f.fsf@gitster.mtv.corp.google.com","threadId":"47481","inReplyTo":"20171222171635.GA230194@google.com","subject":"Re: [PATCH] oidmap.h: strongly discourage using OIDMAP_INIT directly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-22T20:12:00Z","receivedAt":"2017-12-22T20:12:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\n>> -#define OIDMAP_INIT { { NULL } }\n>> +/*\n>> + * This macro initializes the data structure only incompletely, just enough\n>> + * to call oidmap_get() on an empty map. Use oidmap_init() instead.\n>> + */\n>> +#define OIDMAP_INIT_INCOMPLETELY { { NULL } }\n>\n> This is one way of approaching the problem.  Couldn't we also take the\n> approach like we have with oidset and ensure that when oidmap_get() or\n> _put() are called that if the oidmap isn't initialized, we simply do\n> that then?\n\nHmph.  Can you show how the alternative code would look like?\n"},{"id":"335204","messageId":"20171222232729.253936-1-bmwill@google.com","threadId":"47481","inReplyTo":"xmqqlghur08f.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] oidmap: ensure map is initialized","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-22T23:27:29Z","receivedAt":"2017-12-22T23:27:41Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Ensure that an oidmap is initialized before attempting to add, remove,\nor retrieve an entry by simply performing the initialization step\nbefore accessing the underlying hashmap.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n oidmap.c | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/oidmap.c b/oidmap.c\nindex 6db4fffcd..d9fb19ba6 100644\n--- a/oidmap.c\n+++ b/oidmap.c\n@@ -33,12 +33,19 @@ void oidmap_free(struct oidmap *map, int free_entries)\n \n void *oidmap_get(const struct oidmap *map, const struct object_id *key)\n {\n+\tif (!map->map.cmpfn)\n+\t\treturn NULL;\n+\n \treturn hashmap_get_from_hash(&map->map, hash(key), key);\n }\n \n void *oidmap_remove(struct oidmap *map, const struct object_id *key)\n {\n \tstruct hashmap_entry entry;\n+\n+\tif (!map->map.cmpfn)\n+\t\toidmap_init(map, 0);\n+\n \thashmap_entry_init(&entry, hash(key));\n \treturn hashmap_remove(&map->map, &entry, key);\n }\n@@ -46,6 +53,10 @@ void *oidmap_remove(struct oidmap *map, const struct object_id *key)\n void *oidmap_put(struct oidmap *map, void *entry)\n {\n \tstruct oidmap_entry *to_put = entry;\n+\n+\tif (!map->map.cmpfn)\n+\t\toidmap_init(map, 0);\n+\n \thashmap_entry_init(&to_put->internal_entry, hash(&to_put->oid));\n \treturn hashmap_put(&map->map, to_put);\n }\n-- \n2.15.1.620.gb9897f4670-goog\n\n"},{"id":"335212","messageId":"alpine.DEB.2.21.1.1712230112100.406@MININT-6BKU6QN.europe.corp.microsoft.com","threadId":"47481","inReplyTo":"xmqqlghur08f.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] oidmap.h: strongly discourage using OIDMAP_INIT directly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-12-23T00:15:10Z","receivedAt":"2017-12-23T00:15:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 22 Dec 2017, Junio C Hamano wrote:\n\n> Brandon Williams <bmwill@google.com> writes:\n> \n> >> -#define OIDMAP_INIT { { NULL } }\n> >> +/*\n> >> + * This macro initializes the data structure only incompletely, just enough\n> >> + * to call oidmap_get() on an empty map. Use oidmap_init() instead.\n> >> + */\n> >> +#define OIDMAP_INIT_INCOMPLETELY { { NULL } }\n> >\n> > This is one way of approaching the problem.  Couldn't we also take the\n> > approach like we have with oidset and ensure that when oidmap_get() or\n> > _put() are called that if the oidmap isn't initialized, we simply do\n> > that then?\n> \n> Hmph.  Can you show how the alternative code would look like?\n\nNo, because I refuse to perform pointless work, in particular when I am\nalready pretty booked with work.\n\nBut you know how it would look like, right? The cmpfn() function would be\nexported via oidmap.h, and a HASHMAP_INIT(cmpfn) would be introduced in\nhashmap.h that would initialize everything zeroed out except for the\ncmpfn.\n\nBut then you would review it and ask if there would be any use in adding\ncmp_cb_data to the signature of the HASHMAP_INIT() macro, and I would have\nto implement that, too.\n\nAnd then nobody would use it, and the macro would very likely get\nstale/incorrect. And then I would offer another patch reverting that\nchange (because there is no user) and replace it with this here patch.\n\nAs I said: pointless,\nDscho\n"},{"id":"335213","messageId":"alpine.DEB.2.21.1.1712230118340.406@MININT-6BKU6QN.europe.corp.microsoft.com","threadId":"47481","inReplyTo":"20171222232729.253936-1-bmwill@google.com","subject":"Re: [PATCH] oidmap: ensure map is initialized","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-12-23T00:23:53Z","receivedAt":"2017-12-23T00:24:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Brandon,\n\nOn Fri, 22 Dec 2017, Brandon Williams wrote:\n\n> Ensure that an oidmap is initialized before attempting to add, remove,\n> or retrieve an entry by simply performing the initialization step\n> before accessing the underlying hashmap.\n> \n> Signed-off-by: Brandon Williams <bmwill@google.com>\n> ---\n>  oidmap.c | 11 +++++++++++\n>  1 file changed, 11 insertions(+)\n> \n> diff --git a/oidmap.c b/oidmap.c\n> index 6db4fffcd..d9fb19ba6 100644\n> --- a/oidmap.c\n> +++ b/oidmap.c\n> @@ -33,12 +33,19 @@ void oidmap_free(struct oidmap *map, int free_entries)\n>  \n>  void *oidmap_get(const struct oidmap *map, const struct object_id *key)\n>  {\n> +\tif (!map->map.cmpfn)\n> +\t\treturn NULL;\n> +\n\nThis one is unnecessary: if we always _init() before we add, then\nhashmap_get_from_hash() will not have anything to compare, and therefore\nnot call cmpfn.\n\nYou could argue that it is only a teeny-tiny runtime cost, so it'd be\nbetter to be safe than to be sorry.\n\nHowever, it is a death by a thousand cuts. My colleagues and I try to\nshave off a few percent here and a few percent there, in the hope to get\nsome performance improvements worth writing home about. And then patches\nlike this one come in that simply add runtime without much in the way of\nperformance considerations.\n\nAnd that makes me quite a bit sad.\n\n>  \treturn hashmap_get_from_hash(&map->map, hash(key), key);\n>  }\n>  \n>  void *oidmap_remove(struct oidmap *map, const struct object_id *key)\n>  {\n>  \tstruct hashmap_entry entry;\n> +\n> +\tif (!map->map.cmpfn)\n> +\t\toidmap_init(map, 0);\n> +\n>  \thashmap_entry_init(&entry, hash(key));\n>  \treturn hashmap_remove(&map->map, &entry, key);\n>  }\n> @@ -46,6 +53,10 @@ void *oidmap_remove(struct oidmap *map, const struct object_id *key)\n>  void *oidmap_put(struct oidmap *map, void *entry)\n>  {\n>  \tstruct oidmap_entry *to_put = entry;\n> +\n> +\tif (!map->map.cmpfn)\n> +\t\toidmap_init(map, 0);\n> +\n>  \thashmap_entry_init(&to_put->internal_entry, hash(&to_put->oid));\n>  \treturn hashmap_put(&map->map, to_put);\n>  }\n\n\"But it does not add a lot, it's only a couple of microseconds\"\n\nSure. But we could (and do) simply initialize the hashmaps once, and avoid\nhaving to spend unnecessary cycles for every single access.\n\nI *much* prefer my original patch that essentially does not change *any*\ncode path. Everything stays the same, except that there is now a strong\nhint explaining why you need to call oidmap_init() manually instead of\nusing the OIDMAP_INIT macro and then wonder why your code crashes.\n\nCiao,\nDscho\n"},{"id":"335367","messageId":"xmqqtvwcnj6w.fsf@gitster.mtv.corp.google.com","threadId":"47481","inReplyTo":"20171222232729.253936-1-bmwill@google.com","subject":"Re: [PATCH] oidmap: ensure map is initialized","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-27T18:01:59Z","receivedAt":"2017-12-27T18:02:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\n> Ensure that an oidmap is initialized before attempting to add, remove,\n> or retrieve an entry by simply performing the initialization step\n> before accessing the underlying hashmap.\n>\n> Signed-off-by: Brandon Williams <bmwill@google.com>\n> ---\n\nLooks sane.  Thanks for illustrating the idea with actual code.\n\nEssentially, you are using map->map.cmpfn as a boolean to see \"have\nwe initialized this thing?  if not, we need to initialize it on\ndemand\".  \n\nBy the way, I am somewhat more sympathetic than usual to Dscho's\n\"make oidmap_get() very aware of the internal implementation detail\nof hashmap_get_from_hash() to micro-optimize by removing the check\nfrom _get()\".  Such a layering violation is disgusting, and making\nit deliberately shows an even worse design taste, but in this\nparticular case, because the oidmap API is too thin a layer on top\nof hashmap, it is understandably a very tempting approach.\n\n>  oidmap.c | 11 +++++++++++\n>  1 file changed, 11 insertions(+)\n>\n> diff --git a/oidmap.c b/oidmap.c\n> index 6db4fffcd..d9fb19ba6 100644\n> --- a/oidmap.c\n> +++ b/oidmap.c\n> @@ -33,12 +33,19 @@ void oidmap_free(struct oidmap *map, int free_entries)\n>  \n>  void *oidmap_get(const struct oidmap *map, const struct object_id *key)\n>  {\n> +\tif (!map->map.cmpfn)\n> +\t\treturn NULL;\n> +\n>  \treturn hashmap_get_from_hash(&map->map, hash(key), key);\n>  }\n>  \n>  void *oidmap_remove(struct oidmap *map, const struct object_id *key)\n>  {\n>  \tstruct hashmap_entry entry;\n> +\n> +\tif (!map->map.cmpfn)\n> +\t\toidmap_init(map, 0);\n> +\n>  \thashmap_entry_init(&entry, hash(key));\n>  \treturn hashmap_remove(&map->map, &entry, key);\n>  }\n> @@ -46,6 +53,10 @@ void *oidmap_remove(struct oidmap *map, const struct object_id *key)\n>  void *oidmap_put(struct oidmap *map, void *entry)\n>  {\n>  \tstruct oidmap_entry *to_put = entry;\n> +\n> +\tif (!map->map.cmpfn)\n> +\t\toidmap_init(map, 0);\n> +\n>  \thashmap_entry_init(&to_put->internal_entry, hash(&to_put->oid));\n>  \treturn hashmap_put(&map->map, to_put);\n>  }\n"},{"id":"335617","messageId":"90641fbf-64dd-afda-ef66-857edc6684cf@jeffhostetler.com","threadId":"47481","inReplyTo":"20171222171635.GA230194@google.com","subject":"Re: [PATCH] oidmap.h: strongly discourage using OIDMAP_INIT directly","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2018-01-02T18:13:30Z","receivedAt":"2018-01-02T18:13:37Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 12/22/2017 12:16 PM, Brandon Williams wrote:\n> On 12/22, Johannes Schindelin wrote:\n[...]\n>> That pattern is violated by OIDMAP_INIT, though. The first call to\n>> oidmap_put() or oidmap_get() will succeed, but by mistake rather than by\n>> design: The underlying hashmap is not initialized correctly, as the\n>> cmpfn function pointer still points to NULL, but since there are no\n>> entries to be compared, cmpfn will not be called. Things break down,\n>> though, as soon as there is even one entry.\n>>\n>> Rather than causing confusion, frustration and needless loss of time due\n>> to pointless debugging, let's *rename* OIDMAP_INIT so that developers\n>> who have gotten used to the pattern `struct xyz a = XYZ_INIT;` won't do\n>> that with oidmaps.\n>>   \n>> -#define OIDMAP_INIT { { NULL } }\n>> +/*\n>> + * This macro initializes the data structure only incompletely, just enough\n>> + * to call oidmap_get() on an empty map. Use oidmap_init() instead.\n>> + */\n>> +#define OIDMAP_INIT_INCOMPLETELY { { NULL } }\n\nAlternatively, could we define the macro to an expression\nthat would cause a compiler error?  Then any new code written\nwould fail to compile.  And we document the issue in a comment\nabove the macro so no one changes the macro to \"fix\" it.\n\n(I suggest this as opposed to simply removing the macro\nto prevent someone from re-adding it later, since it is the\nstandard pattern.)\n\nJust a thought,\nJeff\n"}]}