TempAlloc Overhaul

dsimcha dsimcha at yahoo.com
Fri Jun 17 07:12:42 PDT 2011


On 6/16/2011 11:46 PM, Andrei Alexandrescu wrote:
> On 6/11/11 11:26 AM, dsimcha wrote:
>> I've overhauled my TempAlloc proposal based on some of the suggestions
>> I've received.
>
> Review:

Thank you for the review.

>
> * Mention no safety at disadvantages: deallocating memory may leave
> pointers dangling.

Good point.

>
> * The synopsis should use scope(exit) TempAlloc.frameFree(); as that is
> a robust idiom.

Good point.

>
> * newArray -> "For performance reasons, the returned array is not
> initialized." Then call it newUninitializedArray. Make safety the
> default and lack thereof the verbose alternative.

Here I'm going to have to strongly disagree with you.  The whole point 
of TempAlloc is that it's a performance hack.  In general I agree it's 
better to do the safe thing by default, but libraries specifically 
written as performance hacks are an exception.  In TempAlloc it makes 
sense to do the efficient thing by default.  I personally would never 
use newArray as opposed to newUninitializedArray and 
TempAlloc.newUninitializedArray (31 characters) feels **ridiculously** 
verbose and would clutter the API.

>
> * newArray -> "It is not scanned for pointers by the garbage collector
> unless GC.addRange is called." I'd agree that malloc() shouldn't do so
> because it doesn't know the type, but with newArray you do know the type
> so you could automatically call GC.addRange appropriately. If the client
> doesn't want that, they can obtain it with extra syntax. Since now you
> have two things (uninitialized and noscan) you may want to make them
> function parameters instead of part of the function name.

Again, I'm going to have to strongly disagree with you here.  Adding the 
range to the GC would require taking the GC lock, which is to be avoided 
like the plague.  In a library that exists specifically as a performance 
hack, things like this should be explicit if there's any doubt about the 
user's intent.  This should come before safety.  Once you require this 
explicitness, there's no need for the TempAlloc API to handle it 
anymore, because the user can just call GC.addRange.

Furthermore, the GC's management of ranges isn't very efficient (I think 
it's O(N)) so if you have a whole bunch of TempAlloc-allocated arrays, 
things are going to grind to a halt.  Lastly, extra bookkeeping would be 
necessary to decide whether to call removeRange().

>
> * I'm a bit unsure about free(). People already have the pointer, so
> free() should use it as a cheap way to check that they're freeing the
> right thing. There should be free(void*), freeLastAllocated(), and
> possibly destroyAndFree(T)(T *).
>

Probably a good idea.  So free(void*) would just do a check and then 
call freeLastAllocated()?  I'm not sure about destroyAndFree().  What 
semantics are you suggesting for it?  Calling the d'tor if any and then 
freeing?

> * segmentSlack() -> segmentAvailable().

I guess that's a little more clear.

>
> * stackCat() is a bit surprising. A better way would be something like a
> TempAlloc appender that accepts a chain().
>
> * In fact looking at tempArray() just below that's all - it can accept
> chain(). No need for stackCat.

Good point.  I kind of didn't like stackCat anyhow.
>
> * mixin newFrame should be renamed to scopedFrame.

Hmm.  I kind of like newFrame.  Why is scopedFrame a better name?  If I 
understood why it's better, I'd be more inclined to change it.



More information about the Digitalmars-d mailing list