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

From: René Scharfe <l.s.r@web.de>
Date: Feb 10, 2017, 19:42 UTC
Message-ID: <213ef44b-9161-1e64-772c-76d93619b6f6@web.de>
In-Reply-To: <CACsJy8CE-cyTZHZZhvhdsNau7iSqBci1BdUqDYvxoE5odV2SBA@mail.gmail.com>

Am 08.02.2017 um 07:22 schrieb Duy Nguyen:

Show 8 quoted lines

> On Wed, Feb 8, 2017 at 5:04 AM, René Scharfe <l.s.r@web.de> 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.

Show 5 quoted lines

> 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é