diff mbox series

[4/3] untracked-cache: use FLEX_ALLOC to create internal structs

Message ID 20190418212405.GA18623@sigill.intra.peff.net (mailing list archive)
State New, archived
Headers show
Series untracked cache parsing fixups | expand

Commit Message

Jeff King April 18, 2019, 9:24 p.m. UTC 
On Thu, Apr 18, 2019 at 05:14:08PM -0400, Jeff King wrote:

> Just so we don't forget about it, I wrote this fix up as a patch. And in
> fact it led to a few other cleanups. I think the first one is definitely
> worth doing now, even if there are other similar cases lurking in the
> rest of the index code.
> 
> The other two are optional, though I think they are worth it (and not
> too hard to verify that they are doing the right thing).
> 
> These are on top of js/untracked-cache-allocfix (though they could
> easily be ported to a separate topic if we want).
> 
>   [1/3]: untracked-cache: be defensive about missing NULs in index
>   [2/3]: untracked-cache: simplify parsing by dropping "next"
>   [3/3]: untracked-cache: simplify parsing by dropping "len"

I also wondered if we could just accept the cost of calloc() here and
use FLEX_ALLOC to simplify things. That resulted in the patch below, but
I didn't include it with the initial 3, because I think it's too
subtle/gross for my tastes.

-- >8 --
Subject: untracked-cache: use FLEX_ALLOC to create internal structs

The untracked_cache_dir struct has a FLEX_ARRAY in it. Let's use
FLEX_ALLOC_MEM to allocate it, which saves us having to compute the
length ourselves.

In theory this could be slightly slower, since the FLEX_ALLOC macros use
calloc (and we just memcpy over most of the contents anyway). But in
practice this distinction is not generally measurable.

Note that because we then fill in the pre-flex elements of the struct
using a memcpy, we need to take care to use the exact size of that
space and _not_ "sizeof(ud)", since the latter may include padding (or
even an extra byte on systems where FLEX_ARRAY is 1).

Signed-off-by: Jeff King <peff@peff.net>
---
If we wanted to go this route, I think it would make sense to provide a
FLEX_ALLOC macro that takes a "template" set of bytes as a ptr/len pair,
and writes it before we fill in the flex portion.

Then we could do something like:

  FLEX_ALLOC_COPY(untracked, &ud, sizeof(ud), name, data, eos - data);

If this is the only such case, it's probably not worth it (I didn't
really look around for more, though).

 dir.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Duy Nguyen April 19, 2019, 9:18 a.m. UTC | #1 
On Fri, Apr 19, 2019 at 4:24 AM Jeff King <peff@peff.net> wrote:
>
> On Thu, Apr 18, 2019 at 05:14:08PM -0400, Jeff King wrote:
>
> > Just so we don't forget about it, I wrote this fix up as a patch. And in
> > fact it led to a few other cleanups. I think the first one is definitely
> > worth doing now, even if there are other similar cases lurking in the
> > rest of the index code.
> >
> > The other two are optional, though I think they are worth it (and not
> > too hard to verify that they are doing the right thing).
> >
> > These are on top of js/untracked-cache-allocfix (though they could
> > easily be ported to a separate topic if we want).
> >
> >   [1/3]: untracked-cache: be defensive about missing NULs in index
> >   [2/3]: untracked-cache: simplify parsing by dropping "next"
> >   [3/3]: untracked-cache: simplify parsing by dropping "len"
>
> I also wondered if we could just accept the cost of calloc() here and
> use FLEX_ALLOC to simplify things. That resulted in the patch below, but
> I didn't include it with the initial 3, because I think it's too
> subtle/gross for my tastes.

It's probably ok. If I remember correctly, reading UNTR extension (on
a huge repo) took the longest time after Ben added support for reading
the index with multiple threads. So performance is a concern, but I
don't think calloc() would be the problem. malloc() itself without the
memory pool could probably slow down more when we have lots and lots
of directories.

> -- >8 --
> Subject: untracked-cache: use FLEX_ALLOC to create internal structs
>
> The untracked_cache_dir struct has a FLEX_ARRAY in it. Let's use
> FLEX_ALLOC_MEM to allocate it, which saves us having to compute the
> length ourselves.
>
> In theory this could be slightly slower, since the FLEX_ALLOC macros use
> calloc (and we just memcpy over most of the contents anyway). But in
> practice this distinction is not generally measurable.
>
> Note that because we then fill in the pre-flex elements of the struct
> using a memcpy, we need to take care to use the exact size of that
> space and _not_ "sizeof(ud)", since the latter may include padding (or
> even an extra byte on systems where FLEX_ARRAY is 1).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> If we wanted to go this route, I think it would make sense to provide a
> FLEX_ALLOC macro that takes a "template" set of bytes as a ptr/len pair,
> and writes it before we fill in the flex portion.
>
> Then we could do something like:
>
>   FLEX_ALLOC_COPY(untracked, &ud, sizeof(ud), name, data, eos - data);
>
> If this is the only such case, it's probably not worth it (I didn't
> really look around for more, though).
>
>  dir.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index 60438b2cdc..7cd4eec198 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2757,9 +2757,9 @@ static int read_one_dir(struct untracked_cache_dir **untracked_,
>         if (!eos || eos == end)
>                 return -1;
>
> -       *untracked_ = untracked = xmalloc(st_add3(sizeof(*untracked), eos - data, 1));
> -       memcpy(untracked, &ud, sizeof(ud));
> -       memcpy(untracked->name, data, eos - data + 1);
> +       FLEX_ALLOC_MEM(untracked, name, data, eos - data);
> +       memcpy(untracked, &ud, offsetof(struct untracked_cache_dir, name));
> +       *untracked_ = untracked;
>         data = eos + 1;
>
>         for (i = 0; i < untracked->untracked_nr; i++) {
> --
> 2.21.0.1092.g8b0302e9c4
>
Jeff King April 19, 2019, 7:43 p.m. UTC | #2 
On Fri, Apr 19, 2019 at 04:18:25PM +0700, Duy Nguyen wrote:

> > I also wondered if we could just accept the cost of calloc() here and
> > use FLEX_ALLOC to simplify things. That resulted in the patch below, but
> > I didn't include it with the initial 3, because I think it's too
> > subtle/gross for my tastes.
> 
> It's probably ok. If I remember correctly, reading UNTR extension (on
> a huge repo) took the longest time after Ben added support for reading
> the index with multiple threads. So performance is a concern, but I
> don't think calloc() would be the problem. malloc() itself without the
> memory pool could probably slow down more when we have lots and lots
> of directories.

I think if we do the FLEX_ALLOC_COPY() thing I mentioned that it would
probably _not_ use calloc() there, since we'd know we were copying in
the content from elsewhere. So that concern would go away either way. :)

(But I'm still skeptical that FLEX_ALLOC_COPY() is worth it unless we
can find at least one other caller).

-Peff
diff mbox series

Patch

diff --git a/dir.c b/dir.c
index 60438b2cdc..7cd4eec198 100644
--- a/dir.c
+++ b/dir.c
@@ -2757,9 +2757,9 @@  static int read_one_dir(struct untracked_cache_dir **untracked_,
 	if (!eos || eos == end)
 		return -1;
 
-	*untracked_ = untracked = xmalloc(st_add3(sizeof(*untracked), eos - data, 1));
-	memcpy(untracked, &ud, sizeof(ud));
-	memcpy(untracked->name, data, eos - data + 1);
+	FLEX_ALLOC_MEM(untracked, name, data, eos - data);
+	memcpy(untracked, &ud, offsetof(struct untracked_cache_dir, name));
+	*untracked_ = untracked;
 	data = eos + 1;
 
 	for (i = 0; i < untracked->untracked_nr; i++) {