Should use of mallopt depend on DOUG_LEA_MALLOC? (WAS: bug#38345)

Should use of mallopt depend on DOUG_LEA_MALLOC? (WAS: bug#38345)

[Noam Postavsky]

[Moving to emacs-devel, since this doesn't seem to be about the bug as such.]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Noam Postavsky <npostavs@gmail.com>
>> Cc: yantar92@gmail.com,  38345@debbugs.gnu.org,  juri@linkov.net
>> Date: Sat, 07 Dec 2019 14:25:06 -0500
>> 
>> > So does this mean modern glibc has no knobs to tailor its behavior to
>> > various needs, like set the threshold for using mmap, etc.?  IOW, no
>> > more support for the likes of mallopt?
>> 
>> Looks like mallopt is still there, but Emacs' DOUG_LEA_MALLOC code
>> depends on malloc_set_state and malloc_get_state which were removed in
>> glibc 2.25 (2017).
>> https://sourceware.org/ml/libc-alpha/2017-02/msg00079.html
>
> Thanks.  Then I don't understand why our calls to mallopt are
> conditioned on DOUG_LEA_MALLOC.  What they are supposed to do should
> be good for modern glibc versions as well.

It could be just accidental, since DOUG_LEA_MALLOC (i.e.,
malloc_get_state & malloc_set_state) used to always come with mallopt.
Although it looks like most of the mallopt calls are only done if
mmap_lisp_allowed_p returns false.  mmap_lisp_allowed_p is only defined
for DOUG_LEA_MALLOC, so I'm not sure what that condition should evaluate
to if DOUG_LEA_MALLOC is not defined.

I also don't really have any idea what the mallopt calls do (that is, I
can see in the documentation that they modify such and such parameter of
the allocator, but I don't know what that means in practice: faster,
slower, less RAM, more RAM, etc).  So I hope some more clueful people will
chime in.

Re: Should use of mallopt depend on DOUG_LEA_MALLOC? (WAS: bug#38345)

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Date: Fri, 13 Dec 2019 07:12:09 -0500
> 
> >> Looks like mallopt is still there, but Emacs' DOUG_LEA_MALLOC code
> >> depends on malloc_set_state and malloc_get_state which were removed in
> >> glibc 2.25 (2017).
> >> https://sourceware.org/ml/libc-alpha/2017-02/msg00079.html
> >
> > Thanks.  Then I don't understand why our calls to mallopt are
> > conditioned on DOUG_LEA_MALLOC.  What they are supposed to do should
> > be good for modern glibc versions as well.
> 
> It could be just accidental, since DOUG_LEA_MALLOC (i.e.,
> malloc_get_state & malloc_set_state) used to always come with mallopt.
> Although it looks like most of the mallopt calls are only done if
> mmap_lisp_allowed_p returns false.  mmap_lisp_allowed_p is only defined
> for DOUG_LEA_MALLOC, so I'm not sure what that condition should evaluate
> to if DOUG_LEA_MALLOC is not defined.

I think the fact that mmap_lisp_allowed_p is only defined when
DOUG_LEA_MALLOC is just the consequence of its being used under that
condition.  IOW, someone tried to reduce the memory footprint by
#ifdef'ing away functions that aren't use otherwise.  I see nothing in
mmap_lisp_allowed_p that is specific to Doug Lea malloc per se.

> I also don't really have any idea what the mallopt calls do (that is, I
> can see in the documentation that they modify such and such parameter of
> the allocator, but I don't know what that means in practice: faster,
> slower, less RAM, more RAM, etc).

AFAIU:

  . mallopt (M_MMAP_MAX, 0) forces malloc not to use mmap when we
    think it's sub-optimal
  . mallopt (M_TRIM_THRESHOLD, 128 * 1024) makes us use sbrk less often
  . mallopt (M_MMAP_THRESHOLD, 64 * 1024) favors mmap allocations for
    smaller objects/buffers than the default, thus making it easier
    for Emacs to release unused memory to the system
  . mallopt (M_MMAP_MAX, MMAP_MAX_AREAS) tells the library that there
    should be no limit on allocations via mmap, since Emacs is a
    single-threaded application

> So I hope some more clueful people will chime in.

Seconded.

Re: Should use of mallopt depend on DOUG_LEA_MALLOC?

[Stefan Monnier]

>> I also don't really have any idea what the mallopt calls do (that is, I
>> can see in the documentation that they modify such and such parameter of
>> the allocator, but I don't know what that means in practice: faster,
>> slower, less RAM, more RAM, etc).

For those in `init_alloc_once_for_pdumper`, I can't help you.
But the ones conditionalized on `mmap_lisp_allowed_p` are used not as
optimizations but to prevent use of mmap within the malloc
implementation in the two known cases where it can break our
(unportable) assumptions:

- when we use MSB bits for the Lisp_Object tags and hence cannot use all
  of the addressable memory for Lisp objects.
  [ Hopefully, there are no platforms in this category remaining, but
  I don't know this to be the case for sure.  ]

- when we're about to do the unexec dump because the unexec code doesn't
  handle those mmap'd areas.


        Stefan

Re: Should use of mallopt depend on DOUG_LEA_MALLOC?

[Paul Eggert]

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

On 12/13/19 7:22 AM, Stefan Monnier wrote:
> For those in `init_alloc_once_for_pdumper`, I can't help you.

Those mallopt calls shouldn't be needed.

> But the ones conditionalized on `mmap_lisp_allowed_p` are used not as
> optimizations but to prevent use of mmap within the malloc
> implementation in the two known cases where it can break our
> (unportable) assumptions:

Yes, we need to keep those only for the vanishingly small cases where we 
use unexec and a Doug Lea-like malloc.

I looked through Emacs's uses of mallopt. There's a lot of cargo-cult 
cruft there that should go. Plus, I expect there's a real bug with newer 
glibc (so mallopt isn't called) but unexec is still used (because the 
builder asked for it - a mistake in my view) and where Emacs doesn't 
disable mmap properly.

Proposed patch attached. The basic idea is to use "#ifdef M_MMAP_MAX" 
instead of "#ifdef DOUG_LEA_MALLOC" to decide whether to call mallopt 
with M_MMAP_MAX. Plus, stop fiddling with mallopt parameters that are 
problematic given that glibc malloc has mutated so much (and is likely 
to mutate further).

[-- Attachment #2: 0001-Simplify-use-of-mallopt.patch --]
[-- Type: text/x-patch, Size: 6783 bytes --]

From ed60b3ae46e50f3ad214d1e2591d1afa7da651a6 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 13 Dec 2019 13:45:10 -0800
Subject: [PATCH] Simplify use of mallopt

Do not mess with mallopt advice except when necessary.
The advice we were giving was so old it was surely wrong now.
This change also should fix some trivial performance issues.
* src/alloc.c (MMAP_MAX_AREAS): Remove.  All uses changed
to glibc default.
(pointers_fit_in_lispobj_p, mmap_lisp_allowed_p):
Define if M_MMAP_MAX, not if DOUG_LEA_MALLOC.
(maybe_disable_malloc_mmap, reenable_malloc_mmap):
New functions, to simplify callers.
Use glibc default when reenabling.
(lisp_align_malloc, allocate_string_data, allocate_vectorlike):
Use these functions.
(lisp_align_malloc): Fix bug where mmap continued to be disabled
when memory was exhausted, on platforms using unexec.
(allocate_vectorlike): Do not disable and then reenable mmap when
calling allocate_vector_from_block, as it allocates only small blocks.
(init_alloc_once_for_pdumper): Do not alter M_MMAP_THRESHOLD
or M_MMAP_MAX; the defaults are fine.
* src/ralloc.c (r_alloc_init) [DOUGL_LEA_MALLOC]:
Do not alter M_TOP_PAD.
---
 src/alloc.c  | 70 +++++++++++++++++++---------------------------------
 src/ralloc.c | 18 +++-----------
 2 files changed, 28 insertions(+), 60 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index 9fbd0d0573..4e8d7d9721 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -106,11 +106,6 @@ Copyright (C) 1985-1986, 1988, 1993-1995, 1997-2019 Free Software
 
 #ifdef DOUG_LEA_MALLOC
 
-/* Specify maximum number of areas to mmap.  It would be nice to use a
-   value that explicitly means "no limit".  */
-
-# define MMAP_MAX_AREAS 100000000
-
 /* A pointer to the memory allocated that copies that static data
    inside glibc's malloc.  */
 static void *malloc_state_ptr;
@@ -552,7 +547,8 @@ tally_consing (ptrdiff_t nbytes)
   consing_until_gc -= nbytes;
 }
 
-#ifdef DOUG_LEA_MALLOC
+#ifdef M_MMAP_MAX
+
 static bool
 pointers_fit_in_lispobj_p (void)
 {
@@ -569,6 +565,23 @@ mmap_lisp_allowed_p (void)
      regions.  */
   return pointers_fit_in_lispobj_p () && !will_dump_with_unexec_p ();
 }
+
+/* Temporarily disable and then reenable malloc's use of mmap.  */
+static void
+maybe_disable_malloc_mmap (void)
+{
+  if (!mmap_lisp_allowed_p ())
+    mallopt (M_MMAP_MAX, 0);
+}
+static void
+reenable_malloc_mmap (void)
+{
+  if (!mmap_lisp_allowed_p ())
+    mallopt (M_MMAP_MAX, 1<<16 /* glibc default */);
+}
+#else
+static void maybe_disable_malloc_mmap (void) {}
+static void reenable_malloc_mmap (void) {}
 #endif
 
 /* Head of a circularly-linked list of extant finalizers. */
@@ -1133,11 +1146,7 @@ lisp_align_malloc (size_t nbytes, enum mem_type type)
       int i;
       bool aligned;
 
-#ifdef DOUG_LEA_MALLOC
-      if (!mmap_lisp_allowed_p ())
-        mallopt (M_MMAP_MAX, 0);
-#endif
-
+      maybe_disable_malloc_mmap ();
 #ifdef USE_ALIGNED_ALLOC
       verify (ABLOCKS_BYTES % BLOCK_ALIGN == 0);
       abase = base = aligned_alloc (BLOCK_ALIGN, ABLOCKS_BYTES);
@@ -1145,6 +1154,7 @@ lisp_align_malloc (size_t nbytes, enum mem_type type)
       base = malloc (ABLOCKS_BYTES);
       abase = pointer_align (base, BLOCK_ALIGN);
 #endif
+      reenable_malloc_mmap ();
 
       if (base == 0)
 	{
@@ -1156,11 +1166,6 @@ lisp_align_malloc (size_t nbytes, enum mem_type type)
       if (!aligned)
 	((void **) abase)[-1] = base;
 
-#ifdef DOUG_LEA_MALLOC
-      if (!mmap_lisp_allowed_p ())
-          mallopt (M_MMAP_MAX, MMAP_MAX_AREAS);
-#endif
-
 #if ! USE_LSB_TAG
       /* If the memory just allocated cannot be addressed thru a Lisp
 	 object's pointer, and it needs to be, that's equivalent to
@@ -1807,18 +1812,9 @@ allocate_string_data (struct Lisp_String *s,
   if (nbytes > LARGE_STRING_BYTES)
     {
       size_t size = FLEXSIZEOF (struct sblock, data, needed);
-
-#ifdef DOUG_LEA_MALLOC
-      if (!mmap_lisp_allowed_p ())
-        mallopt (M_MMAP_MAX, 0);
-#endif
-
+      maybe_disable_malloc_mmap ();
       b = lisp_malloc (size + GC_STRING_EXTRA, MEM_TYPE_NON_LISP);
-
-#ifdef DOUG_LEA_MALLOC
-      if (!mmap_lisp_allowed_p ())
-        mallopt (M_MMAP_MAX, MMAP_MAX_AREAS);
-#endif
+      reenable_malloc_mmap ();
 
       data = b->data;
       b->next = large_sblocks;
@@ -3145,27 +3141,19 @@ allocate_vectorlike (ptrdiff_t len)
 
   MALLOC_BLOCK_INPUT;
 
-#ifdef DOUG_LEA_MALLOC
-  if (!mmap_lisp_allowed_p ())
-    mallopt (M_MMAP_MAX, 0);
-#endif
-
   if (nbytes <= VBLOCK_BYTES_MAX)
     p = allocate_vector_from_block (vroundup (nbytes));
   else
     {
+      maybe_disable_malloc_mmap ();
       struct large_vector *lv = lisp_malloc (large_vector_offset + nbytes,
 					     MEM_TYPE_VECTORLIKE);
+      reenable_malloc_mmap ();
       lv->next = large_vectors;
       large_vectors = lv;
       p = large_vector_vec (lv);
     }
 
-#ifdef DOUG_LEA_MALLOC
-  if (!mmap_lisp_allowed_p ())
-    mallopt (M_MMAP_MAX, MMAP_MAX_AREAS);
-#endif
-
   if (find_suspicious_object_in_range (p, (char *) p + nbytes))
     emacs_abort ();
 
@@ -7302,14 +7290,6 @@ init_alloc_once_for_pdumper (void)
   purebeg = PUREBEG;
   pure_size = PURESIZE;
   mem_init ();
-
-#ifdef DOUG_LEA_MALLOC
-  mallopt (M_TRIM_THRESHOLD, 128 * 1024); /* Trim threshold.  */
-  mallopt (M_MMAP_THRESHOLD, 64 * 1024);  /* Mmap threshold.  */
-  mallopt (M_MMAP_MAX, MMAP_MAX_AREAS);   /* Max. number of mmap'ed areas.  */
-#endif
-
-
   init_finalizer_list (&finalizers);
   init_finalizer_list (&doomed_finalizers);
   refill_memory_reserve ();
diff --git a/src/ralloc.c b/src/ralloc.c
index 66ea2ec411..857702825b 100644
--- a/src/ralloc.c
+++ b/src/ralloc.c
@@ -1176,23 +1176,11 @@ r_alloc_init (void)
   extra_bytes = PAGE_ROUNDUP (50000);
 #endif
 
-#ifdef DOUG_LEA_MALLOC
-  block_input ();
-  mallopt (M_TOP_PAD, 64 * 4096);
-  unblock_input ();
-#else
-#if !defined SYSTEM_MALLOC && !defined HYBRID_MALLOC
-  /* Give GNU malloc's morecore some hysteresis so that we move all
-     the relocatable blocks much less often.  The number used to be
-     64, but alloc.c would override that with 32 in code that was
-     removed when SYNC_INPUT became the only input handling mode.
-     That code was conditioned on !DOUG_LEA_MALLOC, so the call to
-     mallopt above is left unchanged.  (Actually, I think there's no
-     system nowadays that uses DOUG_LEA_MALLOC and also uses
-     REL_ALLOC.)  */
+#if !defined DOUG_LEA_MALLOC && !defined HYBRID_MALLOC && !defined SYSTEM_MALLOC
+  /* Give gmalloc's morecore some hysteresis so that we move all
+     the relocatable blocks much less often.  */
   __malloc_extra_blocks = 32;
 #endif
-#endif
 
 #if !defined SYSTEM_MALLOC && !defined HYBRID_MALLOC
   first_heap->end = (void *) PAGE_ROUNDUP (first_heap->start);
-- 
2.23.0

Re: Should use of mallopt depend on DOUG_LEA_MALLOC?

[Eli Zaretskii]

> Cc: Noam Postavsky <npostavs@gmail.com>, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 13 Dec 2019 13:55:36 -0800
> 
> On 12/13/19 7:22 AM, Stefan Monnier wrote:
> > For those in `init_alloc_once_for_pdumper`, I can't help you.
> 
> Those mallopt calls shouldn't be needed.

Can you elaborate why?

And if that's indeed true, then what is our response to reports such
as the one in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=38345#71 ?
I thought a judicious use of mallopt would allow us to come close to
jemalloc, but if not, then what would?  Assuming what Ihor Radchenko
sees and is reported in that discussion is normal operation of glibc's
malloc, don't we want to be able to return memory to the OS more
eagerly than the default malloc configuration does?

> Proposed patch attached. The basic idea is to use "#ifdef M_MMAP_MAX" 
> instead of "#ifdef DOUG_LEA_MALLOC" to decide whether to call mallopt 
> with M_MMAP_MAX. Plus, stop fiddling with mallopt parameters that are 
> problematic given that glibc malloc has mutated so much (and is likely 
> to mutate further).

Thanks, but let's postpone such cleanups until after the release
branch is cut (hopefully soon).  I would like to leave the
unexec-related code in its current form for Emacs 27.1, and only fix
any bugs that will be reported against it.