Re: [PATCH 1/2] Memory statistics enhancement.

["Martin Liška" <mliska@suse.cz>]

On 06/08/2015 05:01 PM, Martin Liška wrote:
> On 06/01/2015 06:16 PM, mliska wrote:
>> Hi.
>>
>> Following 2 patches improve memory statistics infrastructure. First one
>> ports pool allocator to the new infrastructure. And the second one makes
>> column alignment properly.
>>
>> Both can bootstrap on x86_64-linux-pc and survive regression tests.
>>
>> Ready for trunk?
>> Thank you,
>> Martin
>>
>> Port pool-allocator memory stats to a new infrastructure.
>>
>> gcc/ChangeLog:
>>
>> 2015-06-02  Martin Liska  <mliska@suse.cz>
>>
>> 	* alloc-pool.c (allocate_pool_descriptor): Remove.
>> 	(struct pool_output_info): Likewise.
>> 	(print_alloc_pool_statistics): Likewise.
>> 	(dump_alloc_pool_statistics): Likewise.
>> 	* alloc-pool.h (struct pool_usage): New struct.
>> 	(pool_allocator::initialize): Change usage of memory statistics
>> 	to a new interface.
>> 	(pool_allocator::release): Likewise.
>> 	(pool_allocator::allocate): Likewise.
>> 	(pool_allocator::remove): Likewise.
>> 	* mem-stats-traits.h (enum mem_alloc_origin): Add new enum value
>> 	for a pool allocator.
>> 	* mem-stats.h (struct mem_location): Add new ctor.
>> 	(struct mem_usage): Add counter for number of
>> 	instances.
>> 	(mem_alloc_description::register_descriptor): New overload of
>> 	the function.
>> ---
>>  gcc/alloc-pool.c       |  60 +----------------------------
>>  gcc/alloc-pool.h       | 102 +++++++++++++++++++++++++++++++++++++++----------
>>  gcc/mem-stats-traits.h |   3 +-
>>  gcc/mem-stats.h        |  69 ++++++++++++++++++++++-----------
>>  4 files changed, 132 insertions(+), 102 deletions(-)
>>
>> diff --git a/gcc/alloc-pool.c b/gcc/alloc-pool.c
>> index e9fdc86..601c2b7 100644
>> --- a/gcc/alloc-pool.c
>> +++ b/gcc/alloc-pool.c
>> @@ -26,70 +26,14 @@ along with GCC; see the file COPYING3.  If not see
>>  #include "hash-map.h"
>>  
>>  ALLOC_POOL_ID_TYPE last_id;
>> -
>> -/* Hashtable mapping alloc_pool names to descriptors.  */
>> -hash_map<const char *, alloc_pool_descriptor> *alloc_pool_hash;
>> -
>> -struct alloc_pool_descriptor *
>> -allocate_pool_descriptor (const char *name)
>> -{
>> -  if (!alloc_pool_hash)
>> -    alloc_pool_hash = new hash_map<const char *, alloc_pool_descriptor> (10,
>> -									 false,
>> -									 false);
>> -
>> -  return &alloc_pool_hash->get_or_insert (name);
>> -}
>> -
>> -/* Output per-alloc_pool statistics.  */
>> -
>> -/* Used to accumulate statistics about alloc_pool sizes.  */
>> -struct pool_output_info
>> -{
>> -  unsigned long total_created;
>> -  unsigned long total_allocated;
>> -};
>> -
>> -/* Called via hash_map.traverse.  Output alloc_pool descriptor pointed out by
>> -   SLOT and update statistics.  */
>> -bool
>> -print_alloc_pool_statistics (const char *const &name,
>> -			     const alloc_pool_descriptor &d,
>> -			     struct pool_output_info *i)
>> -{
>> -  if (d.allocated)
>> -    {
>> -      fprintf (stderr,
>> -	       "%-22s %6d %10lu %10lu(%10lu) %10lu(%10lu) %10lu(%10lu)\n",
>> -	       name, d.elt_size, d.created, d.allocated,
>> -	       d.allocated / d.elt_size, d.peak, d.peak / d.elt_size,
>> -	       d.current, d.current / d.elt_size);
>> -      i->total_allocated += d.allocated;
>> -      i->total_created += d.created;
>> -    }
>> -  return 1;
>> -}
>> +mem_alloc_description<pool_usage> pool_allocator_usage;
>>  
>>  /* Output per-alloc_pool memory usage statistics.  */
>>  void
>>  dump_alloc_pool_statistics (void)
>>  {
>> -  struct pool_output_info info;
>> -
>>    if (! GATHER_STATISTICS)
>>      return;
>>  
>> -  if (!alloc_pool_hash)
>> -    return;
>> -
>> -  fprintf (stderr, "\nAlloc-pool Kind         Elt size  Pools  Allocated (elts)            Peak (elts)            Leak (elts)\n");
>> -  fprintf (stderr, "--------------------------------------------------------------------------------------------------------------\n");
>> -  info.total_created = 0;
>> -  info.total_allocated = 0;
>> -  alloc_pool_hash->traverse <struct pool_output_info *,
>> -			     print_alloc_pool_statistics> (&info);
>> -  fprintf (stderr, "--------------------------------------------------------------------------------------------------------------\n");
>> -  fprintf (stderr, "%-22s           %7lu %10lu\n",
>> -	   "Total", info.total_created, info.total_allocated);
>> -  fprintf (stderr, "--------------------------------------------------------------------------------------------------------------\n");
>> +  pool_allocator_usage.dump (ALLOC_POOL);
>>  }
>> diff --git a/gcc/alloc-pool.h b/gcc/alloc-pool.h
>> index 96a1342..a1727ce 100644
>> --- a/gcc/alloc-pool.h
>> +++ b/gcc/alloc-pool.h
>> @@ -26,6 +26,71 @@ extern void dump_alloc_pool_statistics (void);
>>  
>>  typedef unsigned long ALLOC_POOL_ID_TYPE;
>>  
>> +/* Pool allocator memory usage.  */
>> +struct pool_usage: public mem_usage
>> +{
>> +  /* Default contructor.  */
>> +  pool_usage (): m_element_size (0), m_pool_name ("") {}
>> +  /* Constructor.  */
>> +  pool_usage (size_t allocated, size_t times, size_t peak,
>> +	      size_t instances, size_t element_size,
>> +	      const char *pool_name)
>> +    : mem_usage (allocated, times, peak, instances),
>> +      m_element_size (element_size),
>> +      m_pool_name (pool_name) {}
>> +
>> +  /* Sum the usage with SECOND usage.  */
>> +  pool_usage operator+ (const pool_usage &second)
>> +  {
>> +    return pool_usage (m_allocated + second.m_allocated,
>> +			     m_times + second.m_times,
>> +			     m_peak + second.m_peak,
>> +			     m_instances + second.m_instances,
>> +			     m_element_size, m_pool_name);
>> +  }
>> +
>> +  /* Dump usage coupled to LOC location, where TOTAL is sum of all rows.  */
>> +  inline void dump (mem_location *loc, mem_usage &total) const
>> +  {
>> +    char s[4096];
>> +    sprintf (s, "%s:%i (%s)", loc->get_trimmed_filename (),
>> +	     loc->m_line, loc->m_function);
>> +
>> +    s[48] = '\0';
>> +
>> +    fprintf (stderr, "%-32s%-48s %6li%10li:%5.1f%%%10li%10li:%5.1f%%%12li\n",
>> +	     m_pool_name, s, (long)m_instances,
>> +	     (long)m_allocated, get_percent (m_allocated, total.m_allocated),
>> +	     (long)m_peak, (long)m_times,
>> +	     get_percent (m_times, total.m_times),
>> +	     (long)m_element_size);
>> +  }
>> +
>> +  /* Dump header with NAME.  */
>> +  static inline void dump_header (const char *name)
>> +  {
>> +    fprintf (stderr, "%-32s%-48s %6s%11s%16s%17s%12s\n", "Pool name", name,
>> +	     "Pools", "Leak", "Peak", "Times", "Elt size");
>> +    print_dash_line ();
>> +  }
>> +
>> +  /* Dump footer.  */
>> +  inline void dump_footer ()
>> +  {
>> +    print_dash_line ();
>> +    fprintf (stderr, "%s%75li%10li\n", "Total", (long)m_instances,
>> +	     (long)m_allocated);
>> +    print_dash_line ();
>> +  }
>> +
>> +  /* Element size.  */
>> +  size_t m_element_size;
>> +  /* Pool name.  */
>> +  const char *m_pool_name;
>> +};
>> +
>> +extern mem_alloc_description<pool_usage> pool_allocator_usage;
>> +
>>  /* Type based memory pool allocator.  */
>>  template <typename T>
>>  class pool_allocator
>> @@ -35,7 +100,7 @@ public:
>>       has NUM elements.  The allocator support EXTRA_SIZE and can
>>       potentially IGNORE_TYPE_SIZE.  */
>>    pool_allocator (const char *name, size_t num, size_t extra_size = 0,
>> -		  bool ignore_type_size = false);
>> +		  bool ignore_type_size = false CXX_MEM_STAT_INFO);
>>    ~pool_allocator ();
>>    void release ();
>>    void release_if_empty ();
>> @@ -122,6 +187,8 @@ private:
>>    size_t m_extra_size;
>>    /* Flag if a pool allocator is initialized.  */
>>    bool m_initialized;
>> +  /* Memory allocation location.  */
>> +  mem_location m_location;
>>  };
>>  
>>  /* Last used ID.  */
>> @@ -151,19 +218,16 @@ struct alloc_pool_descriptor
>>  /* Hashtable mapping alloc_pool names to descriptors.  */
>>  extern hash_map<const char *, alloc_pool_descriptor> *alloc_pool_hash;
>>  
>> -/* For given name, return descriptor, create new if needed.  */
>> -alloc_pool_descriptor *
>> -allocate_pool_descriptor (const char *name);
>> -
>>  template <typename T>
>>  inline
>>  pool_allocator<T>::pool_allocator (const char *name, size_t num,
>> -				   size_t extra_size, bool ignore_type_size):
>> +				   size_t extra_size, bool ignore_type_size
>> +				   MEM_STAT_DECL):
>>    m_name (name), m_elts_per_block (num), m_returned_free_list (NULL),
>>    m_virgin_free_list (NULL), m_virgin_elts_remaining (0), m_elts_allocated (0),
>>    m_elts_free (0), m_blocks_allocated (0), m_block_list (NULL),
>>    m_ignore_type_size (ignore_type_size), m_extra_size (extra_size),
>> -  m_initialized (false) {}
>> +  m_initialized (false), m_location (ALLOC_POOL, false PASS_MEM_STAT) {}
>>  
>>  /* Initialize a pool allocator.  */
>>  
>> @@ -196,9 +260,11 @@ pool_allocator<T>::initialize ()
>>  
>>    if (GATHER_STATISTICS)
>>      {
>> -      alloc_pool_descriptor *desc = allocate_pool_descriptor (m_name);
>> -      desc->elt_size = size;
>> -      desc->created++;
>> +      pool_usage *u = pool_allocator_usage.register_descriptor
>> +	(this, new mem_location (m_location));
>> +
>> +      u->m_element_size = m_elt_size;
>> +      u->m_pool_name = m_name;
>>      }
>>  
>>    /* List header size should be a multiple of 8.  */
>> @@ -235,10 +301,10 @@ pool_allocator<T>::release ()
>>        free (block);
>>      }
>>  
>> -  if (GATHER_STATISTICS && false)
>> +  if (GATHER_STATISTICS)
>>      {
>> -      alloc_pool_descriptor *desc = allocate_pool_descriptor (m_name);
>> -      desc->current -= (m_elts_allocated - m_elts_free) * m_elt_size;
>> +      pool_allocator_usage.release_instance_overhead (this,
>> +	(m_elts_allocated - m_elts_free) * m_elt_size);
>>      }
>>  
>>    m_returned_free_list = NULL;
>> @@ -279,12 +345,7 @@ pool_allocator<T>::allocate ()
>>  
>>    if (GATHER_STATISTICS)
>>      {
>> -      alloc_pool_descriptor *desc = allocate_pool_descriptor (m_name);
>> -
>> -      desc->allocated += m_elt_size;
>> -      desc->current += m_elt_size;
>> -      if (desc->peak < desc->current)
>> -	desc->peak = desc->current;
>> +      pool_allocator_usage.register_instance_overhead (m_elt_size, this);
>>      }
>>  
>>  #ifdef ENABLE_VALGRIND_ANNOTATIONS
>> @@ -383,8 +444,7 @@ pool_allocator<T>::remove (T *object)
>>  
>>    if (GATHER_STATISTICS)
>>      {
>> -      alloc_pool_descriptor *desc = allocate_pool_descriptor (m_name);
>> -      desc->current -= m_elt_size;
>> +      pool_allocator_usage.release_instance_overhead (this, m_elt_size);
>>      }
>>  }
>>  
>> diff --git a/gcc/mem-stats-traits.h b/gcc/mem-stats-traits.h
>> index de1614e..f7843f2 100644
>> --- a/gcc/mem-stats-traits.h
>> +++ b/gcc/mem-stats-traits.h
>> @@ -10,11 +10,12 @@ enum mem_alloc_origin
>>    VEC,
>>    BITMAP,
>>    GGC,
>> +  ALLOC_POOL,
>>    MEM_ALLOC_ORIGIN_LENGTH
>>  };
>>  
>>  /* Verbose names of the memory allocation origin.  */
>>  static const char * mem_alloc_origin_names[] = { "Hash tables", "Hash maps",
>> -  "Hash sets", "Heap vectors", "Bitmaps", "GGC memory" };
>> +  "Hash sets", "Heap vectors", "Bitmaps", "GGC memory", "Allocation pool" };
>>  
>>  #endif // GCC_MEM_STATS_TRAITS_H
>> diff --git a/gcc/mem-stats.h b/gcc/mem-stats.h
>> index ac47231..fea68fa 100644
>> --- a/gcc/mem-stats.h
>> +++ b/gcc/mem-stats.h
>> @@ -18,11 +18,16 @@ struct mem_location
>>    inline mem_location () {}
>>  
>>    /* Constructor.  */
>> -  inline mem_location (const char *filename, const char *function, int line,
>> -		mem_alloc_origin origin, bool ggc):
>> +  inline mem_location (mem_alloc_origin origin, bool ggc,
>> +		const char *filename = NULL, int line = 0, const char *function = NULL):
>>      m_filename (filename), m_function (function), m_line (line), m_origin
>>      (origin), m_ggc (ggc) {}
>>  
>> +  /* Copy constructor.  */
>> +  inline mem_location (mem_location &other): m_filename (other.m_filename),
>> +    m_function (other.m_function), m_line (other.m_line),
>> +    m_origin (other.m_origin), m_ggc (other.m_ggc) {}
>> +
>>    /* Compute hash value based on file name, function name and line in
>>       source code. As there is just a single pointer registered for every
>>       constant that points to e.g. the same file name, we can use hash
>> @@ -79,11 +84,12 @@ struct mem_location
>>  struct mem_usage
>>  {
>>    /* Default constructor.  */
>> -  mem_usage (): m_allocated (0), m_times (0), m_peak (0) {}
>> +  mem_usage (): m_allocated (0), m_times (0), m_peak (0), m_instances (1) {}
>>  
>>    /* Constructor.  */
>> -  mem_usage (size_t allocated, size_t times, size_t peak):
>> -    m_allocated (allocated), m_times (times), m_peak (peak) {}
>> +  mem_usage (size_t allocated, size_t times, size_t peak, size_t instances = 0):
>> +    m_allocated (allocated), m_times (times), m_peak (peak),
>> +    m_instances (instances) {}
>>  
>>    /* Register overhead of SIZE bytes.  */
>>    inline void register_overhead (size_t size)
>> @@ -108,7 +114,8 @@ struct mem_usage
>>    {
>>      return mem_usage (m_allocated + second.m_allocated,
>>  		      m_times + second.m_times,
>> -		      m_peak + second.m_peak);
>> +		      m_peak + second.m_peak,
>> +		      m_instances + second.m_instances);
>>    }
>>  
>>    /* Comparison operator.  */
>> @@ -163,7 +170,7 @@ struct mem_usage
>>    /* Print line made of dashes.  */
>>    static inline void print_dash_line ()
>>    {
>> -    fprintf (stderr, "%s\n", std::string (128, '-').c_str ());
>> +    fprintf (stderr, "%s\n", std::string (140, '-').c_str ());
>>    }
>>  
>>    /* Dump header with NAME.  */
>> @@ -180,6 +187,8 @@ struct mem_usage
>>    size_t m_times;
>>    /* Peak allocation in bytes.  */
>>    size_t m_peak;
>> +  /* Number of container instances.  */
>> +  size_t m_instances;
>>  };
>>  
>>  /* Memory usage pair that connectes memory usage and number
>> @@ -241,9 +250,13 @@ public:
>>    /* Return descriptor for instance PTR.  */
>>    T *get_descriptor_for_instance (const void *ptr);
>>  
>> -  /* Register memory allocation descriptor for container PTR. ORIGIN identifies
>> +  /* Register memory allocation descriptor for container PTR which is
>> +     described by a memory LOCATION.  */
>> +  T *register_descriptor (const void *ptr, mem_location *location);
>> +
>> +  /* Register memory allocation descriptor for container PTR.  ORIGIN identifies
>>       type of container and GGC identifes if the allocation is handled in GGC
>> -     memory. Each location is identified by file NAME, LINE in source code and
>> +     memory.  Each location is identified by file NAME, LINE in source code and
>>       FUNCTION name.  */
>>    T *register_descriptor (const void *ptr, mem_alloc_origin origin,
>>  			  bool ggc, const char *name, int line,
>> @@ -321,33 +334,27 @@ mem_alloc_description<T>::get_descriptor_for_instance (const void *ptr)
>>    return m_reverse_map->get (ptr) ? (*m_reverse_map->get (ptr)).usage : NULL;
>>  }
>>  
>> -/* Register memory allocation descriptor for container PTR. ORIGIN identifies
>> -   type of container and GGC identifes if the allocation is handled in GGC
>> -   memory. Each location is identified by file NAME, LINE in source code and
>> -   FUNCTION name.  */
>>  
>> +  /* Register memory allocation descriptor for container PTR which is
>> +     described by a memory LOCATION.  */
>>  template <class T>
>>  inline T*
>>  mem_alloc_description<T>::register_descriptor (const void *ptr,
>> -					       mem_alloc_origin origin,
>> -					       bool ggc,
>> -					       const char *filename,
>> -					       int line,
>> -					       const char *function)
>> +					       mem_location *location)
>>  {
>> -  mem_location *l = new mem_location (filename, function, line, origin, ggc);
>>    T *usage = NULL;
>>  
>> -  T **slot = m_map->get (l);
>> +  T **slot = m_map->get (location);
>>    if (slot)
>>      {
>> -      delete l;
>> +      delete location;
>>        usage = *slot;
>> +      usage->m_instances++;
>>      }
>>    else
>>      {
>>        usage = new T ();
>> -      m_map->put (l, usage);
>> +      m_map->put (location, usage);
>>      }
>>  
>>    if (!m_reverse_map->get (ptr))
>> @@ -356,6 +363,24 @@ mem_alloc_description<T>::register_descriptor (const void *ptr,
>>    return usage;
>>  }
>>  
>> +/* Register memory allocation descriptor for container PTR.  ORIGIN identifies
>> +   type of container and GGC identifes if the allocation is handled in GGC
>> +   memory.  Each location is identified by file NAME, LINE in source code and
>> +   FUNCTION name.  */
>> +
>> +template <class T>
>> +inline T*
>> +mem_alloc_description<T>::register_descriptor (const void *ptr,
>> +					       mem_alloc_origin origin,
>> +					       bool ggc,
>> +					       const char *filename,
>> +					       int line,
>> +					       const char *function)
>> +{
>> +  mem_location *l = new mem_location (origin, ggc, filename, line, function);
>> +  return register_descriptor (ptr, l);
>> +}
>> +
>>  /* Register instance overhead identified by PTR pointer. Allocation takes
>>     SIZE bytes.  */
>>  
>>
> 
> Hello.
> 
> There's a small patch that fixes ICE if a compiler is not built with --enable-gather-mem-stats
> and we append -fmem-report to a command line options.
> 
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> 
> Ready for trunk?
> Thanks,
> Martin
> 

Honza approved the patch. I'm going to install the patch.

Martin