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

[Richard Biener <richard dot guenther at gmail dot com>]

On Thu, Jul 16, 2015 at 12:47 PM, Martin LiÅka <mliska@suse.cz> wrote:
> On 07/09/2015 11:43 PM, Martin LiÅka wrote:
>> On 07/03/2015 06:18 PM, Richard Sandiford wrote:
>>> Hi Martin,
>>>
>>> Martin LiÅka <mliska@suse.cz> writes:
>>>> On 07/03/2015 03:07 PM, Richard Sandiford wrote:
>>>>> 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
>>>>>
>>>>
>>>> Ok Richard.
>>>>
>>>> I've just finally understood your suggestions and I would suggest following:
>>>>
>>>> + I will add a new method to object_allocator<T> that will return an
>>>> allocated memory (void*)
>>>> (w/o calling any construction)
>>>> + object_allocator<T>::allocate will call placement new with for a
>>>> parameterless ctor
>>>> + I will remove all overwritten operators new/delete on e.g. et_forest, ...
>>>> + For these classes, I will add void* operator new (size_t,
>>>> object_allocator<T> &)
>>>
>>> I was thinking we'd simply use allocate () for cases where we don't
>>> need to pass arguments to the constructor.  It looks like et_forest
>>> comes into that category.  The operator new would be a single function
>>> defined in pool-allocator.h for cases where explicit construction
>>> is needed.
>>>
>>> In fact, it looks from a quick grep like all current uses of pool operator
>>> new/delete are in POD types, so there are no special constructors.
>>> The best example I could come up with was the copy constructor in:
>>>
>>>    return new lra_live_range (*r);
>>>
>>> which would become:
>>>
>>>    return new (*live_range_pool) lra_live_range (*r);
>>>
>>> but perhaps we should have an object_allocator copy (T *) routine:
>>>
>>>    return live_range_pool->copy (*r);
>>>
>>>> + Pool allocators connected to these classes will be back transformed to
>>>> static variables and
>>>> one would call new et_forest (my_et_forest_allocator)
>>>
>>> Thanks, this sounds really good to me.  Please make sure I'm not the
>>> only one who thinks so though :-)
>>>
>>> I think the "normal" remove () method should then also call the destructor.
>>>
>>> Thanks,
>>> Richard
>>>
>>
>> Hello.
>>
>> This final version which I agreed with Richard Sandiford.
>> Hope this can be finally installed to trunk?
>>
>> Patch can bootstrap and survive regression tests on x86_64-linux-gnu.
>>
>> Thanks,
>> Martin
>
> Hello.
>
> I would like to ping the patch as it's a blocker for some ppc64 machines.

Ok.

Thanks,
Richard.

> Thanks,
> Martin
>
>