{"thread":{"id":"45076","subject":"[PATCH] dir: avoid allocation in fill_directory()","startedAt":"2017-02-07T22:04:35Z","lastAt":"2017-02-10T19:49:14Z","messageCount":4,"participants":["René Scharfe","Duy Nguyen","Brandon Williams"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"311010","messageId":"73ec9cc7-8a86-8ba9-90fd-a954fa031820@web.de","threadId":"45076","inReplyTo":null,"subject":"[PATCH] dir: avoid allocation in fill_directory()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-02-07T22:04:25Z","receivedAt":"2017-02-07T22:04:35Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Pass the match member of the first pathspec item directly to\nread_directory() instead of using common_prefix() to duplicate it first,\nthus avoiding memory duplication, strlen(3) and free(3).\n\nSigned-off-by: Rene Scharfe \n---\nThis updates 966de3028 (dir: convert fill_directory to use the pathspec\nstruct interface). The result is closer to the original, yet still\nusing the modern interface.\n\nThis patch eerily resembles 2b189435 (dir: simplify fill_directory()).\n\n dir.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 65c3e681b8..4541f9e146 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -174,20 +174,19 @@ char *common_prefix(const struct pathspec *pathspec)\n \n int fill_directory(struct dir_struct *dir, const struct pathspec *pathspec)\n {\n-\tchar *prefix;\n+\tconst char *prefix;\n \tsize_t prefix_len;\n \n \t/*\n \t * Calculate common prefix for the pathspec, and\n \t * use that to optimize the directory walk\n \t */\n-\tprefix = common_prefix(pathspec);\n-\tprefix_len = prefix ? strlen(prefix) : 0;\n+\tprefix_len = common_prefix_len(pathspec);\n+\tprefix = prefix_len ? pathspec->items[0].match : \"\";\n \n \t/* Read the directory and prune it */\n \tread_directory(dir, prefix, prefix_len, pathspec);\n \n-\tfree(prefix);\n \treturn prefix_len;\n }\n \n-- \n2.11.1\n\n"},{"id":"311037","messageId":"CACsJy8CE-cyTZHZZhvhdsNau7iSqBci1BdUqDYvxoE5odV2SBA@mail.gmail.com","threadId":"45076","inReplyTo":"73ec9cc7-8a86-8ba9-90fd-a954fa031820@web.de","subject":"Re: [PATCH] dir: avoid allocation in fill_directory()","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-02-08T06:22:03Z","receivedAt":"2017-02-08T06:22:56Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Feb 8, 2017 at 5:04 AM, René Scharfe wrote:\n> Pass the match member of the first pathspec item directly to\n> read_directory() instead of using common_prefix() to duplicate it first,\n> thus avoiding memory duplication, strlen(3) and free(3).\n\nHow about killing common_prefix()? There are two other callers in\nls-files.c and commit.c and it looks safe to do (but I didn't look\nvery hard).\n\n> diff --git a/dir.c b/dir.c\n> index 65c3e681b8..4541f9e146 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -174,20 +174,19 @@ char *common_prefix(const struct pathspec *pathspec)\n>\n> int fill_directory(struct dir_struct *dir, const struct pathspec *pathspec)\n> {\n> - char *prefix;\n> + const char *prefix;\n> size_t prefix_len;\n>\n> /*\n> * Calculate common prefix for the pathspec, and\n> * use that to optimize the directory walk\n> */\n> - prefix = common_prefix(pathspec);\n> - prefix_len = prefix ? strlen(prefix) : 0;\n> + prefix_len = common_prefix_len(pathspec);\n> + prefix = prefix_len ? pathspec->items[0].match : \"\";\n\nThere's a subtle difference. Before the patch, prefix[prefix_len] is\nNUL. After the patch, it's not always true. If some code (incorrectly)\ndepends on that, this patch exposes it. I had a look inside\nread_directory() though and it looks like no such code exists. So, all\ngood.\n\n>\n> /* Read the directory and prune it */\n> read_directory(dir, prefix, prefix_len, pathspec);\n>\n> - free(prefix);\n> return prefix_len;\n> }\n-- \nDuy\n"},{"id":"311091","messageId":"20170208195437.GA108686@google.com","threadId":"45076","inReplyTo":"CACsJy8CE-cyTZHZZhvhdsNau7iSqBci1BdUqDYvxoE5odV2SBA@mail.gmail.com","subject":"Re: [PATCH] dir: avoid allocation in fill_directory()","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-02-08T19:54:37Z","receivedAt":"2017-02-08T20:59:42Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 02/08, Duy Nguyen wrote:\n> On Wed, Feb 8, 2017 at 5:04 AM, René Scharfe wrote:\n> > Pass the match member of the first pathspec item directly to\n> > read_directory() instead of using common_prefix() to duplicate it first,\n> > thus avoiding memory duplication, strlen(3) and free(3).\n> \n> How about killing common_prefix()? There are two other callers in\n> ls-files.c and commit.c and it looks safe to do (but I didn't look\n> very hard).\n> \n> > diff --git a/dir.c b/dir.c\n> > index 65c3e681b8..4541f9e146 100644\n> > --- a/dir.c\n> > +++ b/dir.c\n> > @@ -174,20 +174,19 @@ char *common_prefix(const struct pathspec *pathspec)\n> >\n> > int fill_directory(struct dir_struct *dir, const struct pathspec *pathspec)\n> > {\n> > - char *prefix;\n> > + const char *prefix;\n> > size_t prefix_len;\n> >\n> > /*\n> > * Calculate common prefix for the pathspec, and\n> > * use that to optimize the directory walk\n> > */\n> > - prefix = common_prefix(pathspec);\n> > - prefix_len = prefix ? strlen(prefix) : 0;\n> > + prefix_len = common_prefix_len(pathspec);\n> > + prefix = prefix_len ? pathspec->items[0].match : \"\";\n> \n> There's a subtle difference. Before the patch, prefix[prefix_len] is\n> NUL. After the patch, it's not always true. If some code (incorrectly)\n> depends on that, this patch exposes it. I had a look inside\n> read_directory() though and it looks like no such code exists. So, all\n> good.\n\nYeah I had the exact same thought when looking at this, but I agree\neverything looks fine. And if something does indeed depend on prefix\nhaving a \\0 at prefix_len then this will allow us to more easily find\nthe bug and fix it.\n\n> \n> >\n> > /* Read the directory and prune it */\n> > read_directory(dir, prefix, prefix_len, pathspec);\n> >\n> > - free(prefix);\n> > return prefix_len;\n> > }\n> -- \n> Duy\n\n-- \nBrandon Williams\n"},{"id":"311298","messageId":"213ef44b-9161-1e64-772c-76d93619b6f6@web.de","threadId":"45076","inReplyTo":"CACsJy8CE-cyTZHZZhvhdsNau7iSqBci1BdUqDYvxoE5odV2SBA@mail.gmail.com","subject":"Re: [PATCH] dir: avoid allocation in fill_directory()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-02-10T19:42:22Z","receivedAt":"2017-02-10T19:49:14Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.02.2017 um 07:22 schrieb Duy Nguyen:\n> On Wed, Feb 8, 2017 at 5:04 AM, René Scharfe wrote:\n>> Pass the match member of the first pathspec item directly to\n>> read_directory() instead of using common_prefix() to duplicate it first,\n>> thus avoiding memory duplication, strlen(3) and free(3).\n>\n> How about killing common_prefix()? There are two other callers in\n> ls-files.c and commit.c and it looks safe to do (but I didn't look\n> very hard).\n\nI would like that, but it doesn't look like it's worth it. I have a \npatch series for making overlay_tree_on_cache() take pointer+length, but \nit's surprisingly long and bloats the code. Duplicating a small piece \nof memory once per command doesn't look so bad in comparison.\n\n(The payoff for avoiding an allocation is higher for library functions \nlike fill_directory().)\n\nBut while working on that I found two opportunities for improvement in \nprune_cache(). I'll send patches shortly.\n\n> There's a subtle difference. Before the patch, prefix[prefix_len] is\n> NUL. After the patch, it's not always true. If some code (incorrectly)\n> depends on that, this patch exposes it. I had a look inside\n> read_directory() though and it looks like no such code exists. So, all\n> good.\n\nThanks for checking.\n\nNB: The code before 966de302 (dir: convert fill_directory to use the \npathspec struct interface, committed 2017-01-04) made the same \nassumption, i.e. that NUL is not needed.\n\nRené\n"}]}