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

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

On 05/27/2015 07:44 PM, Jeff Law wrote:
> 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.

Hello.

This mechanism has been just adapted. I find it quite useful as we have examples in source code where we
allocate same struct/class types from a various pool. For debugging purpose, it helps to identify if
release operation is called for a correct pool.

> > ---
>
> > +
> > +  /* 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.

Ok, even tough they are self-explanatory, I'm going to document these fields.

> > +
> > +  /* 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?

Also adapted constant, hope it's chosen as the best.

> > +
> > +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?

Yes, 02/x uses that feature.

> 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?

Right, regression tests were run just once for the whole series, but I've tested that every individual patch can be applied and the compiler can be successfully built.
Anyway, I would like to commit all these patches at once (one by one).
Thus, I'm going to wait for approval for the whole series before I'll commit the set.

Thanks,
Martin

> jeff