[PATCH v9 1/2] malloc: add tcache support for large chunk caching

[Wilco Dijkstra]

Hi Cupertino,

 
-static void * __attribute_noinline__
-tcache_calloc_init (size_t bytes)
-{
-  tcache_init ();
-  return __libc_calloc2 (bytes);
-}

Why remove this?

@@ -3392,10 +3466,28 @@ void *
 __libc_malloc (size_t bytes)
 {
 #if USE_TCACHE
-  size_t tc_idx = usize2tidx (bytes);
+  size_t nb = checked_request2size (bytes);
+
+  if (nb < mp_.tcache_max_bytes)
+    {
+      size_t tc_idx = csize2tidx (nb);
+      if(__glibc_unlikely (tcache == NULL))
+       {
+         tcache_init ();
+         return __libc_malloc2 (bytes);
+       }

This should be a tailcall to a function like tcache_calloc_init
above - otherwise it actually makes things worse than the
MAYBE_TCACHE_INIT.
 
-  if (tcache_available (tc_idx))
-    return tag_new_usable (tcache_get (tc_idx));
+      if (tc_idx < TCACHE_SMALL_BINS
+         && tcache->entries[tc_idx] != NULL)

This won't work with the else below.

+       return tag_new_usable (tcache_get (tc_idx));
+      else
+       if ((tc_idx = large_csize2tidx (nb)) < TCACHE_MAX_BINS)

This if is redundant, and the initialization should not be in it.

But worse, this is actually incorrect - the small tcache code can
get here and access random bins...


@@ -3439,8 +3529,19 @@ __libc_free (void *mem)
       if (__glibc_unlikely (e->key == tcache_key))
         return tcache_double_free_verify (e);
 
-      if (__glibc_likely (tcache->counts[tc_idx] < mp_.tcache_count))
-        return tcache_put (p, tc_idx);
+      size_t tc_idx = csize2tidx (size);
+      if (tc_idx < TCACHE_SMALL_BINS)
+       {
+          if (__glibc_likely (tcache->counts[tc_idx] < mp_.tcache_count))
+           return tcache_put (p, tc_idx);
+       }
+      else
+       {
+         if ((tc_idx = large_csize2tidx (size)) < TCACHE_MAX_BINS

This check is redundant, the tc_idx initialization should be before it.

+             && !chunk_is_mmapped (p)
+              && __glibc_likely (tcache->counts[tc_idx] < mp_.tcache_count))
+           return tcache_put_large (p, tc_idx);
+       }
     }
 #endif
 

@@ -3825,21 +3908,39 @@ __libc_calloc (size_t n, size_t elem_size)
     }
 
 #if USE_TCACHE
-  size_t tc_idx = usize2tidx (bytes);
-  if (__glibc_likely (tc_idx < mp_.tcache_bins))
+  size_t nb = checked_request2size (bytes);
+  if (nb < mp_.tcache_max_bytes)
     {
       if (__glibc_unlikely (tcache == NULL))
-        return tcache_calloc_init (bytes);
-
-      if (__glibc_likely (tcache->entries[tc_idx] != NULL))
         {
-         void *mem = tcache_get (tc_idx);
+         tcache_init ();
+         return __libc_calloc2 (bytes);
+       }

Same here, why remove the optimization and make things slower?
 
+      size_t tc_idx = csize2tidx (nb);
+
+      if (tc_idx < TCACHE_SMALL_BINS
+         && tcache->entries[tc_idx] != NULL)
+        {
+         void *mem = tcache_get (tc_idx);
           if (__glibc_unlikely (mtag_enabled))
-            return tag_new_zero_region (mem, memsize (mem2chunk (mem)));
+           return tag_new_zero_region (mem, memsize (mem2chunk (mem)));
 
-          return clear_memory ((INTERNAL_SIZE_T *) mem, tidx2usize (tc_idx));
+         return clear_memory ((INTERNAL_SIZE_T *) mem, memsize (mem2chunk (mem)));

Why remove the tidx2usize here?

         }
+      else
+       if ((tc_idx = large_csize2tidx (nb)) < TCACHE_MAX_BINS
+           && tcache->entries[tc_idx] != NULL)

This if is redundant (you don't need the entries check here either
in principle) and incorrect due to the earlier if.

-      LIBC_PROBE (memory_tunable_tcache_max_bytes, 2, value, mp_.tcache_max_bytes);
-      mp_.tcache_max_bytes = value;
-      mp_.tcache_bins = csize2tidx (request2size(value)) + 1;
+      if (tc_idx + 1 < TCACHE_SMALL_BINS)
+       mp_.tcache_small_bins = tc_idx + 1;

 tcache_small_bins can be equal to TCACHE_SMALL_BINS, so use <= in the if.


Cheers,
Wilco