Re: [RFC/PATCH] commit-slab.h: Fix memory allocation and addressing
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 29, 2013, 15:42 UTC
- Message-ID
- <7vppu11if4.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <51F418B7.30507@ramsay1.demon.co.uk>
Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
Show 13 quoted lines
> I was convinced, just by reading the code in the header, that when > used with stride > 1, the memory allocated to a slab would not be > sufficient. (ie it would be too small by: > s->slab_size * (sizeof(**s->slab) * (stride - 1)) > ). So, I had expected t3202-show-branch-octopus.sh to provoke memory > error reports when run under valgrind. > > Hmm, it didn't ... so much for that theory! :-D > > So, I'm a little puzzled; I must be missing something obvious, which > is why this is marked RFC. > > What am I missing?
Because sizeof(struct reachabe_cslab) is much larger than sizeof(unsigned char), and the test does not use that many commits to cause us to spill to more than one slab?
Your fix of elem_size in your first hunk is correct; for stride N, we want to allocate N*sizeof(elemtype) consecutive bytes in a single slab (we do not want to split the array of N elements across two slabs). So "elem_size" is the size of the array for a single commit (when stride==1, we are storing an array with a single element).
And we can fit arrays for s->slab_size = (COMMIT_SLAB_SIZE / elem_size) commits in a single slab. c->index / s->slab_size gives us which slab the array for the commit goes, and modulus gives us which slot in that slab the array sits. So the second hunk looks correct, too.
And of course, the slab must be allocated as an array with s->slab_size elements, each of which holds s->stride elements of elem_type that is sizeof(**s->slab) bytes long.
Thanks; the patch looks correct to me.
It is somewhat embarrassing that nobody caught it X-<.
Show 42 quoted lines
> commit-slab.h | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/commit-slab.h b/commit-slab.h
> index 7d48163..d4c8286 100644
> --- a/commit-slab.h
> +++ b/commit-slab.h
> @@ -48,7 +48,7 @@ static void init_ ##slabname## _with_stride(struct slabname *s, \
> if (!stride) \
> stride = 1; \
> s->stride = stride; \
> - elem_size = sizeof(struct slabname) * stride; \
> + elem_size = sizeof(elemtype) * stride; \
> s->slab_size = COMMIT_SLAB_SIZE / elem_size; \
> s->slab_count = 0; \
> s->slab = NULL; \
> @@ -72,11 +72,10 @@ static void clear_ ##slabname(struct slabname *s) \
> static elemtype *slabname## _at(struct slabname *s, \
> const struct commit *c) \
> { \
> - int nth_slab, nth_slot, ix; \
> + int nth_slab, nth_slot; \
> \
> - ix = c->index * s->stride; \
> - nth_slab = ix / s->slab_size; \
> - nth_slot = ix % s->slab_size; \
> + nth_slab = c->index / s->slab_size; \
> + nth_slot = c->index % s->slab_size; \
> \
> if (s->slab_count <= nth_slab) { \
> int i; \
> @@ -89,8 +88,8 @@ static elemtype *slabname## _at(struct slabname *s, \
> } \
> if (!s->slab[nth_slab]) \
> s->slab[nth_slab] = xcalloc(s->slab_size, \
> - sizeof(**s->slab)); \
> - return &s->slab[nth_slab][nth_slot]; \
> + sizeof(**s->slab) * s->stride); \
> + return &s->slab[nth_slab][nth_slot * s->stride]; \
> } \
> \
> static int stat_ ##slabname## realloc