On 9/10/2011 5:07 PM, Andrei Alexandrescu wrote:
On 9/6/11 2:53 PM, Jonathan M Davis wrote:
Okay. Onto the next item in the review queue. I'm going to be the review
manager this time around, and we're going to be reviewing dsimcha's
region
allocator. The review starts now and will end on 2011-09-21 at
midnight in UTC
(so about 2 weeks from now, when the 21st begins). Assuming that we're
ready
to vote at that point, I'll start a thread for voting for inclusion in
Phobos,
and the vote will last a week. If it's not ready to be voted on for some
reason (such as dsimcha needing to go back and make a bunch of changes
that
are going to need to be reviewed), then voting will be postponed and
it'll be
put back in the review queue once it's ready for review again.
Docs:
http://cis.jhu.edu/~dsimcha/d/phobos/std_regionallocator.html
Unfortunately my review is incomplete because I don't see the allocator
interface.
In theory it should be possible to first define the region allocator and
then the allocator interface. The other way around is also valid.
However, clearly it's best to define the general allocator interface
_together_ with the region allocator.
I mostly took your proposal from a few months ago and doctored it
slightly. Basically, my theory was to define the allocator interface by
example (both RegionAllocator and GCAllocator being examples) and
resolve any ambiguities through the review process before creating more
formal documentation of the allocator interface. I'll probably create a
std.allocator.allocator, which would be similar to the top of
std.container and contain documentation of the general allocator interface.
Containers will need an allocator, and I think it's best to make it a
straight dynamic interface. In that design, the region allocator would
offer the scoped structs PLUS factory methods in those structs that
return implementations for those interfaces.
"Dynamic" as in using the `interface` keyword? My concern with this is
that a major purpose of RegionAllocator is to avoid the GC and its
global lock like the plague. I think it would be a **terrible** misuse
of things if you had to perform a heap allocation to get at the dynamic
interface. I'll think about this, though. There may be an easy
workaround.
My other concern is that dynamic interfaces would break ref counting of
RegionAllocator instances because the GC frees things in an undefined
order, and I guess you'd have to rely on the GC to free a region. Maybe
I'm completely misunderstanding your suggestion, though.
* It's region_allocator or region.allocator, not regionallocator.
Ironically a good example of why is in this very name - is it "region
allocator" or "regional locator"?
You mean for the module name? I was thinking std.allocators.region.
Are you ok with this, too?
* Use "bumped up" and "bumped down" or something instead of
"incremented" and "decremented", which imply 1 as the unit.
Good point.
* Enumerated paragraphs are not visually distinguished.
If you mean the lists of advantages/disadvantages at the top of the
module, I'd like to distinguish them better but don't know how to in
DDoc. Any advice?
* Synopsis should go before the textual intro.
Ok.
* Exception should be at the bottom, not the top of the dox.
Makes sense. I'm also thinking it should be derived from Error, not
Exception and be renamed RegionAllocatorError, since it's only thrown on
API misuse, i.e. bugs in client code. I could also use asserts, but I
decided not to because I've made it so that everything can be checked
cheaply and measured to make sure the checking doesn't lead to
performance problems, and because certain misuses of the API would be
virtually impossible to debug without the exception error messages.
* There's a problem with "std.regionallocator
RegionAllocator.regionallocator RegionAllocator" - i.e. a macro that
didn't expand to what it should have.
I noticed this, too, but have no idea how to fix it.
* I didn't understand from the dox what the relationship between
RegionAllocator and RegionAllocatorStack is, mainly because
RegionAllocator is used before having been defined. (This is a difficult
problem in general.)
Hmm, maybe I should move RegionAllocator before RegionAllocatorStack and
focus on the thread-local default instantiation first, since this is
what people should use for most simple use cases. Creating explicit
RegionAllocatorStack instances is a more advanced use case.
Anyhow, the explanation FYI is that a RegionAllocatorStack is a large
chunk of memory and can be allocated from by the RegionAllocator that's
conceptually at the top of the stack. When the last instance of the
RegionAllocator at the top of the stack goes out of scope (this is kept
track of via reference counting), the RegionAllocatorStack is freed up
to the memory used by the next RegionAllocator down on the stack.
Conceptually, you can think of this as a "stack of simple regions" where
"simple regions" are the canonical ones described in the literature
rather than the somewhat embellished ones in this module.
* The GCScan enum is defined uncomfortably far from the places where
it's relevant. (Another difficult matter.)
I agree, but I don't really see any easy way to fix this. I could put
GCScan inside RegionAllocatorStack, but who wants to type
RegionAllocatorStack.GCScan.yes just to get a simple flag?
* I think "scanThreadLocalStack" should be "threadLocalStackIsScannable"
so as people don't confuse it with an action.
Sounds good.
* newArray could only take the element type as a template parameter; the
number of dimensions in unequivocally determined from the number of
arguments.
Good point. Actually, this can probably be changed in
std.array.uninitializedArray, too. There was some reason why I did it
the way I did, but I don't remember why. I'll look into it.
* In fact new(Uninitialized)Array should not be a member as its workings
are not specific to this allocator. It should be a free function taking
an allocator as a parameter.
My concern about this is that some allocators might want to do some
"special" things based on knowing the type. A good example is that
GCAllocator decides whether to scan for pointers based on knowing the
type. RegionAllocator has a completely different method for determining
this policy. When/if I make a std.allocators.allocator (which would
contain documentation of the general allocator interface, etc.) I can
make a mixin template that provides default implementations of things
that have obvious implementations in terms of lower-level allocator
features. (I think array() would also be included here.) This could be
mixed in and overridden if need be, but would provide the defaults to
save on code duplication across allocators.
* The documentation for resize() is the first time "allocator interface"
is mentioned (without having been defined). Wait a minute. I was
hungrily looking for this since I opened the doc.
Yeah, see above about documentation of the allocator interface.
* I think alignBytes should be a compile-time computable property, and
that the documentation should mention that.
Good point.
* isAutomatic, isScoped, freeIsChecked should also be static and
statically computable properties (perhaps not freeIsChecked).
I think they are (actually, I think they're enums) and DDoc just doesn't
reflect this. If not, then I'll definitely fix it.
Andrei
