malloc: Trim unused arenas on thread exit

malloc: Trim unused arenas on thread exit

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 376 bytes --]

I tried the attached patch to trim unused arenas on thread exit.  The 
trimming actually happens (the heap consolidation is visible in the 
malloc_info output from tst-malloc_info), but the arena heaps aren't 
deallocated.

I think trimming unused arenas as much as possible is a good heuristics 
to minimize RSS, so getting this to work might be worthwhile.

Thanks,
Florian

[-- Attachment #2: mtrim.patch --]
[-- Type: text/x-patch, Size: 1273 bytes --]

diff --git a/malloc/arena.c b/malloc/arena.c
index 85b985e193..758226c222 100644
--- a/malloc/arena.c
+++ b/malloc/arena.c
@@ -953,12 +953,22 @@ arena_thread_freeres (void)
       /* If this was the last attached thread for this arena, put the
 	 arena on the free list.  */
       assert (a->attached_threads > 0);
-      if (--a->attached_threads == 0)
+      bool arena_is_unused = --a->attached_threads == 0;
+      if (arena_is_unused)
 	{
 	  a->next_free = free_list;
 	  free_list = a;
 	}
       __libc_lock_unlock (free_list_lock);
+
+      /* If there are no more users, compact the arena as much as
+	 possible.  */
+      if (arena_is_unused)
+	{
+	  __libc_lock_lock (a->mutex);
+	  mtrim (a, 0);
+	  __libc_lock_unlock (a->mutex);
+	}
     }
 }
 text_set_element (__libc_thread_subfreeres, arena_thread_freeres);
diff --git a/malloc/malloc.c b/malloc/malloc.c
index 1f003d2ef0..a0b11784d2 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -1831,7 +1831,7 @@ malloc_init_state (mstate av)
 static void *sysmalloc (INTERNAL_SIZE_T, mstate);
 static int      systrim (size_t, mstate);
 static void     malloc_consolidate (mstate);
-
+static int mtrim (mstate av, size_t pad);
 
 /* -------------- Early definitions for debugging hooks ---------------- */
 

Re: malloc: Trim unused arenas on thread exit

[Siddhesh Poyarekar]

On Thursday 09 November 2017 04:25 PM, Florian Weimer wrote:
> I tried the attached patch to trim unused arenas on thread exit.  The
> trimming actually happens (the heap consolidation is visible in the
> malloc_info output from tst-malloc_info), but the arena heaps aren't
> deallocated.
> 
> I think trimming unused arenas as much as possible is a good heuristics
> to minimize RSS, so getting this to work might be worthwhile.

I wonder if the overhead of unmapping the arena heaps is worthwhile.
For cases where it matters (overcommit is a ratio or is disabled or for
__libc_enable_secure), trimming of the heaps should reduce the commit
charge already since we do remap the pages as PROT_NONE.

The patch seems OK, I'm just wondering if the additional work is worth
the effort because it will also hurt applications that spawn threads
frequently and have similar resource usage; they'll miss out on the
caching effect.

Siddhesh

> Thanks,
> Florian
> 
> mtrim.patch
> 
> 
> diff --git a/malloc/arena.c b/malloc/arena.c
> index 85b985e193..758226c222 100644
> --- a/malloc/arena.c
> +++ b/malloc/arena.c
> @@ -953,12 +953,22 @@ arena_thread_freeres (void)
>        /* If this was the last attached thread for this arena, put the
>  	 arena on the free list.  */
>        assert (a->attached_threads > 0);
> -      if (--a->attached_threads == 0)
> +      bool arena_is_unused = --a->attached_threads == 0;
> +      if (arena_is_unused)
>  	{
>  	  a->next_free = free_list;
>  	  free_list = a;
>  	}
>        __libc_lock_unlock (free_list_lock);
> +
> +      /* If there are no more users, compact the arena as much as
> +	 possible.  */
> +      if (arena_is_unused)
> +	{
> +	  __libc_lock_lock (a->mutex);
> +	  mtrim (a, 0);
> +	  __libc_lock_unlock (a->mutex);
> +	}
>      }
>  }
>  text_set_element (__libc_thread_subfreeres, arena_thread_freeres);
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 1f003d2ef0..a0b11784d2 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -1831,7 +1831,7 @@ malloc_init_state (mstate av)
>  static void *sysmalloc (INTERNAL_SIZE_T, mstate);
>  static int      systrim (size_t, mstate);
>  static void     malloc_consolidate (mstate);
> -
> +static int mtrim (mstate av, size_t pad);
>  
>  /* -------------- Early definitions for debugging hooks ---------------- */
>  
> 

Re: malloc: Trim unused arenas on thread exit

[Florian Weimer]

On 11/10/2017 11:52 AM, Siddhesh Poyarekar wrote:
> On Thursday 09 November 2017 04:25 PM, Florian Weimer wrote:
>> I tried the attached patch to trim unused arenas on thread exit.  The
>> trimming actually happens (the heap consolidation is visible in the
>> malloc_info output from tst-malloc_info), but the arena heaps aren't
>> deallocated.
>>
>> I think trimming unused arenas as much as possible is a good heuristics
>> to minimize RSS, so getting this to work might be worthwhile.
> 
> I wonder if the overhead of unmapping the arena heaps is worthwhile.
> For cases where it matters (overcommit is a ratio or is disabled or for
> __libc_enable_secure), trimming of the heaps should reduce the commit
> charge already since we do remap the pages as PROT_NONE.

I would have expected that the test makes the second sub-heap completely 
unused, so that the existing logic would unmap it.  But that does not 
seem to happen.

> The patch seems OK, I'm just wondering if the additional work is worth
> the effort because it will also hurt applications that spawn threads
> frequently and have similar resource usage; they'll miss out on the
> caching effect.

We could do this not for the current arena, but he arena at the op of 
the free list.  Then we'd return some caching, and you'd get a ping-pong 
effect only if you stop and start more than one thread.

The consolidation we should still perform on the current arena, maybe 
even if it is not unused.  I assume that before the thread exits, it 
deallocates some resources, and we should make sure that malloc sees 
them once the arena is reused, even from a thread which has a vastly 
different allocation pattern.

Thanks,
Florian

Re: malloc: Trim unused arenas on thread exit

[DJ Delorie]

Florian Weimer <fweimer@redhat.com> writes:
> I think trimming unused arenas as much as possible is a good heuristics 
> to minimize RSS, so getting this to work might be worthwhile.

I ran this against many of my real-world workloads and did not see a
significant (i.e. obvious) reduction in actual RSS.  Some tests showed a
minor (i.e. untrustworthy, I didn't run enough passes to reduce the
noise below any measured change) *increase* in RSS, as well as an
expected (but still untrustworthy) increase in cycles required.

Now that we have the tools to *measure* what our malloc changes cause, I
think it's prudent that we start asking for benchmarks (and workloads)
that back up any proposed performance/rss change.

Re: malloc: Trim unused arenas on thread exit

[Florian Weimer]

On 11/13/2017 10:57 PM, DJ Delorie wrote:
> Florian Weimer <fweimer@redhat.com> writes:
>> I think trimming unused arenas as much as possible is a good heuristics
>> to minimize RSS, so getting this to work might be worthwhile.
> 
> I ran this against many of my real-world workloads and did not see a
> significant (i.e. obvious) reduction in actual RSS.  Some tests showed a
> minor (i.e. untrustworthy, I didn't run enough passes to reduce the
> noise below any measured change) *increase* in RSS, as well as an
> expected (but still untrustworthy) increase in cycles required.

Sorry, as I tried to say in my original message: It does not actually 
reuse RSS because the sub-heaps are not deallocated.  I don't know why 
this does not happen.

The trimming really should be a win because the workload is likely to 
change when a thread exits.

Thanks,
Florian

Re: malloc: Trim unused arenas on thread exit

[Siddhesh Poyarekar]

On Tuesday 14 November 2017 12:38 PM, Florian Weimer wrote:
> The trimming really should be a win because the workload is likely to
> change when a thread exits.

Not really, worker threads will often have similar workload
characteristics, e.g. web servers, middleware messaging servers.

Siddhesh

Re: malloc: Trim unused arenas on thread exit

[Florian Weimer]

On 11/14/2017 08:12 AM, Siddhesh Poyarekar wrote:
> On Tuesday 14 November 2017 12:38 PM, Florian Weimer wrote:
>> The trimming really should be a win because the workload is likely to
>> change when a thread exits.
> 
> Not really, worker threads will often have similar workload
> characteristics, e.g. web servers, middleware messaging servers.

But in these cases, the application will generally use a thread pool, so 
glibc will not see the thread exit anyway.

Florian

Re: malloc: Trim unused arenas on thread exit

[Siddhesh Poyarekar]

On Tuesday 14 November 2017 12:43 PM, Florian Weimer wrote:
> But in these cases, the application will generally use a thread pool, so
> glibc will not see the thread exit anyway.

We kinda emulate thread pools with our thread stack cache and arena
cache; the only thing we don't cache is the actual threads.  If your
premise is that it is not important enough then the premise will likely
extend to the utility of this optimization.

Siddhesh

Re: malloc: Trim unused arenas on thread exit

[Florian Weimer]

On 11/14/2017 08:20 AM, Siddhesh Poyarekar wrote:
> On Tuesday 14 November 2017 12:43 PM, Florian Weimer wrote:
>> But in these cases, the application will generally use a thread pool, so
>> glibc will not see the thread exit anyway.
> 
> We kinda emulate thread pools with our thread stack cache and arena
> cache; the only thing we don't cache is the actual threads.  If your
> premise is that it is not important enough then the premise will likely
> extend to the utility of this optimization.

Well, I just double-checked, and the optimization completely avoids 
unmap on thread exit, so it is likely still a win.  Unmapping memory can 
be ridiculously expensive, so I can see the argument that we should not 
do that on thread exit.

Maybe I misspoke further up the thread.  What I wanted to suggest was 
consolidation for the current arena on thread exit, and trimming for the 
arena at the start of the free list.  My patch did not implement that, 
but a more aggressive scheme.  It still did not have an impact on RSS, 
which might be a bug somewhere in the allocator.

Thanks,
Florian

Re: malloc: Trim unused arenas on thread exit

[Florian Weimer]

On 11/14/2017 01:12 PM, Florian Weimer wrote:
> On 11/14/2017 08:20 AM, Siddhesh Poyarekar wrote:
>> On Tuesday 14 November 2017 12:43 PM, Florian Weimer wrote:
>>> But in these cases, the application will generally use a thread pool, so
>>> glibc will not see the thread exit anyway.
>>
>> We kinda emulate thread pools with our thread stack cache and arena
>> cache; the only thing we don't cache is the actual threads.  If your
>> premise is that it is not important enough then the premise will likely
>> extend to the utility of this optimization.
> 
> Well, I just double-checked, and the optimization completely avoids 
> unmap on thread exit, so it is likely still a win.  Unmapping memory can 
> be ridiculously expensive, so I can see the argument that we should not 
> do that on thread exit.
> 
> Maybe I misspoke further up the thread.  What I wanted to suggest was 
> consolidation for the current arena on thread exit, and trimming for the 
> arena at the start of the free list.  My patch did not implement that, 
> but a more aggressive scheme.  It still did not have an impact on RSS, 
> which might be a bug somewhere in the allocator.

It could be related to the ordering between tcache_thread_freeres and 
arena_thread_freeres.  We should merge the two to enforce the correct 
ordering.

Thanks,
Florian

Re: malloc: Trim unused arenas on thread exit

[Siddhesh Poyarekar]

On Wednesday 15 November 2017 04:14 PM, Florian Weimer wrote:
> It could be related to the ordering between tcache_thread_freeres and
> arena_thread_freeres.  We should merge the two to enforce the correct
> ordering.

I agree, it sounds like a better idea to have a single freeres for a
module anyway.

Siddhesh

Re: malloc: Trim unused arenas on thread exit

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 532 bytes --]

On 11/15/2017 12:19 PM, Siddhesh Poyarekar wrote:
> On Wednesday 15 November 2017 04:14 PM, Florian Weimer wrote:
>> It could be related to the ordering between tcache_thread_freeres and
>> arena_thread_freeres.  We should merge the two to enforce the correct
>> ordering.
> 
> I agree, it sounds like a better idea to have a single freeres for a
> module anyway.

Here's a patch which does just that.  Tested with and without 
--disable-experimental-malloc.

It doesn't have any consolidation/trimming changes.

Thanks,
Florian

[-- Attachment #2: malloc-shutdown.patch --]
[-- Type: text/x-patch, Size: 2370 bytes --]

malloc: Use a single thread shutdown hook for tcache and arenas

This modifies the code to make the ordering explicit (tcache first).

2017-11-15  Florian Weimer  <fweimer@redhat.com>

	* malloc/malloc.c (tcache_thread_shutdown): Rename from
	tcache_thread_freeres.  Define for USE_TCACHE and !USE_TCACHE
	alike.  Remove freeres marker.
	* malloc/arena.c (arena_thread_freeres): Call
	tcache_thread_shutdown.

diff --git a/malloc/arena.c b/malloc/arena.c
index 85b985e193..4d27e17c46 100644
--- a/malloc/arena.c
+++ b/malloc/arena.c
@@ -944,6 +944,11 @@ arena_get_retry (mstate ar_ptr, size_t bytes)
 static void __attribute__ ((section ("__libc_thread_freeres_fn")))
 arena_thread_freeres (void)
 {
+  /* Shut down the thread cache first.  This could deallocate data for
+     the thread arena, so do this before we put the arena on the free
+     list.  */
+  tcache_thread_shutdown ();
+
   mstate a = thread_arena;
   thread_arena = NULL;
 
diff --git a/malloc/malloc.c b/malloc/malloc.c
index 2999ac4d2f..79f0e9eac7 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -1869,6 +1869,9 @@ void *weak_variable (*__memalign_hook)
   = memalign_hook_ini;
 void weak_variable (*__after_morecore_hook) (void) = NULL;
 
+/* This function is called from the arena shutdown hook, to free the
+   thread cache (if it exists).  */
+static void tcache_thread_shutdown (void);
 
 /* ------------------ Testing support ----------------------------------*/
 
@@ -2938,8 +2941,8 @@ tcache_get (size_t tc_idx)
   return (void *) e;
 }
 
-static void __attribute__ ((section ("__libc_thread_freeres_fn")))
-tcache_thread_freeres (void)
+static void
+tcache_thread_shutdown (void)
 {
   int i;
   tcache_perthread_struct *tcache_tmp = tcache;
@@ -2965,7 +2968,6 @@ tcache_thread_freeres (void)
 
   __libc_free (tcache_tmp);
 }
-text_set_element (__libc_thread_subfreeres, tcache_thread_freeres);
 
 static void
 tcache_init(void)
@@ -3002,13 +3004,20 @@ tcache_init(void)
 
 }
 
-#define MAYBE_INIT_TCACHE() \
+# define MAYBE_INIT_TCACHE() \
   if (__glibc_unlikely (tcache == NULL)) \
     tcache_init();
 
-#else
-#define MAYBE_INIT_TCACHE()
-#endif
+#else  /* !USE_TCACHE */
+# define MAYBE_INIT_TCACHE()
+
+static void
+tcache_thread_shutdown (void)
+{
+  /* Nothing to do if there is no thread cache.  */
+}
+
+#endif /* !USE_TCACHE  */
 
 void *
 __libc_malloc (size_t bytes)

Re: malloc: Trim unused arenas on thread exit

[Siddhesh Poyarekar]

On Wednesday 15 November 2017 07:06 PM, Florian Weimer wrote:
> Here's a patch which does just that.  Tested with and without
> --disable-experimental-malloc.
> 
> It doesn't have any consolidation/trimming changes.

LGTM, I trust you'll write a nice git commit log to explain the change :)

Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>

> 
> malloc-shutdown.patch
> 
> 
> malloc: Use a single thread shutdown hook for tcache and arenas
> 
> This modifies the code to make the ordering explicit (tcache first).
> 
> 2017-11-15  Florian Weimer  <fweimer@redhat.com>
> 
> 	* malloc/malloc.c (tcache_thread_shutdown): Rename from
> 	tcache_thread_freeres.  Define for USE_TCACHE and !USE_TCACHE
> 	alike.  Remove freeres marker.
> 	* malloc/arena.c (arena_thread_freeres): Call
> 	tcache_thread_shutdown.
> 
> diff --git a/malloc/arena.c b/malloc/arena.c
> index 85b985e193..4d27e17c46 100644
> --- a/malloc/arena.c
> +++ b/malloc/arena.c
> @@ -944,6 +944,11 @@ arena_get_retry (mstate ar_ptr, size_t bytes)
>  static void __attribute__ ((section ("__libc_thread_freeres_fn")))
>  arena_thread_freeres (void)
>  {
> +  /* Shut down the thread cache first.  This could deallocate data for
> +     the thread arena, so do this before we put the arena on the free
> +     list.  */
> +  tcache_thread_shutdown ();
> +
>    mstate a = thread_arena;
>    thread_arena = NULL;
>  
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 2999ac4d2f..79f0e9eac7 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -1869,6 +1869,9 @@ void *weak_variable (*__memalign_hook)
>    = memalign_hook_ini;
>  void weak_variable (*__after_morecore_hook) (void) = NULL;
>  
> +/* This function is called from the arena shutdown hook, to free the
> +   thread cache (if it exists).  */
> +static void tcache_thread_shutdown (void);
>  
>  /* ------------------ Testing support ----------------------------------*/
>  
> @@ -2938,8 +2941,8 @@ tcache_get (size_t tc_idx)
>    return (void *) e;
>  }
>  
> -static void __attribute__ ((section ("__libc_thread_freeres_fn")))
> -tcache_thread_freeres (void)
> +static void
> +tcache_thread_shutdown (void)
>  {
>    int i;
>    tcache_perthread_struct *tcache_tmp = tcache;
> @@ -2965,7 +2968,6 @@ tcache_thread_freeres (void)
>  
>    __libc_free (tcache_tmp);
>  }
> -text_set_element (__libc_thread_subfreeres, tcache_thread_freeres);
>  
>  static void
>  tcache_init(void)
> @@ -3002,13 +3004,20 @@ tcache_init(void)
>  
>  }
>  
> -#define MAYBE_INIT_TCACHE() \
> +# define MAYBE_INIT_TCACHE() \
>    if (__glibc_unlikely (tcache == NULL)) \
>      tcache_init();
>  
> -#else
> -#define MAYBE_INIT_TCACHE()
> -#endif
> +#else  /* !USE_TCACHE */
> +# define MAYBE_INIT_TCACHE()
> +
> +static void
> +tcache_thread_shutdown (void)
> +{
> +  /* Nothing to do if there is no thread cache.  */
> +}
> +
> +#endif /* !USE_TCACHE  */
>  
>  void *
>  __libc_malloc (size_t bytes)
> 

Re: malloc: Trim unused arenas on thread exit

[Florian Weimer]

On 11/15/2017 03:00 PM, Siddhesh Poyarekar wrote:
> On Wednesday 15 November 2017 07:06 PM, Florian Weimer wrote:
>> Here's a patch which does just that.  Tested with and without
>> --disable-experimental-malloc.
>>
>> It doesn't have any consolidation/trimming changes.
> 
> LGTM, I trust you'll write a nice git commit log to explain the change :)

I assumed the comment was sufficient:

>>   static void __attribute__ ((section ("__libc_thread_freeres_fn")))
>>   arena_thread_freeres (void)
>>   {
>> +  /* Shut down the thread cache first.  This could deallocate data for
>> +     the thread arena, so do this before we put the arena on the free
>> +     list.  */
>> +  tcache_thread_shutdown ();

If it is not, we need to put more documentation in the code itself.

Thanks,
Florian

Re: malloc: Trim unused arenas on thread exit

[Siddhesh Poyarekar]

On Wednesday 15 November 2017 07:40 PM, Florian Weimer wrote:
> 
> I assumed the comment was sufficient:
> 
>>>   static void __attribute__ ((section ("__libc_thread_freeres_fn")))
>>>   arena_thread_freeres (void)
>>>   {
>>> +  /* Shut down the thread cache first.  This could deallocate data for
>>> +     the thread arena, so do this before we put the arena on the free
>>> +     list.  */
>>> +  tcache_thread_shutdown ();
> 
> If it is not, we need to put more documentation in the code itself.

The comment describes the current behaviour while the commit log should
describe the change, something like:

"Call tcache destructor in arena_thread_freeres

Having separate cleanup functions for arena and tcache could result in
the tcache freeres function being called later and thus not deallocate
data for the thread arena.  Avoid this by calling the tcache cleanup
function from within arena_thread_freeres."

Siddhesh

Re: malloc: Trim unused arenas on thread exit

[Florian Weimer]

On 11/15/2017 03:17 PM, Siddhesh Poyarekar wrote:
> On Wednesday 15 November 2017 07:40 PM, Florian Weimer wrote:
>>
>> I assumed the comment was sufficient:
>>
>>>>    static void __attribute__ ((section ("__libc_thread_freeres_fn")))
>>>>    arena_thread_freeres (void)
>>>>    {
>>>> +  /* Shut down the thread cache first.  This could deallocate data for
>>>> +     the thread arena, so do this before we put the arena on the free
>>>> +     list.  */
>>>> +  tcache_thread_shutdown ();
>>
>> If it is not, we need to put more documentation in the code itself.
> 
> The comment describes the current behaviour while the commit log should
> describe the change, something like:
> 
> "Call tcache destructor in arena_thread_freeres
> 
> Having separate cleanup functions for arena and tcache could result in
> the tcache freeres function being called later and thus not deallocate
> data for the thread arena.  Avoid this by calling the tcache cleanup
> function from within arena_thread_freeres."

But I have not verified that this actually happens, and we agreed that 
it was a good change nevertheless.

Thanks,
Florian

Re: malloc: Trim unused arenas on thread exit

[Siddhesh Poyarekar]

On Wednesday 15 November 2017 08:05 PM, Florian Weimer wrote:
>> "Call tcache destructor in arena_thread_freeres
>>
>> Having separate cleanup functions for arena and tcache could result in
>> the tcache freeres function being called later and thus not deallocate
>> data for the thread arena.  Avoid this by calling the tcache cleanup
>> function from within arena_thread_freeres."
> 
> But I have not verified that this actually happens, and we agreed that
> it was a good change nevertheless.

Then maybe this is a more accurate description:

"Call tcache destructor in arena_thread_freeres

It does not make sense to register separate cleanup functions for arena
and tcache since they're always going to be called together.  Call the
tcache cleanup function from within arena_thread_freeres since it at
least makes the order of those cleanups clear in the code."

My point is that we should be documenting the change clearly enough in
the commit log that one shouldn't have to read the code to get a high
level understanding of what the change is about.

Siddhesh