Re: [PATCH] dir: avoid allocation in fill_directory()

[René Scharfe]

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é

Re: [PATCH] dir: avoid allocation in fill_directory()

[Brandon Williams]

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

Re: [PATCH] dir: avoid allocation in fill_directory()

[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).

> 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

[PATCH] dir: avoid allocation in fill_directory()

[René Scharfe]

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