[RFC] Split pool_allocator and create a new object_allocator

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

[RFC] Split pool_allocator and create a new object_allocator

Commit Message

Comments

Patch