# [PATCH] dir: avoid allocation in fill_directory() 4 messages from 2017-02-07 to 2017-02-10. Participants: René Scharfe, Duy Nguyen, Brandon Williams. Thread: https://gitlist.dev/t/45076 ## René Scharfe, 2017-02-07 22:04 Subject: [PATCH] dir: avoid allocation in fill_directory() Message-ID: <73ec9cc7-8a86-8ba9-90fd-a954fa031820@web.de> URL: https://gitlist.dev/e/73ec9cc7-8a86-8ba9-90fd-a954fa031820%40web.de ``` Pass the match member of the first pathspec item directly to read_directory() instead of using common_prefix() to duplicate it first, thus avoiding memory duplication, strlen(3) and free(3). Signed-off-by: Rene Scharfe --- This updates 966de3028 (dir: convert fill_directory to use the pathspec struct interface). The result is closer to the original, yet still using the modern interface. This patch eerily resembles 2b189435 (dir: simplify fill_directory()). dir.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/dir.c b/dir.c index 65c3e681b8..4541f9e146 100644 --- a/dir.c +++ b/dir.c @@ -174,20 +174,19 @@ char *common_prefix(const struct pathspec *pathspec) int fill_directory(struct dir_struct *dir, const struct pathspec *pathspec) { - char *prefix; + const char *prefix; size_t prefix_len; /* * Calculate common prefix for the pathspec, and * use that to optimize the directory walk */ - prefix = common_prefix(pathspec); - prefix_len = prefix ? strlen(prefix) : 0; + prefix_len = common_prefix_len(pathspec); + prefix = prefix_len ? pathspec->items[0].match : ""; /* Read the directory and prune it */ read_directory(dir, prefix, prefix_len, pathspec); - free(prefix); return prefix_len; } -- 2.11.1 ``` ## Duy Nguyen, 2017-02-08 06:22 Subject: Re: [PATCH] dir: avoid allocation in fill_directory() Message-ID: URL: https://gitlist.dev/e/CACsJy8CE-cyTZHZZhvhdsNau7iSqBci1BdUqDYvxoE5odV2SBA%40mail.gmail.com In-Reply-To: <73ec9cc7-8a86-8ba9-90fd-a954fa031820@web.de> ``` On Wed, Feb 8, 2017 at 5:04 AM, René Scharfe wrote: > Pass the match member of the first pathspec item directly to > read_directory() instead of using common_prefix() to duplicate it first, > thus avoiding memory duplication, strlen(3) and free(3). How about killing common_prefix()? There are two other callers in ls-files.c and commit.c and it looks safe to do (but I didn't look very hard). > diff --git a/dir.c b/dir.c > index 65c3e681b8..4541f9e146 100644 > --- a/dir.c > +++ b/dir.c > @@ -174,20 +174,19 @@ char *common_prefix(const struct pathspec *pathspec) > > int fill_directory(struct dir_struct *dir, const struct pathspec *pathspec) > { > - char *prefix; > + const char *prefix; > size_t prefix_len; > > /* > * Calculate common prefix for the pathspec, and > * use that to optimize the directory walk > */ > - prefix = common_prefix(pathspec); > - prefix_len = prefix ? strlen(prefix) : 0; > + prefix_len = common_prefix_len(pathspec); > + prefix = prefix_len ? pathspec->items[0].match : ""; There's a subtle difference. Before the patch, prefix[prefix_len] is NUL. After the patch, it's not always true. If some code (incorrectly) depends on that, this patch exposes it. I had a look inside read_directory() though and it looks like no such code exists. So, all good. > > /* Read the directory and prune it */ > read_directory(dir, prefix, prefix_len, pathspec); > > - free(prefix); > return prefix_len; > } -- Duy ``` ## Brandon Williams, 2017-02-08 19:54 Subject: Re: [PATCH] dir: avoid allocation in fill_directory() Message-ID: <20170208195437.GA108686@google.com> URL: https://gitlist.dev/e/20170208195437.GA108686%40google.com In-Reply-To: ``` On 02/08, Duy Nguyen wrote: > On Wed, Feb 8, 2017 at 5:04 AM, René Scharfe wrote: > > Pass the match member of the first pathspec item directly to > > read_directory() instead of using common_prefix() to duplicate it first, > > thus avoiding memory duplication, strlen(3) and free(3). > > How about killing common_prefix()? There are two other callers in > ls-files.c and commit.c and it looks safe to do (but I didn't look > very hard). > > > diff --git a/dir.c b/dir.c > > index 65c3e681b8..4541f9e146 100644 > > --- a/dir.c > > +++ b/dir.c > > @@ -174,20 +174,19 @@ char *common_prefix(const struct pathspec *pathspec) > > > > int fill_directory(struct dir_struct *dir, const struct pathspec *pathspec) > > { > > - char *prefix; > > + const char *prefix; > > size_t prefix_len; > > > > /* > > * Calculate common prefix for the pathspec, and > > * use that to optimize the directory walk > > */ > > - prefix = common_prefix(pathspec); > > - prefix_len = prefix ? strlen(prefix) : 0; > > + prefix_len = common_prefix_len(pathspec); > > + prefix = prefix_len ? pathspec->items[0].match : ""; > > There's a subtle difference. Before the patch, prefix[prefix_len] is > NUL. After the patch, it's not always true. If some code (incorrectly) > depends on that, this patch exposes it. I had a look inside > read_directory() though and it looks like no such code exists. So, all > good. Yeah I had the exact same thought when looking at this, but I agree everything looks fine. And if something does indeed depend on prefix having a \0 at prefix_len then this will allow us to more easily find the bug and fix it. > > > > > /* Read the directory and prune it */ > > read_directory(dir, prefix, prefix_len, pathspec); > > > > - free(prefix); > > return prefix_len; > > } > -- > Duy -- Brandon Williams ``` ## René Scharfe, 2017-02-10 19:42 Subject: Re: [PATCH] dir: avoid allocation in fill_directory() Message-ID: <213ef44b-9161-1e64-772c-76d93619b6f6@web.de> URL: https://gitlist.dev/e/213ef44b-9161-1e64-772c-76d93619b6f6%40web.de In-Reply-To: ``` Am 08.02.2017 um 07:22 schrieb Duy Nguyen: > On Wed, Feb 8, 2017 at 5:04 AM, René Scharfe wrote: >> Pass the match member of the first pathspec item directly to >> read_directory() instead of using common_prefix() to duplicate it first, >> thus avoiding memory duplication, strlen(3) and free(3). > > How about killing common_prefix()? There are two other callers in > ls-files.c and commit.c and it looks safe to do (but I didn't look > very hard). I would like that, but it doesn't look like it's worth it. I have a patch series for making overlay_tree_on_cache() take pointer+length, but it's surprisingly long and bloats the code. Duplicating a small piece of memory once per command doesn't look so bad in comparison. (The payoff for avoiding an allocation is higher for library functions like fill_directory().) But while working on that I found two opportunities for improvement in prune_cache(). I'll send patches shortly. > There's a subtle difference. Before the patch, prefix[prefix_len] is > NUL. After the patch, it's not always true. If some code (incorrectly) > depends on that, this patch exposes it. I had a look inside > read_directory() though and it looks like no such code exists. So, all > good. Thanks for checking. NB: The code before 966de302 (dir: convert fill_directory to use the pathspec struct interface, committed 2017-01-04) made the same assumption, i.e. that NUL is not needed. René ```