Re: [PATCH 1/2] Memory statistics enhancement.
On 06/01/2015 10:16 AM, 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
* 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.
-
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
+ /* 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);
Static sized buffer used in a sprintf where the strings are potentially
user controlled. Not good, even in dumping code, still not good.
+
+s[48] = '\0';
?!? Presumably you're just truncating the output line here for the
subsequent fprintf call. Consider using a const with a symbolic name
rather than the magic "48". I say "consider" because there's magic
constants all over the place in the dumping code. So it may not be worth
the effort. Your call.
+
+ /* 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 ();
+ }
Note the header is static inline, footer is just inline. Please try to
make them consistent.
@@ -235,10 +301,10 @@ pool_allocator::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);
Looks like line wrapping needs to be fixed.
Clearly the biggest issue is that static sized buffer used to hold the
results of sprintf... Once that and the smaller issues are fixed, this
is OK.
jeff
Re: [PATCH 1/2] Memory statistics enhancement.
On 06/02/2015 03:58 PM, Jeff Law wrote:
> On 06/01/2015 10:16 AM, 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
>>
>> * 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.
> -
>
>> 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
>
>> + /* 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);
> Static sized buffer used in a sprintf where the strings are potentially user
> controlled. Not good, even in dumping code, still not good.
>
>> +
>> +s[48] = '\0';
> ?!? Presumably you're just truncating the output line here for the
> subsequent fprintf call. Consider using a const with a symbolic name rather
> than the magic "48". I say "consider" because there's magic constants all
> over the place in the dumping code. So it may not be worth the effort. Your
> call.
>
> +
>> + /* 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 ();
>> + }
> Note the header is static inline, footer is just inline. Please try to make
> them consistent.
>
> @@ -235,10 +301,10 @@ pool_allocator::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);
> Looks like line wrapping needs to be fixed.
>
>
> Clearly the biggest issue is that static sized buffer used to hold the
> results of sprintf... Once that and the smaller issues are fixed, this is OK.
>
> jeff
>
Hi Jeff.
This patch improves the patch in suggested way. I verified it by valgrind that
the allocated string buffer
is used correctly.
Ready for trunk?
Thanks,
Martin
>From 3373344b681f343ebe038e4c75abc8fe4c39ebea Mon Sep 17 00:00:00 2001
From: mliska
Date: Mon, 1 Jun 2015 18:16:52 +0200
Subject: [PATCH 1/2] Port pool-allocator memory stats to a new infrastructure.
gcc/ChangeLog:
2015-06-02 Martin Liska
* 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
* mem-stats.h (mem_location::to_string): New function.
* bitmap.h (struct bitmap_usage): Use this new function.
* ggc-common.c (struct ggc_usage): Likewise.
the function.
---
gcc/alloc-pool.c | 60 +
gcc/alloc-pool.h | 100 ++---
gcc/bitmap.h | 11 +++---
gcc/ggc-common.
Re: [PATCH 1/2] Memory statistics enhancement.
On 06/02/2015 09:05 AM, Martin Liška wrote:
On 06/02/2015 03:58 PM, Jeff Law wrote:
On 06/01/2015 10:16 AM, 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
* 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.
-
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
+ /* 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);
Static sized buffer used in a sprintf where the strings are potentially user
controlled. Not good, even in dumping code, still not good.
+
+s[48] = '\0';
?!? Presumably you're just truncating the output line here for the subsequent fprintf call.
Consider using a const with a symbolic name rather than the magic "48". I say
"consider" because there's magic constants all over the place in the dumping code. So it
may not be worth the effort. Your call.
+
+ /* 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 ();
+ }
Note the header is static inline, footer is just inline. Please try to make
them consistent.
It doesn't look like you did anything with this. Is there a reason that
the dump_header and dump_footer have different linkage? Also the
linkage/return type for dump_header should be on its own line.
With that fixed, this is OK for the trunk.
jeff
Re: [PATCH 1/2] Memory statistics enhancement.
On 06/02/2015 07:17 PM, Jeff Law wrote:
> On 06/02/2015 09:05 AM, Martin Liška wrote:
>> On 06/02/2015 03:58 PM, Jeff Law wrote:
>>> On 06/01/2015 10:16 AM, 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
* 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.
>>> -
>>>
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
>>>
+ /* 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);
>>> Static sized buffer used in a sprintf where the strings are potentially
>>> user controlled. Not good, even in dumping code, still not good.
>>>
+
+s[48] = '\0';
>>> ?!? Presumably you're just truncating the output line here for the
>>> subsequent fprintf call. Consider using a const with a symbolic name
>>> rather than the magic "48". I say "consider" because there's magic
>>> constants all over the place in the dumping code. So it may not be worth
>>> the effort. Your call.
>>>
>>> +
+ /* 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 ();
+ }
>>> Note the header is static inline, footer is just inline. Please try to
>>> make them consistent.
> It doesn't look like you did anything with this. Is there a reason that the
> dump_header and dump_footer have different linkage? Also the linkage/return
> type for dump_header should be on its own line.
>
> With that fixed, this is OK for the trunk.
>
> jeff
Hi Jeff.
Different linkage is because of dump_header is just a generic header unrelated
to any real numbers.
On the other hand, dump_footer is called on a total instance.
I've just identified that the whole memory statistics infrastructure lacks
correct GNU formatting
in case of C++ member functions. I'm going to fix it in a different patch.
Thanks,
Martin
Re: [PATCH 1/2] Memory statistics enhancement.
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
>
> * 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 *alloc_pool_hash;
> -
> -struct alloc_pool_descriptor *
> -allocate_pool_descriptor (const char *name)
> -{
> - if (!alloc_pool_hash)
> -alloc_pool_hash = new hash_map (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_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 - 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,
> +
Re: [PATCH 1/2] Memory statistics enhancement.
On 06/03/2015 03:14 AM, Martin Liška wrote:
On 06/02/2015 07:17 PM, Jeff Law wrote:
On 06/02/2015 09:05 AM, Martin Liška wrote:
On 06/02/2015 03:58 PM, Jeff Law wrote:
On 06/01/2015 10:16 AM, 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
* 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.
-
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
+ /* 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);
Static sized buffer used in a sprintf where the strings are potentially user
controlled. Not good, even in dumping code, still not good.
+
+s[48] = '\0';
?!? Presumably you're just truncating the output line here for the subsequent fprintf call.
Consider using a const with a symbolic name rather than the magic "48". I say
"consider" because there's magic constants all over the place in the dumping code. So it
may not be worth the effort. Your call.
+
+ /* 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 ();
+ }
Note the header is static inline, footer is just inline. Please try to make
them consistent.
It doesn't look like you did anything with this. Is there a reason that the
dump_header and dump_footer have different linkage? Also the linkage/return
type for dump_header should be on its own line.
With that fixed, this is OK for the trunk.
jeff
Hi Jeff.
Different linkage is because of dump_header is just a generic header unrelated
to any real numbers.
On the other hand, dump_footer is called on a total instance.
Right, but why does that affect linkage? Sorry to keep harping on this
minor issue, but something just doesn't make any sense to me.
I've just identified that the whole memory statistics infrastructure lacks
correct GNU formatting
in case of C++ member functions. I'm going to fix it in a different patch.
Excellent. Thanks,
jeff
Re: [PATCH 1/2] Memory statistics enhancement.
On 06/03/2015 03:41 AM, Martin Liška wrote: From a52eecf7c78f2eee0ccac662459e89ddbedd0244 Mon Sep 17 00:00:00 2001 From: mliska Date: Wed, 3 Jun 2015 11:37:01 +0200 Subject: [PATCH] Fix GNU coding style in memory statistics. gcc/ChangeLog: 2015-06-03 Martin Liska * alloc-pool.h (struct pool_usage): Correct GNU coding style. * bitmap.h (struct bitmap_usage): Likewise. * ggc-common.c (struct ggc_usage): Likewise. * mem-stats.h (struct mem_location): Likewise. (struct mem_usage): Likewise. * vec.c (struct vec_usage): Likewise. OK. jeff
Re: [PATCH 1/2] Memory statistics enhancement.
On 06/03/2015 03:18 PM, Jeff Law wrote:
> On 06/03/2015 03:14 AM, Martin Liška wrote:
>> On 06/02/2015 07:17 PM, Jeff Law wrote:
>>> On 06/02/2015 09:05 AM, Martin Liška wrote:
On 06/02/2015 03:58 PM, Jeff Law wrote:
> On 06/01/2015 10:16 AM, 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
>>
>> * 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.
>-
>
>> 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
>
>> + /* 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);
> Static sized buffer used in a sprintf where the strings are potentially
> user controlled. Not good, even in dumping code, still not good.
>
>> +
>> +s[48] = '\0';
> ?!? Presumably you're just truncating the output line here for the
> subsequent fprintf call. Consider using a const with a symbolic name
> rather than the magic "48". I say "consider" because there's magic
> constants all over the place in the dumping code. So it may not be worth
> the effort. Your call.
>
>+
>> + /* 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 ();
>> + }
> Note the header is static inline, footer is just inline. Please try to
> make them consistent.
>>> It doesn't look like you did anything with this. Is there a reason that
>>> the dump_header and dump_footer have different linkage? Also the
>>> linkage/return type for dump_header should be on its own line.
>>>
>>> With that fixed, this is OK for the trunk.
>>>
>>> jeff
>>
>> Hi Jeff.
>>
>> Different linkage is because of dump_header is just a generic header
>> unrelated to any real numbers.
>> On the other hand, dump_footer is called on a total instance.
> Right, but why does that affect linkage? Sorry to keep harping on this
> minor issue, but something just doesn't make any sense to me.
Hi.
That's fine, I would like to explain that difference. Each of these two
functions are called in a bit different way:
T::dump_header (mem_location::get_origin_name (origin)); // that's why the
function is static
total.dump_footer (); // that's why the function is method
Martin
>
>>
>> I've just identified that the whole memory statistics infrastructure lacks
>> correct GNU formatting
>> in case of C++ member functions. I'm going to fix it in a different patch.
> Excellent. Thanks,
>
> jeff
Re: [PATCH 1/2] Memory statistics enhancement.
On 06/03/2015 08:11 AM, Martin Liška wrote: That's fine, I would like to explain that difference. Each of these two functions are called in a bit different way: T::dump_header (mem_location::get_origin_name (origin)); // that's why the function is static total.dump_footer (); // that's why the function is method OK. Patch is good to go. Thanks for being patient with my questions :-) jeff
Re: [PATCH 1/2] Memory statistics enhancement.
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
>
> * 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 *alloc_pool_hash;
> -
> -struct alloc_pool_descriptor *
> -allocate_pool_descriptor (const char *name)
> -{
> - if (!alloc_pool_hash)
> -alloc_pool_hash = new hash_map (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_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 - 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,
> +
Re: [PATCH 1/2] Memory statistics enhancement.
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
>>
>> * 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 *alloc_pool_hash;
>> -
>> -struct alloc_pool_descriptor *
>> -allocate_pool_descriptor (const char *name)
>> -{
>> - if (!alloc_pool_hash)
>> -alloc_pool_hash = new hash_map (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_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 > - 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. */
>> + p
