Re: [RFC, PATCH] Split pool_allocator and create a new object_allocator

[Richard Sandiford <rdsandiford@googlemail.com>]

Martin Jambor <mjambor@suse.cz> writes:
> On Fri, Jul 03, 2015 at 09:55:58AM +0100, Richard Sandiford wrote:
>> Trevor Saunders <tbsaunde@tbsaunde.org> writes:
>> > On Thu, Jul 02, 2015 at 09:09:31PM +0100, Richard Sandiford wrote:
>> >> Martin Liška <mliska@suse.cz> writes:
>> >> > diff --git a/gcc/asan.c b/gcc/asan.c
>> >> > index e89817e..dabd6f1 100644
>> >> > --- a/gcc/asan.c
>> >> > +++ b/gcc/asan.c
>> >> > @@ -362,20 +362,20 @@ struct asan_mem_ref
>> >> >    /* Pool allocation new operator.  */
>> >> >    inline void *operator new (size_t)
>> >> >    {
>> >> > -    return pool.allocate ();
>> >> > +    return ::new (pool.allocate ()) asan_mem_ref ();
>> >> >    }
>> >> >  
>> >> >    /* Delete operator utilizing pool allocation.  */
>> >> >    inline void operator delete (void *ptr)
>> >> >    {
>> >> > -    pool.remove ((asan_mem_ref *) ptr);
>> >> > +    pool.remove (ptr);
>> >> >    }
>> >> >  
>> >> >    /* Memory allocation pool.  */
>> >> > -  static pool_allocator<asan_mem_ref> pool;
>> >> > +  static pool_allocator pool;
>> >> >  };
>> >> 
>> >> I'm probably going over old ground/wounds, sorry, but what's the benefit
>> >> of having this sort of pattern?  Why not simply have object_allocators
>> >> and make callers use pool.allocate () and pool.remove (x) (with pool.remove
>> >> calling the destructor) instead of new and delete?  It feels wrong to me
>> >> to tie the data type to a particular allocation object like this.
>> >
>> > Well the big question is what does allocate() do about construction?  if
>> > it seems wierd for it to not call the ctor, but I'm not sure we can do a
>> > good job of forwarding args to allocate() with C++98.
>> 
>> If you need non-default constructors then:
>> 
>>   new (pool) type (aaa, bbb)...;
>> 
>> doesn't seem too bad.  I agree object_allocator's allocate () should call
>> the constructor.
>> 
>
> but then the pool allocator must not call placement new on the
> allocated memory itself because that would result in double
> construction.

But we're talking about two different methods.  The "normal" allocator
object_allocator <T>::allocate () would use placement new and return a
pointer to the new object while operator new (size_t, object_allocator <T> &)
wouldn't call placement new and would just return a pointer to the memory.

>> >> And using the pool allocator functions directly has the nice property
>> >> that you can tell when a delete/remove isn't necessary because the pool
>> >> itself is being cleared.
>> >
>> > Well, all these cases involve a pool with static storage lifetime right?
>> > so actually if you don't delete things in these pool they are
>> > effectively leaked.
>> 
>> They might have a static storage lifetime now, but it doesn't seem like
>> a good idea to hard-bake that into the interface
>
> Does that mean that operators new and delete are considered evil?

Not IMO.  Just that static load-time-initialized caches are not
necessarily a good thing.  That's effectively what the pool
allocator is.

>> (by saying that for
>> these types you should use new and delete, but for other pool-allocated
>> types you should use object_allocators).
>
> Depending on what kind of pool allocator you use, you will be forced
> to either call placement new or not, so the inconsistency will be
> there anyway.

But how we handle argument-taking constructors is a problem that needs
to be solved for the pool-allocated objects that don't use a single
static type-specific pool.  And once we solve that, we get consistency
across all pools:

- if you want a new object and argumentless construction is OK,
  use "pool.allocate ()"

- if you want a new object and need to pass arguments to the constructor,
  use "new (pool) some_type (arg1, arg2, ...)"

>> Maybe I just have bad memories
>> from doing the SWITCHABLE_TARGET stuff, but there I was changing a lot
>> of state that was "obviously" static in the old days, but that needed
>> to become non-static to support vaguely-efficient switching between
>> different subtargets.  The same kind of thing is likely to happen again.
>> I assume things like the jit would prefer not to have new global state
>> with load-time construction.
>
> I'm not sure I follow this branch of the discussion, the allocators of
> any kind surely can dynamically allocated themselves?

Sure, but either (a) you keep the pools as a static part of the class
and some initialisation and finalisation code that has tendrils into
all such classes or (b) you move the static pool outside of the
class to some new (still global) state.  Explicit pool allocation,
like in the C days, gives you the option of putting the pool whereever
it needs to go without relying on the principle that you can get to
it from global state.

Thanks,
Richard