Hi Martin, Martin Liška <mli...@suse.cz> writes: > On 07/03/2015 03:07 PM, Richard Sandiford wrote: >> Martin Jambor <mjam...@suse.cz> writes: >>> On Fri, Jul 03, 2015 at 09:55:58AM +0100, Richard Sandiford wrote: >>>> Trevor Saunders <tbsau...@tbsaunde.org> writes: >>>>> On Thu, Jul 02, 2015 at 09:09:31PM +0100, Richard Sandiford wrote: >>>>>> Martin Liška <mli...@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