Re: [PATCH] Fix up --enable-gather-detailed-mem-stats with lazy construction of hash_{table,set}

[Martin Liška <mliska at suse dot cz>]

On 3/26/19 10:56 AM, Jakub Jelinek wrote:
> On Mon, Mar 25, 2019 at 09:42:01PM +0100, Jakub Jelinek wrote:
>> On Mon, Mar 25, 2019 at 03:10:04PM -0400, Jason Merrill wrote:
>>>> 2) has the false -> true fixed
>>>> 3) ditto, but furthermore is moved out of add_capture into the lambda
>>>> introducer parsing routine only, tests for [this, this] and [this, *this]
>>>> etc. are done using LAMBDA_EXPR_THIS_CAPTURE and the duplicate check for
>>>> capture_id is simplified too.
>>>
>>> I needed to make the below change to avoid crashing with
>>> --enable-gather-detailed-mem-stats, make sense?  Incidentally, why not set
>>
>> CCing Martin on this, I really don't know how the mem-stats.h stuff is
>> supposed to work and how it works on reallocations.
>> Seems release_instance_overhead is used in expand method without the second
>> argument true and in the dtor with true, so the latter likely does both
>> what expand needs to do and undo what register_descriptor did.
>> And because register_descriptor has been called, it needs to be undone, but
>> as no register_instance_overhead has been called, that part should not be.
>> We don't have unregister_descriptor or something similar though.
>> So, the fix probably needs to add something in mem-stats.h and do what your
>> patch did + else if (m_gather_mem_stats) call this new mem_stat.h method
>> after the if (!Lazy || m_entries) block.
> 
> Here is a patch that does that.

The patch is correct. Note that unregistering of a descriptor is not a critical,
but yes, it occupies a memory.

Martin

> 
> Bootstrapped on x86_64-linux with --enable-gather-detailed-mem-stats,
> tested on a couple of lambda testcases with -fmem-report, some of them with
> zero or one explicit captures, on others with more of them (e.g. on
> lambda-init1{8,9}.C) and tested build without
> --enable-gather-detailed-mem-stats.  Ok for trunk?
> 
> 2019-03-26  Jason Merrill  <jason@redhat.com>
> 	    Jakub Jelinek  <jakub@redhat.com>
> 
> 	* mem-stats.h (mem_alloc_description::unregister_descriptor): New
> 	method.
> 	(mem_alloc_description::release_object_overhead): Fix comment typos.
> 	* hash-table.h (hash_table::~hash_table): Call
> 	release_instance_overhead only if m_entries is non-NULL, otherwise
> 	call unregister_descriptor.
> 
> --- gcc/mem-stats.h.jj	2019-02-26 21:35:28.959081094 +0100
> +++ gcc/mem-stats.h	2019-03-26 09:25:10.132128088 +0100
> @@ -342,9 +342,15 @@ public:
>    T *release_instance_overhead (void *ptr, size_t size,
>  				bool remove_from_map = false);
>  
> -  /* Release intance object identified by PTR pointer.  */
> +  /* Release instance object identified by PTR pointer.  */
>    void release_object_overhead (void *ptr);
>  
> +  /* Unregister a memory allocation descriptor registered with
> +     register_descriptor (remove from reverse map), unless it is
> +     unregistered through release_instance_overhead with
> +     REMOVE_FROM_MAP = true.  */
> +  void unregister_descriptor (void *ptr);
> +
>    /* Get sum value for ORIGIN type of allocation for the descriptor.  */
>    T get_sum (mem_alloc_origin origin);
>  
> @@ -522,7 +528,7 @@ mem_alloc_description<T>::release_instan
>    return usage;
>  }
>  
> -/* Release intance object identified by PTR pointer.  */
> +/* Release instance object identified by PTR pointer.  */
>  
>  template <class T>
>  inline void
> @@ -536,6 +542,17 @@ mem_alloc_description<T>::release_object
>      }
>  }
>  
> +/* Unregister a memory allocation descriptor registered with
> +   register_descriptor (remove from reverse map), unless it is
> +   unregistered through release_instance_overhead with
> +   REMOVE_FROM_MAP = true.  */
> +template <class T>
> +inline void
> +mem_alloc_description<T>::unregister_descriptor (void *ptr)
> +{
> +  m_reverse_map->remove (ptr);
> +}
> +
>  /* Default contructor.  */
>  
>  template <class T>
> --- gcc/hash-table.h.jj	2019-03-26 08:52:52.739640547 +0100
> +++ gcc/hash-table.h	2019-03-26 09:26:27.697864773 +0100
> @@ -652,12 +652,13 @@ hash_table<Descriptor, Lazy, Allocator>:
>  	Allocator <value_type> ::data_free (m_entries);
>        else
>  	ggc_free (m_entries);
> +      if (m_gather_mem_stats)
> +	hash_table_usage ().release_instance_overhead (this,
> +						       sizeof (value_type)
> +						       * m_size, true);
>      }
> -
> -  if (m_gather_mem_stats)
> -    hash_table_usage ().release_instance_overhead (this,
> -						   sizeof (value_type)
> -						   * m_size, true);
> +  else if (m_gather_mem_stats)
> +    hash_table_usage ().unregister_descriptor (this);
>  }
>  
>  /* This function returns an array of empty hash table elements.  */
> 
> 
> 	Jakub
>