[PATCH v4 4/7] malloc: add tcache support for large chunk caching

[DJ Delorie]

Cupertino Miranda <cupertino.miranda@oracle.com> writes:
> Existing tcache implementation in glibc seems to focus in caching
> smaller data size allocations, limiting the size of the allocation to
> 1KB.

Statistics at that time showed that such packets were by far the most
popular ones to allocate.

> This patch changes tcache implementation to allow to cache any chunk
> size allocations.  The implementation adds extra bins (linked-lists)
> which store chunks with different ranges of allocation sizes. Bin
> selection is done in multiples in powers of 2 and chunks are inserted in
> growing size ordering within the bin.  The last bin contains all other
> sizes of allocations.

I fear this will cause fragmentation and thus higher VSS usage.  I've
already seen proposals for changes in tcache to try to reduce the
fragmentation it's already causing.  Do you have any benchmarks on
memory *usage* changes with this patch?  It would have to be a fairly
complex application.


>  /* We want 64 entries.  This is an arbitrary limit, which tunables can reduce.  */
> +# define TCACHE_UNBOUND_SIZE_BINS	10
>  # define TCACHE_MAX_BINS		64

Putting the new line there means the comment no longer makes sense ;-)

>  # define MAX_TCACHE_SIZE	tidx2usize (TCACHE_MAX_BINS-1)
> +# define TCACHE_FIXED_SIZE_BINS	\
> +	   (mp_.tcache_bins < TCACHE_MAX_BINS ? mp_.tcache_bins : TCACHE_MAX_BINS)

I can see this adding a lot of logic to tcache.  Do the benchmarks
indicate it's faster?

>  /* Only used to pre-fill the tunables.  */
> -# define tidx2usize(idx)	(((size_t) idx) * MALLOC_ALIGNMENT + MINSIZE - SIZE_SZ)
> +# define tidx2usize(idx)	(((size_t) idx) * MALLOC_ALIGNMENT + MINSIZE - MALLOC_ALIGNMENT + 1)

This seems unrelated.  Has this been verified on a platform where you
know SIZE_SZ is different than MALLOC_ALIGNMENT+1 ?

>    ,
>    .tcache_count = TCACHE_FILL_COUNT,
>    .tcache_bins = TCACHE_MAX_BINS,
> -  .tcache_max_bytes = tidx2usize (TCACHE_MAX_BINS-1),
> +  .tcache_max_bytes = MAX_TCACHE_SIZE,
> -  .tcache_unsorted_limit = 0 /* No limit.  */
> +  .tcache_unsorted_limit = 0, /* No limit.  */
>  };

Do we really need the extra comma here?

> -  for (i = 0; i < TCACHE_MAX_BINS; ++i)
> +  for (i = 0; i < mp_.tcache_bins; ++i)

This is wrong if the number of bins can be changed during program
execution; the intent is to flush the WHOLE cache, not the CURRENT
cache.