diff mbox series

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

Message ID 20250415163209.289309-1-cupertino.miranda@oracle.com (mailing list archive)
State Committed
Delegated to: Wilco Dijkstra
Headers
Series [v3] 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
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Build passed
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 fail Patch failed to apply

Commit Message

Cupertino Miranda April 15, 2025, 4:32 p.m. UTC 
  Hi everyone,

New patch version after review from Wilco.

Thanks,
Cupertino

Changes from v2:
 - Updated patch to latest master.
 - Removed call MAYBE_INIT_TCACHE from __libc_malloc.
 - Removed save/restore of errno from tcache_init. It is not longer
   needed.

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. Also the patch
removes the save/restore of errno from tcache_init code, as it is no
longer needed.
---
 malloc/malloc.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

Comments

Wilco Dijkstra April 16, 2025, 11:56 a.m. UTC | #1 
Hi Cupertino,

> 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. Also the patch
> removes the save/restore of errno from tcache_init code, as it is no
> longer needed.

This version looks good. Performance improves by 1.5% on bench-malloc-simple,
and 0.9%/0/2% on bench-malloc-thread 1/32.

Reviewed-by: Wilco Dijkstra  <Wilco.Dijkstra@arm.com>

> @@ -3307,12 +3304,9 @@  tcache_init(void)
>       victim = _int_malloc (ar_ptr, bytes);
>     }
> 
>-
>   if (ar_ptr != NULL)
>     __libc_lock_unlock (ar_ptr->mutex);
 
One minor nit: spurious line removal (can be fixed in commit).

Do you have commit rights, or should I commit this?

Cheers,
Wilco
Cupertino Miranda April 16, 2025, 12:22 p.m. UTC | #2 
Hi Wilco,

Thanks for the review.
No, I do not have commit writes. Please do commit. :)

Thanks,
Cupertino

On 16-04-2025 12:56, Wilco Dijkstra wrote:
> Hi Cupertino,
> 
>> 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. Also the patch
>> removes the save/restore of errno from tcache_init code, as it is no
>> longer needed.
> 
> This version looks good. Performance improves by 1.5% on bench-malloc-simple,
> and 0.9%/0/2% on bench-malloc-thread 1/32.
> 
> Reviewed-by: Wilco Dijkstra  <Wilco.Dijkstra@arm.com>
> 
>> @@ -3307,12 +3304,9 @@  tcache_init(void)
>>        victim = _int_malloc (ar_ptr, bytes);
>>      }
>>
>> -
>>    if (ar_ptr != NULL)
>>      __libc_lock_unlock (ar_ptr->mutex);
>   
> One minor nit: spurious line removal (can be fixed in commit).
> 
> Do you have commit rights, or should I commit this?
> 
> Cheers,
> Wilco
Cupertino Miranda April 16, 2025, 12:54 p.m. UTC | #3 
On 16-04-2025 13:22, Cupertino Miranda wrote:
> Hi Wilco,
> 
> Thanks for the review.
> No, I do not have commit writes. Please do commit. :)
Rights! :)
> 
> Thanks,
> Cupertino
> 
> On 16-04-2025 12:56, Wilco Dijkstra wrote:
>> Hi Cupertino,
>>
>>> 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. Also the patch
>>> removes the save/restore of errno from tcache_init code, as it is no
>>> longer needed.
>>
>> This version looks good. Performance improves by 1.5% on bench-malloc- 
>> simple,
>> and 0.9%/0/2% on bench-malloc-thread 1/32.
>>
>> Reviewed-by: Wilco Dijkstra  <Wilco.Dijkstra@arm.com>
>>
>>> @@ -3307,12 +3304,9 @@  tcache_init(void)
>>>        victim = _int_malloc (ar_ptr, bytes);
>>>      }
>>>
>>> -
>>>    if (ar_ptr != NULL)
>>>      __libc_lock_unlock (ar_ptr->mutex);
>> One minor nit: spurious line removal (can be fixed in commit).
>>
>> Do you have commit rights, or should I commit this?
>>
>> Cheers,
>> Wilco
>
diff mbox series

Patch

diff --git a/malloc/malloc.c b/malloc/malloc.c
index f30fb4b1ba..83d7f661ad 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3296,9 +3296,6 @@  tcache_init(void)
   if (MAX_TCACHE_SIZE >= GLRO (dl_pagesize) / 2)
     malloc_printerr ("max tcache size too large");
 
-  /* Preserve errno when called from free() - _int_malloc may corrupt it.  */
-  int err = errno;
-
   arena_get (ar_ptr, bytes);
   victim = _int_malloc (ar_ptr, bytes);
   if (!victim && ar_ptr != NULL)
@@ -3307,12 +3304,9 @@  tcache_init(void)
       victim = _int_malloc (ar_ptr, bytes);
     }
 
-
   if (ar_ptr != NULL)
     __libc_lock_unlock (ar_ptr->mutex);
 
-  __set_errno (err);
-
   /* In a low memory situation, we may not be able to allocate memory
      - in which case, we just keep trying later.  However, we
      typically do this very early, so either there is sufficient
@@ -3346,13 +3340,15 @@  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;
 }
 
@@ -3684,8 +3680,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.  */
@@ -3702,6 +3696,7 @@  _mid_memalign (size_t alignment, size_t bytes, void *address)
 	    return tag_new_usable (victim);
 	  }
       }
+    MAYBE_INIT_TCACHE ();
   }
 #endif
 
@@ -4556,8 +4551,6 @@  _int_free_chunk (mstate av, mchunkptr p, INTERNAL_SIZE_T size, int have_lock)
 {
   mfastbinptr *fb;             /* associated fastbin */
 
-  MAYBE_INIT_TCACHE ();
-
   /*
     If eligible, place chunk on a fastbin so it can be found
     and used quickly in malloc.