diff mbox

[COMMITTED] malloc/malloc.c : Remove nested function mi_arena.

Message ID 5383536D.5010601@redhat.com
State New
Headers show

Commit Message

Carlos O'Donell May 26, 2014, 2:45 p.m. UTC 
I've committed the following patch to mechanically remove
the nested function mi_arena. This is a very simple
refactoring that's easy to review and prove. Which is what
we want from any refactoring.

We do this because it's harder to debug nested functions,
and it allows non-GNU compilers to compile this code, and
this code is not performance critical.

The resulting code is ~100 bytes larger because the compiler
has to handle the function arguments and pointers to size_t
objects. This code, and in particular calling malloc_info, is
not considered in the hot path of average applications.

I've added `inline' to the function to indicate to the compiler
that we'd like it inlined if beneficial.

Tested on x86_64 with no regressions.

Tested before and after with simple malloc_info application
that prints stats to verify statistics look sane. Test passes.

Red Hat in particular is looking to extend and change this
interface to support JSON and provide better statistics,
therefore we do what is minimally required here to remove the
nested function, as opposed to an inlined version as suggested
by Ondrej Bilka.

Cheers,
Carlos.

2014-05-26  Carlos O'Donell  <carlos@redhat.com>

	* malloc/malloc.c (mi_arena): New function.
	(malloc_info): Remove nested function mi_arena. Call non-nested
	function mi_arena.

---

Cheers,
Carlos.

Comments

Ludovic Courtès May 26, 2014, 7:21 p.m. UTC | #1 
"Carlos O'Donell" <carlos@redhat.com> skribis:

> We do this because it's harder to debug nested functions,
> and it allows non-GNU compilers to compile this code, and
> this code is not performance critical.
>
> The resulting code is ~100 bytes larger because the compiler
> has to handle the function arguments and pointers to size_t
> objects. This code, and in particular calling malloc_info, is
> not considered in the hot path of average applications.

Seems to me like it meets the criteria for rejection that Roland
suggested:

  http://sourceware.org/ml/libc-alpha/2014-05/msg00707.html

> +static inline void
> +mi_arena (mstate ar_ptr,
> +	  FILE *fp,
> +	  size_t *total_nblocks,
> +	  size_t *total_nfastblocks,
> +	  size_t *total_avail,
> +	  size_t *total_fastavail,
> +	  size_t *total_system,
> +	  size_t *total_max_system,
> +	  size_t *total_aspace,
> +	  size_t *total_aspace_mprotect)

[...]

> -      mi_arena (ar_ptr);
> +      mi_arena (ar_ptr, fp, &total_nblocks, &total_nfastblocks, &total_avail,
> +		&total_fastavail, &total_system, &total_max_system,
> +		&total_aspace, &total_aspace_mprotect);
>        ar_ptr = ar_ptr->next;
>      }

Anyone claiming that this improves readability?

Ludo’.
Rich Felker May 28, 2014, 3:15 a.m. UTC | #2 
On Mon, May 26, 2014 at 09:21:47PM +0200, Ludovic Courtès wrote:
> "Carlos O'Donell" <carlos@redhat.com> skribis:
> 
> > We do this because it's harder to debug nested functions,
> > and it allows non-GNU compilers to compile this code, and
> > this code is not performance critical.
> >
> > The resulting code is ~100 bytes larger because the compiler
> > has to handle the function arguments and pointers to size_t
> > objects. This code, and in particular calling malloc_info, is
> > not considered in the hot path of average applications.
> 
> Seems to me like it meets the criteria for rejection that Roland
> suggested:
> 
>   http://sourceware.org/ml/libc-alpha/2014-05/msg00707.html
> 
> > +static inline void
> > +mi_arena (mstate ar_ptr,
> > +	  FILE *fp,
> > +	  size_t *total_nblocks,
> > +	  size_t *total_nfastblocks,
> > +	  size_t *total_avail,
> > +	  size_t *total_fastavail,
> > +	  size_t *total_system,
> > +	  size_t *total_max_system,
> > +	  size_t *total_aspace,
> > +	  size_t *total_aspace_mprotect)

Despite my being in favor of eliminating nested functions, I'm in
complete agreement. This kind of mechanical transformation with
pointers to all of the outer function's locals is utterly hideous to
read, and makes the compiler's work much more difficult.

I'm in favor of the earlier proposal to inline the code directly.

Rich
Carlos O'Donell May 29, 2014, 1:42 a.m. UTC | #3 
On 05/27/2014 11:15 PM, Rich Felker wrote:
> Despite my being in favor of eliminating nested functions, I'm in
> complete agreement. This kind of mechanical transformation with
> pointers to all of the outer function's locals is utterly hideous to
> read, and makes the compiler's work much more difficult.
> 
> I'm in favor of the earlier proposal to inline the code directly.

That's 4 in favour after review.

Ondrej, Please feel free to commit your inlining of the function.

Cheers,
Carlos.
Carlos O'Donell May 29, 2014, 1:51 a.m. UTC | #4 
On 05/26/2014 03:21 PM, Ludovic Courtès wrote:
> "Carlos O'Donell" <carlos@redhat.com> skribis:
> 
>> We do this because it's harder to debug nested functions,
>> and it allows non-GNU compilers to compile this code, and
>> this code is not performance critical.
>>
>> The resulting code is ~100 bytes larger because the compiler
>> has to handle the function arguments and pointers to size_t
>> objects. This code, and in particular calling malloc_info, is
>> not considered in the hot path of average applications.
> 
> Seems to me like it meets the criteria for rejection that Roland
> suggested:
> 
>   http://sourceware.org/ml/libc-alpha/2014-05/msg00707.html

The removal of nested functions is an orthogonal discussion that
has been going on since February (before the clang discussion).

The patch improves debugging this code. The code is not on the
fast path, and it's not terrible to read.

Let me point at the bugs again:

Bug 8300 - no local symbol information within nested or nesting procedures
https://sourceware.org/bugzilla/show_bug.cgi?id=8300

Bug 53927 - wrong value for DW_AT_static_link 
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53927

The point is moot though consensus is to inline the function.
I've ACK'd Ondrej's patch to inline it.

I'm happy as long as there is consensus for a solution.

Cheers,
Carlos.
Carlos O'Donell May 29, 2014, 2:14 a.m. UTC | #5 
On 05/28/2014 09:42 PM, Carlos O'Donell wrote:
> On 05/27/2014 11:15 PM, Rich Felker wrote:
>> Despite my being in favor of eliminating nested functions, I'm in
>> complete agreement. This kind of mechanical transformation with
>> pointers to all of the outer function's locals is utterly hideous to
>> read, and makes the compiler's work much more difficult.
>>
>> I'm in favor of the earlier proposal to inline the code directly.
> 
> That's 4 in favour after review.
> 
> Ondrej, Please feel free to commit your inlining of the function.

I also apologize for not looking closely at the function
in question, and being ideological about trying to keep
the functions as simple as possible. The truth is that as
Rich says, inlining was the better solution.

After I refactored the function I realized it wasn't all
that complex, but by then I'd read the function over a
few times already. I see why Siddhesh also thought inlining
it was the right solution.

Cheers,
Carlos.
Ondřej Bílka May 30, 2014, 11:38 a.m. UTC | #6 
On Wed, May 28, 2014 at 09:42:22PM -0400, Carlos O'Donell wrote:
> On 05/27/2014 11:15 PM, Rich Felker wrote:
> > Despite my being in favor of eliminating nested functions, I'm in
> > complete agreement. This kind of mechanical transformation with
> > pointers to all of the outer function's locals is utterly hideous to
> > read, and makes the compiler's work much more difficult.
> > 
> > I'm in favor of the earlier proposal to inline the code directly.
> 
> That's 4 in favour after review.
> 
> Ondrej, Please feel free to commit your inlining of the function.
> 
I applied it, thanks.
diff mbox

Patch

diff --git a/malloc/malloc.c b/malloc/malloc.c
index 1120d4d..92ad9f9 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -4986,154 +4986,162 @@  __posix_memalign (void **memptr, size_t alignment, size_t size)
 }
 weak_alias (__posix_memalign, posix_memalign)
 
-
-int
-malloc_info (int options, FILE *fp)
+static inline void
+mi_arena (mstate ar_ptr,
+	  FILE *fp,
+	  size_t *total_nblocks,
+	  size_t *total_nfastblocks,
+	  size_t *total_avail,
+	  size_t *total_fastavail,
+	  size_t *total_system,
+	  size_t *total_max_system,
+	  size_t *total_aspace,
+	  size_t *total_aspace_mprotect)
 {
-  /* For now, at least.  */
-  if (options != 0)
-    return EINVAL;
-
   int n = 0;
-  size_t total_nblocks = 0;
-  size_t total_nfastblocks = 0;
-  size_t total_avail = 0;
-  size_t total_fastavail = 0;
-  size_t total_system = 0;
-  size_t total_max_system = 0;
-  size_t total_aspace = 0;
-  size_t total_aspace_mprotect = 0;
 
-  void
-  mi_arena (mstate ar_ptr)
-  {
-    fprintf (fp, "<heap nr=\"%d\">\n<sizes>\n", n++);
+  fprintf (fp, "<heap nr=\"%d\">\n<sizes>\n", n++);
 
-    size_t nblocks = 0;
-    size_t nfastblocks = 0;
-    size_t avail = 0;
-    size_t fastavail = 0;
-    struct
-    {
-      size_t from;
-      size_t to;
-      size_t total;
-      size_t count;
-    } sizes[NFASTBINS + NBINS - 1];
+  size_t nblocks = 0;
+  size_t nfastblocks = 0;
+  size_t avail = 0;
+  size_t fastavail = 0;
+  struct
+  {
+    size_t from;
+    size_t to;
+    size_t total;
+    size_t count;
+  } sizes[NFASTBINS + NBINS - 1];
 #define nsizes (sizeof (sizes) / sizeof (sizes[0]))
 
-    mutex_lock (&ar_ptr->mutex);
-
-    for (size_t i = 0; i < NFASTBINS; ++i)
-      {
-        mchunkptr p = fastbin (ar_ptr, i);
-        if (p != NULL)
-          {
-            size_t nthissize = 0;
-            size_t thissize = chunksize (p);
+  mutex_lock (&ar_ptr->mutex);
+  for (size_t i = 0; i < NFASTBINS; ++i)
+    {
+      mchunkptr p = fastbin (ar_ptr, i);
+      if (p != NULL)
+        {
+          size_t nthissize = 0;
+          size_t thissize = chunksize (p);
 
-            while (p != NULL)
-              {
-                ++nthissize;
-                p = p->fd;
-              }
+          while (p != NULL)
+            {
+              ++nthissize;
+              p = p->fd;
+            }
 
-            fastavail += nthissize * thissize;
-            nfastblocks += nthissize;
-            sizes[i].from = thissize - (MALLOC_ALIGNMENT - 1);
-            sizes[i].to = thissize;
-            sizes[i].count = nthissize;
-          }
-        else
-          sizes[i].from = sizes[i].to = sizes[i].count = 0;
+          fastavail += nthissize * thissize;
+          nfastblocks += nthissize;
+          sizes[i].from = thissize - (MALLOC_ALIGNMENT - 1);
+          sizes[i].to = thissize;
+          sizes[i].count = nthissize;
+        }
+      else
+        sizes[i].from = sizes[i].to = sizes[i].count = 0;
 
-        sizes[i].total = sizes[i].count * sizes[i].to;
-      }
+      sizes[i].total = sizes[i].count * sizes[i].to;
+    }
 
 
-    mbinptr bin;
-    struct malloc_chunk *r;
+  mbinptr bin;
+  struct malloc_chunk *r;
 
-    for (size_t i = 1; i < NBINS; ++i)
-      {
-        bin = bin_at (ar_ptr, i);
-        r = bin->fd;
-        sizes[NFASTBINS - 1 + i].from = ~((size_t) 0);
-        sizes[NFASTBINS - 1 + i].to = sizes[NFASTBINS - 1 + i].total
-                                        = sizes[NFASTBINS - 1 + i].count = 0;
-
-        if (r != NULL)
-          while (r != bin)
-            {
-              ++sizes[NFASTBINS - 1 + i].count;
-              sizes[NFASTBINS - 1 + i].total += r->size;
-              sizes[NFASTBINS - 1 + i].from
-                = MIN (sizes[NFASTBINS - 1 + i].from, r->size);
-              sizes[NFASTBINS - 1 + i].to = MAX (sizes[NFASTBINS - 1 + i].to,
-                                                 r->size);
-
-              r = r->fd;
-            }
+  for (size_t i = 1; i < NBINS; ++i)
+    {
+      bin = bin_at (ar_ptr, i);
+      r = bin->fd;
+      sizes[NFASTBINS - 1 + i].from = ~((size_t) 0);
+      sizes[NFASTBINS - 1 + i].to = sizes[NFASTBINS - 1 + i].total
+                                    = sizes[NFASTBINS - 1 + i].count = 0;
+
+      if (r != NULL)
+        while (r != bin)
+          {
+            ++sizes[NFASTBINS - 1 + i].count;
+            sizes[NFASTBINS - 1 + i].total += r->size;
+            sizes[NFASTBINS - 1 + i].from
+              = MIN (sizes[NFASTBINS - 1 + i].from, r->size);
+            sizes[NFASTBINS - 1 + i].to = MAX (sizes[NFASTBINS - 1 + i].to,
+                                               r->size);
+
+            r = r->fd;
+          }
 
-        if (sizes[NFASTBINS - 1 + i].count == 0)
-          sizes[NFASTBINS - 1 + i].from = 0;
-        nblocks += sizes[NFASTBINS - 1 + i].count;
-        avail += sizes[NFASTBINS - 1 + i].total;
-      }
+      if (sizes[NFASTBINS - 1 + i].count == 0)
+        sizes[NFASTBINS - 1 + i].from = 0;
+      nblocks += sizes[NFASTBINS - 1 + i].count;
+      avail += sizes[NFASTBINS - 1 + i].total;
+    }
 
-    mutex_unlock (&ar_ptr->mutex);
+  mutex_unlock (&ar_ptr->mutex);
 
-    total_nfastblocks += nfastblocks;
-    total_fastavail += fastavail;
+  *total_nfastblocks += nfastblocks;
+  *total_fastavail += fastavail;
 
-    total_nblocks += nblocks;
-    total_avail += avail;
+  *total_nblocks += nblocks;
+  *total_avail += avail;
 
-    for (size_t i = 0; i < nsizes; ++i)
-      if (sizes[i].count != 0 && i != NFASTBINS)
-        fprintf (fp, "							      \
+  for (size_t i = 0; i < nsizes; ++i)
+    if (sizes[i].count != 0 && i != NFASTBINS)
+      fprintf (fp, "							      \
 <size from=\"%zu\" to=\"%zu\" total=\"%zu\" count=\"%zu\"/>\n",
-                 sizes[i].from, sizes[i].to, sizes[i].total, sizes[i].count);
+               sizes[i].from, sizes[i].to, sizes[i].total, sizes[i].count);
 
-    if (sizes[NFASTBINS].count != 0)
-      fprintf (fp, "\
+  if (sizes[NFASTBINS].count != 0)
+    fprintf (fp, "\
 <unsorted from=\"%zu\" to=\"%zu\" total=\"%zu\" count=\"%zu\"/>\n",
-               sizes[NFASTBINS].from, sizes[NFASTBINS].to,
-               sizes[NFASTBINS].total, sizes[NFASTBINS].count);
+             sizes[NFASTBINS].from, sizes[NFASTBINS].to,
+             sizes[NFASTBINS].total, sizes[NFASTBINS].count);
 
-    total_system += ar_ptr->system_mem;
-    total_max_system += ar_ptr->max_system_mem;
+  *total_system += ar_ptr->system_mem;
+  *total_max_system += ar_ptr->max_system_mem;
 
-    fprintf (fp,
-             "</sizes>\n<total type=\"fast\" count=\"%zu\" size=\"%zu\"/>\n"
-             "<total type=\"rest\" count=\"%zu\" size=\"%zu\"/>\n"
-             "<system type=\"current\" size=\"%zu\"/>\n"
-             "<system type=\"max\" size=\"%zu\"/>\n",
-             nfastblocks, fastavail, nblocks, avail,
-             ar_ptr->system_mem, ar_ptr->max_system_mem);
+  fprintf (fp,
+           "</sizes>\n<total type=\"fast\" count=\"%zu\" size=\"%zu\"/>\n"
+           "<total type=\"rest\" count=\"%zu\" size=\"%zu\"/>\n"
+           "<system type=\"current\" size=\"%zu\"/>\n"
+           "<system type=\"max\" size=\"%zu\"/>\n",
+           nfastblocks, fastavail, nblocks, avail,
+           ar_ptr->system_mem, ar_ptr->max_system_mem);
 
-    if (ar_ptr != &main_arena)
-      {
-        heap_info *heap = heap_for_ptr (top (ar_ptr));
-        fprintf (fp,
-                 "<aspace type=\"total\" size=\"%zu\"/>\n"
-                 "<aspace type=\"mprotect\" size=\"%zu\"/>\n",
-                 heap->size, heap->mprotect_size);
-        total_aspace += heap->size;
-        total_aspace_mprotect += heap->mprotect_size;
-      }
-    else
-      {
-        fprintf (fp,
-                 "<aspace type=\"total\" size=\"%zu\"/>\n"
-                 "<aspace type=\"mprotect\" size=\"%zu\"/>\n",
-                 ar_ptr->system_mem, ar_ptr->system_mem);
-        total_aspace += ar_ptr->system_mem;
-        total_aspace_mprotect += ar_ptr->system_mem;
-      }
+  if (ar_ptr != &main_arena)
+    {
+      heap_info *heap = heap_for_ptr (top (ar_ptr));
+      fprintf (fp,
+               "<aspace type=\"total\" size=\"%zu\"/>\n"
+               "<aspace type=\"mprotect\" size=\"%zu\"/>\n",
+               heap->size, heap->mprotect_size);
+      *total_aspace += heap->size;
+      *total_aspace_mprotect += heap->mprotect_size;
+    }
+  else
+    {
+      fprintf (fp,
+               "<aspace type=\"total\" size=\"%zu\"/>\n"
+               "<aspace type=\"mprotect\" size=\"%zu\"/>\n",
+               ar_ptr->system_mem, ar_ptr->system_mem);
+      *total_aspace += ar_ptr->system_mem;
+      *total_aspace_mprotect += ar_ptr->system_mem;
+    }
 
-    fputs ("</heap>\n", fp);
-  }
+  fputs ("</heap>\n", fp);
+}
+
+int
+malloc_info (int options, FILE *fp)
+{
+  /* For now, at least.  */
+  if (options != 0)
+    return EINVAL;
+
+  size_t total_nblocks = 0;
+  size_t total_nfastblocks = 0;
+  size_t total_avail = 0;
+  size_t total_fastavail = 0;
+  size_t total_system = 0;
+  size_t total_max_system = 0;
+  size_t total_aspace = 0;
+  size_t total_aspace_mprotect = 0;
 
   if (__malloc_initialized < 0)
     ptmalloc_init ();
@@ -5144,7 +5152,9 @@  malloc_info (int options, FILE *fp)
   mstate ar_ptr = &main_arena;
   do
     {
-      mi_arena (ar_ptr);
+      mi_arena (ar_ptr, fp, &total_nblocks, &total_nfastblocks, &total_avail,
+		&total_fastavail, &total_system, &total_max_system,
+		&total_aspace, &total_aspace_mprotect);
       ar_ptr = ar_ptr->next;
     }
   while (ar_ptr != &main_arena);