{"thread":{"id":"54042","subject":"[PATCH] clear_pattern_list(): clear embedded hashmaps","startedAt":"2020-08-14T11:10:52Z","lastAt":"2020-08-17T18:48:35Z","messageCount":5,"participants":["Jeff King","Derrick Stolee","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"403640","messageId":"20200814111049.GA4101811@coredump.intra.peff.net","threadId":"54042","inReplyTo":null,"subject":"[PATCH] clear_pattern_list(): clear embedded hashmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-14T11:10:49Z","receivedAt":"2020-08-14T11:10:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 96cc8ab531 (sparse-checkout: use hashmaps for cone patterns,\n2019-11-21) added some auxiliary hashmaps to the pattern_list struct,\nbut they're leaked when clear_pattern_list() is called.\n\nSigned-off-by: Jeff King \n---\nI have no idea how often this leak triggers in practice. I just noticed\nit while poking at LSan output (which we remain depressingly far\nfrom getting a clean run on).\n\n dir.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/dir.c b/dir.c\nindex fe64be30ed..9411b94e9b 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -916,6 +916,8 @@ void clear_pattern_list(struct pattern_list *pl)\n \t\tfree(pl->patterns[i]);\n \tfree(pl->patterns);\n \tfree(pl->filebuf);\n+\thashmap_free_entries(&pl->recursive_hashmap, struct pattern_entry, ent);\n+\thashmap_free_entries(&pl->parent_hashmap, struct pattern_entry, ent);\n \n \tmemset(pl, 0, sizeof(*pl));\n }\n-- \n2.28.0.596.g9c08d63829\n"},{"id":"403643","messageId":"e0c102e9-57fd-ac0e-72ee-8be5f2095160@gmail.com","threadId":"54042","inReplyTo":"20200814111049.GA4101811@coredump.intra.peff.net","subject":"Re: [PATCH] clear_pattern_list(): clear embedded hashmaps","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-08-14T12:13:23Z","receivedAt":"2020-08-14T12:13:28Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/14/2020 7:10 AM, Jeff King wrote:\n> Commit 96cc8ab531 (sparse-checkout: use hashmaps for cone patterns,\n> 2019-11-21) added some auxiliary hashmaps to the pattern_list struct,\n> but they're leaked when clear_pattern_list() is called.\n> \n> Signed-off-by: Jeff King \n> ---\n> I have no idea how often this leak triggers in practice. I just noticed\n> it while poking at LSan output (which we remain depressingly far\n> from getting a clean run on).\n\nGood find. The impact of the leak is likely low since we don't create\nmultiple pattern_list structs per process (with these hashmaps) very\noften. The sparse-checkout builtin is likely the only place where\nmultiple could be instantiated at the same time.\n\nI also double-checked that hashmap_free_entries() handles a NULL\nhashmap pointer or uninitialized hashmap, which is what happens\nwhen cone mode is not enabled _or_ the pattern_list corresponds to\nsomething like a .gitignore file.\n\nThanks,\n-Stolee\n\n> dir.c | 2 ++\n> 1 file changed, 2 insertions(+)\n> \n> diff --git a/dir.c b/dir.c\n> index fe64be30ed..9411b94e9b 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -916,6 +916,8 @@ void clear_pattern_list(struct pattern_list *pl)\n> \t\tfree(pl->patterns[i]);\n> \tfree(pl->patterns);\n> \tfree(pl->filebuf);\n> +\thashmap_free_entries(&pl->recursive_hashmap, struct pattern_entry, ent);\n> +\thashmap_free_entries(&pl->parent_hashmap, struct pattern_entry, ent);\n> \n> \tmemset(pl, 0, sizeof(*pl));\n> }\n> \n\n"},{"id":"403835","messageId":"CABPp-BGUBPJCW3-GrVvom5CwfauY-m5ZCLJmKP=P=fXWm=Kepg@mail.gmail.com","threadId":"54042","inReplyTo":"CABPp-BGrxaY-ObzuJ60TDMfQD+crq+TTnYE03T=KVmG5mEf-5A@mail.gmail.com","subject":"Re: [PATCH] clear_pattern_list(): clear embedded hashmaps","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-08-17T17:22:27Z","receivedAt":"2020-08-17T17:26:55Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Aug 17, 2020 at 9:55 AM Elijah Newren wrote:\n>\n> Hi,\n>\n> On Fri, Aug 14, 2020 at 5:23 AM Jeff King wrote:\n> >\n> > Commit 96cc8ab531 (sparse-checkout: use hashmaps for cone patterns,\n> > 2019-11-21) added some auxiliary hashmaps to the pattern_list struct,\n> > but they're leaked when clear_pattern_list() is called.\n> >\n> > Signed-off-by: Jeff King \n> > ---\n> > I have no idea how often this leak triggers in practice. I just noticed\n> > it while poking at LSan output (which we remain depressingly far\n> > from getting a clean run on).\n> >\n> > dir.c | 2 ++\n> > 1 file changed, 2 insertions(+)\n> >\n> > diff --git a/dir.c b/dir.c\n> > index fe64be30ed..9411b94e9b 100644\n> > --- a/dir.c\n> > +++ b/dir.c\n> > @@ -916,6 +916,8 @@ void clear_pattern_list(struct pattern_list *pl)\n> > free(pl->patterns[i]);\n> > free(pl->patterns);\n> > free(pl->filebuf);\n> > + hashmap_free_entries(&pl->recursive_hashmap, struct pattern_entry, ent);\n> > + hashmap_free_entries(&pl->parent_hashmap, struct pattern_entry, ent);\n>\n> This clears up the hash entries, but continues to leak the hash table.\n> Since you submitted first, can you fix this to use hashmap_free_()\n> instead, as per\n> https://lore.kernel.org/git/932741d7598ca2934dbca40f715ba2d3819fcc51.1597561152.git.gitgitgadget@gmail.com/?\n> Then I'll rebase my series on yours and drop my first patch (since\n> it'll then be identical).\n\nNevermind, I got confused once again by the name.\nhashmap_free_entries() doesn't mean just free the entries, it means\nfree what hashmap_free() would plus all the entries, i.e. do what\nhashmap_free() *should* *have* *been* defined to do. Such a confusing\nAPI. And hashmap_free() really perplexes me -- it seems like a\nfunction that can't possibly be useful; it's sole purpose seems to be\na trap for the unwary.\n"},{"id":"403842","messageId":"CABPp-BGrxaY-ObzuJ60TDMfQD+crq+TTnYE03T=KVmG5mEf-5A@mail.gmail.com","threadId":"54042","inReplyTo":"20200814111049.GA4101811@coredump.intra.peff.net","subject":"Re: [PATCH] clear_pattern_list(): clear embedded hashmaps","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-08-17T16:55:56Z","receivedAt":"2020-08-17T17:33:09Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Fri, Aug 14, 2020 at 5:23 AM Jeff King wrote:\n>\n> Commit 96cc8ab531 (sparse-checkout: use hashmaps for cone patterns,\n> 2019-11-21) added some auxiliary hashmaps to the pattern_list struct,\n> but they're leaked when clear_pattern_list() is called.\n>\n> Signed-off-by: Jeff King \n> ---\n> I have no idea how often this leak triggers in practice. I just noticed\n> it while poking at LSan output (which we remain depressingly far\n> from getting a clean run on).\n>\n> dir.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/dir.c b/dir.c\n> index fe64be30ed..9411b94e9b 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -916,6 +916,8 @@ void clear_pattern_list(struct pattern_list *pl)\n> free(pl->patterns[i]);\n> free(pl->patterns);\n> free(pl->filebuf);\n> + hashmap_free_entries(&pl->recursive_hashmap, struct pattern_entry, ent);\n> + hashmap_free_entries(&pl->parent_hashmap, struct pattern_entry, ent);\n\nThis clears up the hash entries, but continues to leak the hash table.\nSince you submitted first, can you fix this to use hashmap_free_()\ninstead, as per\nhttps://lore.kernel.org/git/932741d7598ca2934dbca40f715ba2d3819fcc51.1597561152.git.gitgitgadget@gmail.com/?\n Then I'll rebase my series on yours and drop my first patch (since\nit'll then be identical).\n\nThanks,\nElijah\n"},{"id":"403867","messageId":"20200817184821.GA1278968@coredump.intra.peff.net","threadId":"54042","inReplyTo":"CABPp-BGUBPJCW3-GrVvom5CwfauY-m5ZCLJmKP=P=fXWm=Kepg@mail.gmail.com","subject":"Re: [PATCH] clear_pattern_list(): clear embedded hashmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-17T18:48:21Z","receivedAt":"2020-08-17T18:48:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 17, 2020 at 10:22:27AM -0700, Elijah Newren wrote:\n\n> > > + hashmap_free_entries(&pl->recursive_hashmap, struct pattern_entry, ent);\n> > > + hashmap_free_entries(&pl->parent_hashmap, struct pattern_entry, ent);\n> >\n> > This clears up the hash entries, but continues to leak the hash table.\n> > Since you submitted first, can you fix this to use hashmap_free_()\n> > instead, as per\n> > https://lore.kernel.org/git/932741d7598ca2934dbca40f715ba2d3819fcc51.1597561152.git.gitgitgadget@gmail.com/?\n> > Then I'll rebase my series on yours and drop my first patch (since\n> > it'll then be identical).\n> \n> Nevermind, I got confused once again by the name.\n> hashmap_free_entries() doesn't mean just free the entries, it means\n> free what hashmap_free() would plus all the entries, i.e. do what\n> hashmap_free() *should* *have* *been* defined to do. Such a confusing\n> API. And hashmap_free() really perplexes me -- it seems like a\n> function that can't possibly be useful; it's sole purpose seems to be\n> a trap for the unwary.\n\nThere used to be an \"also free entries\" flag, but that got complicated\nby the loosening of the \"hashmap_entry must be at the front of the\nstruct to be freed\" rule.\n\nWith this kind of embedded-entry data structure (and list.h is in the\nsame boat) it _is_ sometimes useful to be part of a data structure\nwithout giving up ownership of the memory. But I agree that the more\nnormal case is to free items when the hashmap is destroyed.\n\nLikewise, the whole \"you have to define a struct that contains the map\nentry\" thing is flexible and efficient, but a pain to use.\n\nI generally find khash's \"map this type to that type, the hash owns the\nmemory\" much more natural. And it doesn't lose efficiency (and indeed\nsometimes even gains it) because it uses macros to store concrete types.\nBut of course macros create their own headaches. :)\n\nAnyway, I'm definitely open to renaming to something more sensible. I\nalready mentioned the free/clear thing earlier, but\nhashmap_clear_entries() ends up _very_ confusing. Because it's clearing\nthe hashmap but freeing the entries. hashmap_clear_and_free_entries() is\nkind of long, but a lot more descriptive.\n\n-Peff\n"}]}