Re: [PATCH 13/13] alloc: allow arbitrary repositories for alloc functions

From: Duy Nguyen <pclouds@gmail.com>
Date: May 2, 2018, 17:44 UTC
Message-ID: <CACsJy8B=i_0EKGRLxS5N5tfz8666hm_aTns949eb9DAW1--XWg@mail.gmail.com>
In-Reply-To: <20180501213403.14643-14-sbeller@google.com>

On Tue, May 1, 2018 at 11:34 PM, Stefan Beller <sbeller@google.com> wrote:

Show 9 quoted lines

>  #include "cache.h"
>  #include "object.h"
> @@ -30,8 +31,25 @@ struct alloc_state {
>         int count; /* total number of nodes allocated */
>         int nr;    /* number of nodes left in current allocation */
>         void *p;   /* first free node in current allocation */
> +
> +       /* bookkeeping of allocations */
> +       void **slabs;

Another way to manage this is linked list: you could reserve one "object" in each slab to store the "next" (or "prev") pointer to another slab, then you can just walk through all slabs and free. It's a bit cheaper than reallocating slabs[], but I guess we reallocate so few times that readability matters more (whichever way is chosen).

Show 13 quoted lines

> +       int slab_nr, slab_alloc;
>  };
>
> +void *allocate_alloc_state(void)
> +{
> +       return xcalloc(1, sizeof(struct alloc_state));
> +}
> +
> +void clear_alloc_state(struct alloc_state *s)
> +{
> +       while (s->slab_nr > 0) {
> +               s->slab_nr--;
> +               free(s->slabs[s->slab_nr]);

I think you're leaking memory here. Commit and tree objects may have more allocations in them (especially trees, but I think we have commit_list in struct commit too). Those need to be freed as well.

Show 6 quoted lines

> +       }
> +}
> +
>  static inline void *alloc_node(struct alloc_state *s, size_t node_size)
>  {
>         void *ret;

-- 
Duy