On Saturday, 13 June 2015 at 19:08:26 UTC, Jacob Carlborg wrote:
> On 2015-06-12 13:06, Dicebot wrote:
>> The legendary allocator package by Andrei Alexandrescu has arrived at
>> your doorsteps and kindly asks to let it into Phobos
>>
>> http://wiki.dlang.org/Review/std.experimental.allocator
>>
>> Docs: http://erdani.com/d/phobos-prerelease/std_experimental_allocator.html
>> Code:
>> https://github.com/andralex/phobos/tree/allocator/std/experimental/allocator
>
> I think "IAllocator", "theAllocator" and "it" are really bad names. I recommend renaming those symbols to:
>
> IAllocator -> Allocator
> theAllocator -> currentAllocator or tlsAllocator
> it -> allocator or instance. Even better if a static "opDispatch" could be used to forward all methods to the instance.


https://github.com/D-Programming-Language/phobos/commit/319f3297418c515a6d2e52e6e52d0f3f5895f587 changes .it to .instance for all allocators. -- Andrei

On 2015-06-16 22:29, Andrei Alexandrescu wrote:

> https://github.com/D-Programming-Language/phobos/commit/319f3297418c515a6d2e52e6e52d0f3f5895f587
> changes .it to .instance for all allocators. -- Andrei

Awesome, thanks.

-- 
/Jacob Carlborg

Bringing back to first page.

On Tue, 16 Jun 2015 16:29:08 -0400, Andrei Alexandrescu <SeeWebsiteForEmail@erdani.org> wrote:

> On Saturday, 13 June 2015 at 19:08:26 UTC, Jacob Carlborg wrote:
>> On 2015-06-12 13:06, Dicebot wrote:
>>> The legendary allocator package by Andrei Alexandrescu has arrived at
>>> your doorsteps and kindly asks to let it into Phobos
>>>
>>> http://wiki.dlang.org/Review/std.experimental.allocator
>>>
>>> Docs: http://erdani.com/d/phobos-prerelease/std_experimental_allocator.html
>>> Code:
>>> https://github.com/andralex/phobos/tree/allocator/std/experimental/allocator
>>
>> I think "IAllocator", "theAllocator" and "it" are really bad names. I recommend renaming those symbols to:
>>
>> IAllocator -> Allocator
>> theAllocator -> currentAllocator or tlsAllocator
>> it -> allocator or instance. Even better if a static "opDispatch" could be used to forward all methods to the instance.
>
>
> https://github.com/D-Programming-Language/phobos/commit/319f3297418c515a6d2e52e6e52d0f3f5895f587 changes .it to .instance for all allocators. -- Andrei

Why not .that? ;)

  Bit

Ok, as there has not been much attention here in last days, I will put a short summary of my own.

In general, I believe this is extremely high quality proposal and Andrei stands up to his reputation. The very design seems to fit with D idiomatics and it may change completely how people think about interaction with allocator library. I like that is focuses on robust API for building allocators instead of more "magic" to use those. That said, reviewing actual implementation quality is impossible until I have a chance to try it in real large scale project and I will not be trying to do that, assuming that Andrei is incapable of writing something completely terrible even if he tried to.

Pretty much all of my concerns are about documentation, API and clarity of intention. Quality of implementation won't matter if developers won't have a clear understanding on how to use it and existing proposals feels overwhelming unless one is willing to commit quite some time to research it.

Some notes:

1. I have already mentioned that there is neither module structure overview in `package.d` nor actual module structure. This is still the case for http://erdani.com/d/phobos-prerelease/std_experimental_allocator.html
2. `IAllocator` is defined inside `package.d` file. That means that it is impossible to use interface without import ALL of allocator modules
3. Same concern is about https://github.com/andralex/phobos/blob/allocator/std/experimental/allocator/package.d#L218-L228 - unless you actually import all stuff via package.d, those configured allocators are not available.
4. There are no higher level usage examples and/or guidelines about how this is supposed to fit in user applications. Intention behind the library may be familiar to users coming from C++ but target D audience is much more than that. Having std.allocator.showcase is nice but it is still a bit too theoretical.
5. http://erdani.com/d/phobos-prerelease/std_experimental_allocator_stats_collector.html has no overview documentation at all
6. Usage of ternary is not always clear / justified. In `IAllocator` it is explained and makes sense but there are things like http://erdani.com/d/phobos-prerelease/std_experimental_allocator_bitmapped_block.html ("Ternary empty() - Returns true iff no memory is currently allocated with this allocator"). I am still not sure why it is used there instead of simple boolean.

Overall opinion : I would surely vote for inclusion of this proposal in Phobos but in current shape it is not something I'd recommend to try to beginner/intermediate level D user.

On 6/21/15 4:47 AM, Dicebot wrote:
> 1. I have already mentioned that there is neither module structure
> overview in `package.d` nor actual module structure. This is still the
> case for
> http://erdani.com/d/phobos-prerelease/std_experimental_allocator.html
> 2. `IAllocator` is defined inside `package.d` file. That means that it
> is impossible to use interface without import ALL of allocator modules
> 3. Same concern is about
> https://github.com/andralex/phobos/blob/allocator/std/experimental/allocator/package.d#L218-L228
> - unless you actually import all stuff via package.d, those configured
> allocators are not available.
> 4. There are no higher level usage examples and/or guidelines about how
> this is supposed to fit in user applications. Intention behind the
> library may be familiar to users coming from C++ but target D audience
> is much more than that. Having std.allocator.showcase is nice but it is
> still a bit too theoretical.
> 5.
> http://erdani.com/d/phobos-prerelease/std_experimental_allocator_stats_collector.html
> has no overview documentation at all
> 6. Usage of ternary is not always clear / justified. In `IAllocator` it
> is explained and makes sense but there are things like
> http://erdani.com/d/phobos-prerelease/std_experimental_allocator_bitmapped_block.html
> ("Ternary empty() - Returns true iff no memory is currently allocated
> with this allocator"). I am still not sure why it is used there instead
> of simple boolean.
>
> Overall opinion : I would surely vote for inclusion of this proposal in
> Phobos but in current shape it is not something I'd recommend to try to
> beginner/intermediate level D user.

Thanks for the feedback. I'll get to work on this straight away. -- Andrei

On 6/13/15 4:16 PM, ZombineDev wrote:
> On Saturday, 13 June 2015 at 15:48:31 UTC, Andrei Alexandrescu wrote:
>> On 6/13/15 3:14 AM, Dicebot wrote:
>>> Andrei, have you considered creating additional std.allocator.impl
>>> package and moving actual allocators there? Or, probably, the other way
>>> around with std.allocator.core
>>>
>>> Existing flat hierarchy does not hint about internal structure in any
>>> way.
>>
>> It's good documentation, not directories, that helps understanding
>> internal structure. There are 23 files in std/experimental/allocator,
>> which seems manageable. I think we're good as we are.
>>
>> Andrei
>
> I also think putting some of the files in folder will make things
> cleaner and easier to understand.
>
> These files:
> std.experimental.allocator.affix_allocator,
> std.experimental.allocator.allocator_list,
> std.experimental.allocator.bucketizer,
> std.experimental.allocator.fallback_allocator,
> std.experimental.allocator.free_list,
> std.experimental.allocator.free_tree,
> std.experimental.allocator.gc_allocator,
> std.experimental.allocator.bitmapped_block,
> std.experimental.allocator.kernighan_ritchie,
> std.experimental.allocator.mallocator,
> std.experimental.allocator.mmap_allocator,
> std.experimental.allocator.null_allocator,
> std.experimental.allocator.quantizer,
> std.experimental.allocator.region,
> std.experimental.allocator.segregator,
> std.experimental.allocator.stats_collector;
>
> are great candidates for a std.experimental.allocator.building_blocks
> folder.

Moved a bunch of these into building_blocks:

https://github.com/andralex/phobos/commit/14ccc3a02f3dd332cb435abaf3c35cb8847797c0

However, I kept gc_allocator, mallocator, and mmap_allocator outside of it as "core" allocator that doesn't depend on any other.


Andrei

Can I use these allocators in @nogc code too ?

On 6/21/15 4:47 AM, Dicebot wrote:
> 1. I have already mentioned that there is neither module structure
> overview in `package.d` nor actual module structure. This is still the
> case for
> http://erdani.com/d/phobos-prerelease/std_experimental_allocator.html

Improved: http://erdani.com/d/phobos-prerelease/std_experimental_allocator.html

> 2. `IAllocator` is defined inside `package.d` file. That means that it
> is impossible to use interface without import ALL of allocator modules

Fixed, now interested users need to import std.experimental.allocator.building_blocks.

> 3. Same concern is about
> https://github.com/andralex/phobos/blob/allocator/std/experimental/allocator/package.d#L218-L228
> - unless you actually import all stuff via package.d, those configured
> allocators are not available.

Fixed per above.

> 4. There are no higher level usage examples and/or guidelines about how
> this is supposed to fit in user applications. Intention behind the
> library may be familiar to users coming from C++ but target D audience
> is much more than that. Having std.allocator.showcase is nice but it is
> still a bit too theoretical.

Coming soon.

> 5.
> http://erdani.com/d/phobos-prerelease/std_experimental_allocator_stats_collector.html
> has no overview documentation at all

Coming soon.

> 6. Usage of ternary is not always clear / justified. In `IAllocator` it
> is explained and makes sense but there are things like
> http://erdani.com/d/phobos-prerelease/std_experimental_allocator_bitmapped_block.html
> ("Ternary empty() - Returns true iff no memory is currently allocated
> with this allocator"). I am still not sure why it is used there instead
> of simple boolean.

Coming soon.


Andrei

On 6/21/15 4:47 AM, Dicebot wrote:
> 4. There are no higher level usage examples and/or guidelines about how
> this is supposed to fit in user applications. Intention behind the
> library may be familiar to users coming from C++ but target D audience
> is much more than that. Having std.allocator.showcase is nice but it is
> still a bit too theoretical.

Done: https://github.com/D-Programming-Language/phobos/commit/fd7689ef1369e55252d2f6cfa5baddd5260aeb13

Will be coming forth with 5 and 6.

Andrei