Re: [PATCH 01/35] Introduce new type-based pool allocator.

[Jeff Law <law@redhat.com>]

On 05/27/2015 07:56 AM, mliska wrote:
> Hello.
>
> Following patch set attempts to replace old-style pool allocator
> to a type-based one. Moreover, as we utilize  classes and structs that are used
> just by a pool allocator, these types have overwritten ctors and dtors.
> Thus, using the allocator is much easier and we shouldn't cast types
> back and forth. Another beneficat can be achieved in future, as we will
> be able to call a class constructors to correctly register a location,
> where a memory is allocated (-fgather-detailed-mem-stats).
>
> Patch can boostrap on x86_64-linux-gnu and ppc64-linux-gnu and
> survives regression tests on x86_64-linux-gnu.
>
> Ready for trunk?
> Thanks,
> Martin
>
> gcc/ChangeLog:
>
> 2015-04-30  Martin Liska  <mliska@suse.cz>
>
> 	* alloc-pool.c (struct alloc_pool_descriptor): Move definition
> 	to header file.
> 	* alloc-pool.h (pool_allocator::pool_allocator): New function.
> 	(pool_allocator::release): Likewise.
> 	(inline pool_allocator::release_if_empty): Likewise.
> 	(inline pool_allocator::~pool_allocator): Likewise.
> 	(pool_allocator::allocate): Likewise.
> 	(pool_allocator::remove): Likewise.
So on a general note, I don't like changing the size of the structure 
based on ENABLE_CHECKING.  If we've got other cases where we do this, 
then I guess it's OK, but if not, I'd prefer not to start doing so.


> ---

> +
> +  /* Align X to 8.  */
> +  size_t align_eight (size_t x)
> +  {
> +    return (((x+7) >> 3) << 3);
> +  }
> +
> +  const char *m_name;
> +#ifdef ENABLE_CHECKING
> +  ALLOC_POOL_ID_TYPE m_id;
> +#endif
> +  size_t m_elts_per_block;
> +
> +  /* These are the elements that have been allocated at least once and freed.  */
> +  allocation_pool_list *m_returned_free_list;
> +
> +  /* These are the elements that have not yet been allocated out of
> +     the last block obtained from XNEWVEC.  */
> +  char* m_virgin_free_list;
> +
> +  /* The number of elements in the virgin_free_list that can be
> +     allocated before needing another block.  */
> +  size_t m_virgin_elts_remaining;
> +  size_t m_elts_allocated;
> +  size_t m_elts_free;
> +  size_t m_blocks_allocated;
> +  allocation_pool_list *m_block_list;
> +  size_t m_block_size;
> +  size_t m_elt_size;
Several fields aren't documented.  They're largely self-explanatory, so 
I won't insist you document those trailing fields.  Your call whether or 
not to add docs for them.


> +
> +  /* Now align the size to a multiple of 4.  */
> +  size = align_eight (size);
Why not just aligned to 4, rather than a multiple of 4?  Presumably the 
extra 4 bytes don't matter in practice?

> +
> +template <typename T>
> +void
> +inline pool_allocator<T>::release_if_empty ()
> +{
> +  if (m_elts_free == m_elts_allocated)
> +    release ();
> +}
Is the release_if_empty all that useful in practice?

So the big issue in my mind continues to be the additional element in 
the structure when ENABLE_CHECKING is on.  As mentioned earlier, if 
we're already doing this elsewhere, then I won't object.  If we aren't, 
then I don't want to start doing so now.

The rest of the stuff are just minor questions, but nothing which would 
in my mind stop this from going forward.

Presumably your testing was with the whole series and they can't go in 
piecemeal, right?


jeff