diff mbox series

[v2] malloc: move tcache_init out of hot tcache paths

Message ID 20250414134150.275322-1-cupertino.miranda@oracle.com (mailing list archive)
State Superseded
Headers
Series [v2] malloc: move tcache_init out of hot tcache paths |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_glibc_build--master-arm fail Patch failed to apply

Commit Message

Cupertino Miranda April 14, 2025, 1:41 p.m. UTC 
  This patch moves any calls of tcache_init away after tcache hot paths.
Since there is no reason to initialize tcaches in the hot path and
since we need to be able to check tcache != NULL in any case, because of
tcache_thread_shutdown function, moving tcache_init away from hot path
can only be beneficial.
The patch also removes the initialization of tcaches within the
__libc_free call. It only makes sense to initialize tcaches for the
thread after it calls one of the allocation functions.
---
 malloc/malloc.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

Wilco Dijkstra April 15, 2025, 3:32 p.m. UTC | #1 
Hi Cupertino,

> @@ -3433,6 +3434,8 @@ __libc_malloc (size_t bytes)
> 
>   if (tcache_available (tc_idx))
>     return tag_new_usable (tcache_get (tc_idx));
>+  else
>+    MAYBE_INIT_TCACHE ();
>#endif
> 
>   return __libc_malloc2 (bytes);

It still has this which slows down malloc by adding a call. There is another issue with
moving tcache initialization earlier: malloc is initialized in __libc_malloc2, so tcache
will now be initialized before it - however tcache assumes malloc has been initialized
before it...

> @@ -3474,8 +3477,6 @@ __libc_free (void *mem)
>     }
>   else
>     {
> -      MAYBE_INIT_TCACHE ();
> -

On latest trunk this has now moved to _int_free_chunk. We could also remove the errno
save/restore from tcache_init since it's only there because of the above line.

Cheers,
Wilco
Cupertino Miranda April 15, 2025, 3:58 p.m. UTC | #2 
Hi Wilco,

On 15-04-2025 16:32, Wilco Dijkstra wrote:
> Hi Cupertino,
> 
>> @@ -3433,6 +3434,8 @@ __libc_malloc (size_t bytes)
>>   
>>     if (tcache_available (tc_idx))
>>       return tag_new_usable (tcache_get (tc_idx));
>> +  else
>> +    MAYBE_INIT_TCACHE ();
>> #endif
>>   
>>     return __libc_malloc2 (bytes);
> 
> It still has this which slows down malloc by adding a call. There is another issue with
> moving tcache initialization earlier: malloc is initialized in __libc_malloc2, so tcache
> will now be initialized before it - however tcache assumes malloc has been initialized
> before it...
Right, I understand better now the benefits of the tailcall.
That move of the call from __libc_malloc2 to __libc_malloc is clearly 
wrong. Sorry for that.
> 
>> @@ -3474,8 +3477,6 @@ __libc_free (void *mem)
>>       }
>>     else
>>       {
>> -      MAYBE_INIT_TCACHE ();
>> -
> 
> On latest trunk this has now moved to _int_free_chunk. We could also remove the errno
> save/restore from tcache_init since it's only there because of the above line.
Ok, will update the patch! Thanks for the hint on the errno.

Cheers,
Cupertino
diff mbox series

Patch

diff --git a/malloc/malloc.c b/malloc/malloc.c
index a0bc733482..2dcb8abeab 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3361,13 +3361,16 @@  tcache_try_malloc (size_t bytes, void **memptr)
 
   size_t tc_idx = csize2tidx (tbytes);
 
-  MAYBE_INIT_TCACHE ();
 
   if (tcache_available (tc_idx))
-    *memptr = tcache_get (tc_idx);
+    {
+      *memptr = tcache_get (tc_idx);
+      return false;
+    }
   else
     *memptr = NULL;
 
+  MAYBE_INIT_TCACHE ();
   return false;
 }
 
@@ -3393,8 +3396,6 @@  __libc_malloc2 (size_t bytes)
   if (!__malloc_initialized)
     ptmalloc_init ();
 
-  MAYBE_INIT_TCACHE ();
-
   if (SINGLE_THREAD_P)
     {
       victim = tag_new_usable (_int_malloc (&main_arena, bytes));
@@ -3433,6 +3434,8 @@  __libc_malloc (size_t bytes)
 
   if (tcache_available (tc_idx))
     return tag_new_usable (tcache_get (tc_idx));
+  else
+    MAYBE_INIT_TCACHE ();
 #endif
 
   return __libc_malloc2 (bytes);
@@ -3474,8 +3477,6 @@  __libc_free (void *mem)
     }
   else
     {
-      MAYBE_INIT_TCACHE ();
-
       /* Mark the chunk as belonging to the library again.  */
       (void)tag_region (chunk2mem (p), memsize (p));
 
@@ -3696,8 +3697,6 @@  _mid_memalign (size_t alignment, size_t bytes, void *address)
       }
     size_t tc_idx = csize2tidx (tbytes);
 
-    MAYBE_INIT_TCACHE ();
-
     if (tcache_available (tc_idx))
       {
 	/* The tcache itself isn't encoded, but the chain is.  */
@@ -3715,6 +3714,7 @@  _mid_memalign (size_t alignment, size_t bytes, void *address)
 	  }
       }
   }
+  MAYBE_INIT_TCACHE ();
 #endif
 
   if (SINGLE_THREAD_P)