Re: Phobos addition formal review: std.experimental.allocator
Something I thought about today, if a class or struct is allocated by an allocator(say a malloc based allocator) and one of its fields is populated with a reference to GCed memory, will the GC know to scan the allocator memory to keep that GCed memory alive?
Re: Phobos addition formal review: std.experimental.allocator
On Thursday, 13 August 2015 at 21:05:28 UTC, rsw0x wrote: On Thursday, 13 August 2015 at 20:56:36 UTC, Tofu Ninja wrote: Something I thought about today, if a class or struct is allocated by an allocator(say a malloc based allocator) and one of its fields is populated with a reference to GCed memory, will the GC know to scan the allocator memory to keep that GCed memory alive? http://dlang.org/phobos/core_memory.html#.GC.addRange Yeah I know about that, I was just asking if all the allocators would be marked to be scanned by default. Or if they might try to only mark as needing to be scanned if they contain types that could contain references. The information of which types have internal references seems to be available at least to the GC. Theoretically an allocator could use that information to mark its allocations as needing to be scanned as well. Or it could be a wrapper allocator that simply marks things as needing to be scanned when they need to be, that way it would be an opt-in type deal. just some thoughts...
Re: Phobos addition formal review: std.experimental.allocator
On Thursday, 13 August 2015 at 20:56:36 UTC, Tofu Ninja wrote: Something I thought about today, if a class or struct is allocated by an allocator(say a malloc based allocator) and one of its fields is populated with a reference to GCed memory, will the GC know to scan the allocator memory to keep that GCed memory alive? http://dlang.org/phobos/core_memory.html#.GC.addRange
Re: Phobos addition formal review: std.experimental.allocator
On 2015-07-12 18:10, Dmitry Olshansky wrote: But then a Parent allocator would like to know if the child actually doesn't support reallocate vs say failing to reallocate. In the former case a lot of extra logic is simply removed at compile-time. Perhaps this is a simplified view, but most cases there were no difference between failing to reallocate or does not support reallocate. deallocate, for example, was used like this: static if (hasMember!(deallocate)) deallocate(); No else, no action was taken when it didn't support deallocate. I haven't look through the whole source code. -- /Jacob Carlborg
Re: Phobos addition formal review: std.experimental.allocator
On 2015-07-13 20:52, Andrei Alexandrescu wrote: That design would have been possible, e.g. have deallocate return false, owns return Ternary.unknown, expand return false, alignedAllocate return null etc. Perhaps it's a simplified view, but for all examples in the talk it seems it would work without changing the API. The branch which was executed for does not support this operation was the same for this operation failed. Example: static if (hasMember!(deallocate)) deallocate(); There was no else, no action when deallocate was not supported. Or: static if (hasMember!(reallocate)) return reallocate(); else return false; Of course I haven't look through the whole source code. I see the following issues with that design: 1. Many incorrect or grossly unfit allocators can be statically defined, for example a FallbackAllocator putting in the front an allocator that has a dummy implementation of owns. Unittesting could be expected to reasonably get rid of most. But there are many byzantine failure modes that are likely to escape even thorough unittesting. Consider, for example, building a Segregator out of several allocators, most of which support alignedAllocate, but some that don't (i.e. return null). During testing, if only the supported size ranges are tested, it all works. In production, if the wrong size is asked for in alignedAllocate, the allocator will return null as if the system ran out of memory. 2. Efficiency becomes increasingly tenuous. Simple cases of calls to do-nothing functions can be reasonably expected to be handled by the optimizer, but e.g. AllocatorList does significant work in owns, deallocate, deallocateAll, etc. - all under the assumption that the parent does implement the expected functionality. Emery Berger told me this was a constant source of concern with HeapLayers; he'd need to look at disassembly and tweak code in various ways to obtain the desired inlining, which when absent would cause dramatic slowdowns. Compiler technology has improved since that work, but also Heap Building Blocks go quite a longer distance than HeapLayers. With the DbI approach, the performance profile of a composite allocator is immediate and obvious. Yeah, my suggestion assumes the compiler can optimize away all these dummy functions. -- /Jacob Carlborg
Re: Phobos addition formal review: std.experimental.allocator
On 7/14/15 7:35 AM, Jacob Carlborg wrote: On 2015-07-13 20:52, Andrei Alexandrescu wrote: That design would have been possible, e.g. have deallocate return false, owns return Ternary.unknown, expand return false, alignedAllocate return null etc. Perhaps it's a simplified view, but for all examples in the talk it seems it would work without changing the API. The branch which was executed for does not support this operation was the same for this operation failed. Example: static if (hasMember!(deallocate)) deallocate(); There was no else, no action when deallocate was not supported. Or: static if (hasMember!(reallocate)) return reallocate(); else return false; I explained matters at length in the post you're replying to, and from what I can tell this reply is a reiteration of your same point. I don't know what to add - you may want to pay some more mind to that post. Of course I haven't look through the whole source code. One easy way to take a look at things is to clone the branch and then: git grep --context=5 hasMember or however many lines you need. Yeah, my suggestion assumes the compiler can optimize away all these dummy functions. There were two other considerations. Andrei
Re: Phobos addition formal review: std.experimental.allocator
On 7/12/15 11:08 AM, 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 just looked at Andrei's dconf talk and started to look at the source code how this hasMember is used. To me it looks like in most cases the optional methods, like deallocate, owns, expand and so on, could instead be required methods. Allocators that don't support these methods would need to have dummy implementations. But at the same time it would be easier for the user of the allocators, doesn't need to use all these static if. That design would have been possible, e.g. have deallocate return false, owns return Ternary.unknown, expand return false, alignedAllocate return null etc. I see the following issues with that design: 1. Many incorrect or grossly unfit allocators can be statically defined, for example a FallbackAllocator putting in the front an allocator that has a dummy implementation of owns. Unittesting could be expected to reasonably get rid of most. But there are many byzantine failure modes that are likely to escape even thorough unittesting. Consider, for example, building a Segregator out of several allocators, most of which support alignedAllocate, but some that don't (i.e. return null). During testing, if only the supported size ranges are tested, it all works. In production, if the wrong size is asked for in alignedAllocate, the allocator will return null as if the system ran out of memory. 2. Efficiency becomes increasingly tenuous. Simple cases of calls to do-nothing functions can be reasonably expected to be handled by the optimizer, but e.g. AllocatorList does significant work in owns, deallocate, deallocateAll, etc. - all under the assumption that the parent does implement the expected functionality. Emery Berger told me this was a constant source of concern with HeapLayers; he'd need to look at disassembly and tweak code in various ways to obtain the desired inlining, which when absent would cause dramatic slowdowns. Compiler technology has improved since that work, but also Heap Building Blocks go quite a longer distance than HeapLayers. With the DbI approach, the performance profile of a composite allocator is immediate and obvious. 3. Layout decisions cannot be made with dummy implementations. Currently there are no layout decisions in std.allocator that depend on presence of members, but ScopedAllocator came very closed to such. (There are many layout decisions that depend on instance size.) Going with a dynamic approach would preclude data layout decisions. Andrei
Re: Phobos addition formal review: std.experimental.allocator
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 just looked at Andrei's dconf talk and started to look at the source code how this hasMember is used. To me it looks like in most cases the optional methods, like deallocate, owns, expand and so on, could instead be required methods. Allocators that don't support these methods would need to have dummy implementations. But at the same time it would be easier for the user of the allocators, doesn't need to use all these static if. The current way of using hasMember and static if seems to be a more complicated design. -- /Jacob Carlborg
Re: Phobos addition formal review: std.experimental.allocator
On 12-Jul-2015 18:08, 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 just looked at Andrei's dconf talk and started to look at the source code how this hasMember is used. To me it looks like in most cases the optional methods, like deallocate, owns, expand and so on, could instead be required methods. Allocators that don't support these methods would need to have dummy implementations. But at the same time it would be easier for the user of the allocators, doesn't need to use all these static if. But then a Parent allocator would like to know if the child actually doesn't support reallocate vs say failing to reallocate. In the former case a lot of extra logic is simply removed at compile-time. -- Dmitry Olshansky
Re: Phobos addition formal review: std.experimental.allocator
On Friday, 12 June 2015 at 11:06:43 UTC, Dicebot wrote: The legendary allocator package by Andrei Alexandrescu has arrived at your doorsteps and kindly asks to let it into Phobos Sorry for being late, I wanted to restate an idea that could be crucial for optimizations. https://github.com/D-Programming-Language/druntime/pull/1183#issuecomment-77065554 This reminds me of the fact, that the IAllocator interface (std.allocator) should also have a strongly pure allocate function. This might allow the compiler to optimize allocations even with a dynamic dispatch allocator, because it assumes that an allocation doesn't have a side-effect and always returns fresh unaliased memory. Chandler Carruth was talking about this problem at the last CppCon. https://www.youtube.com/watch?v=fHNmRkzxHWst=3950 https://www.youtube.com/watch?v=fHNmRkzxHWst=4037 Now putting pure onto the allocate function of IAllocator would be a very harsh constraint for any implementation. So maybe we could employ a compiler hack, marking any IAllocator.allocate implementation as pure. This optimization is really important, b/c it allows the compiler to ellide allocations when it can compute something using a temporary buffer. It'd probably require a magic free as well, so that the compiler knows the lifetime and sees that data isn't escaped.
Re: Phobos addition formal review: std.experimental.allocator
On Tuesday, 30 June 2015 at 06:07:14 UTC, Baz wrote: On Friday, 26 June 2015 at 15:23:25 UTC, Alex Parrill wrote: On Friday, 26 June 2015 at 14:56:21 UTC, Dmitry Olshansky wrote: [...] Yea, VirtualAlloc seems like a better fit. (I don't actually know the windows API that well) [...] Here's the paragraph I'm reading: Mapped views of a file mapping object maintain internal references to the object, and a file mapping object does not close until all references to it are released. Therefore, to fully close a file mapping object, an application must unmap all mapped views of the file mapping object by calling UnmapViewOfFile and close the file mapping object handle by calling CloseHandle. These functions can be called in any order. https://github.com/andralex/phobos/pull/17 By the way, the ddoc comment for mmap needs to be updated (not posix only). I don't know what to write because initially i just wanted to build allocator under win and proposed to the mmap allocator for win without thinking more... currently under win the only difference between a simple malloc is that the allocation happen on commitment...previously it was really an anonymous memory mapped file...).
Re: Phobos addition formal review: std.experimental.allocator
On Friday, 26 June 2015 at 15:23:25 UTC, Alex Parrill wrote: On Friday, 26 June 2015 at 14:56:21 UTC, Dmitry Olshansky wrote: [...] Yea, VirtualAlloc seems like a better fit. (I don't actually know the windows API that well) [...] Here's the paragraph I'm reading: Mapped views of a file mapping object maintain internal references to the object, and a file mapping object does not close until all references to it are released. Therefore, to fully close a file mapping object, an application must unmap all mapped views of the file mapping object by calling UnmapViewOfFile and close the file mapping object handle by calling CloseHandle. These functions can be called in any order. https://github.com/andralex/phobos/pull/17
Re: Phobos addition formal review: std.experimental.allocator
On Friday, 26 June 2015 at 13:35:34 UTC, Dicebot wrote: It is ok to use that time for any additional comments :P I would second the suggestion for improvement in the documentation. The overview linked to in the original post is written assuming advanced programming knowledge. My understanding of allocation is pretty much limited to something could be on stack or heap (I had to Google what an allocator was). So I would say it could be improved by adding some explanation of the basics and why to use allocators instead of new or other techniques. I had seen this when it was originally posted two weeks ago, but I came back around to read it again today. What brought me to look it over again was that I had come across the std.container.array package and I didn't understand how it was different from the built-in array (I still don't have a very good understanding there, but a little better). Anyway, as I was searching through the Learn forum, I came across a post from 2011 saying that std.container might change in the future based on Andrei's work on allocators. I take it this is the work it was referring to (especially after looking at the wikipedia page for allocators). With that I think I only have two questions. Am I correct that the makeArray function is for allocating a dynamic array and not an std.container Array? Are there plans to refactor std.container with std.allocator in mind?
Re: Phobos addition formal review: std.experimental.allocator
The Windows MMap allocator only keeps one HANDLE around, and creates a new one on each `allocate`. Thus, `deallocate` closes the latest handle, regardless of what it was actually passed, so it leaks. If I'm reading the docs for `CreateFileMapping` right, you should be able to close the handle after calling `MapViewOfFile`; the internal data will persist until you unmap the memory region.
Re: Phobos addition formal review: std.experimental.allocator
On Friday, 26 June 2015 at 14:56:21 UTC, Dmitry Olshansky wrote: On 26-Jun-2015 17:51, Alex Parrill wrote: The Windows MMap allocator only keeps one HANDLE around, and creates a new one on each `allocate`. Thus, `deallocate` closes the latest handle, regardless of what it was actually passed, so it leaks. Actually I don't see why Windows couldnt' just use VirtualAlloc w/o messing with files. Yea, VirtualAlloc seems like a better fit. (I don't actually know the windows API that well) If I'm reading the docs for `CreateFileMapping` right, you should be able to close the handle after calling `MapViewOfFile`; the internal data will persist until you unmap the memory region. IIRC no you can't. I'd need to double check that though. Here's the paragraph I'm reading: Mapped views of a file mapping object maintain internal references to the object, and a file mapping object does not close until all references to it are released. Therefore, to fully close a file mapping object, an application must unmap all mapped views of the file mapping object by calling UnmapViewOfFile and close the file mapping object handle by calling CloseHandle. These functions can be called in any order.
Re: Phobos addition formal review: std.experimental.allocator
On 26-Jun-2015 17:51, Alex Parrill wrote: The Windows MMap allocator only keeps one HANDLE around, and creates a new one on each `allocate`. Thus, `deallocate` closes the latest handle, regardless of what it was actually passed, so it leaks. Actually I don't see why Windows couldnt' just use VirtualAlloc w/o messing with files. If I'm reading the docs for `CreateFileMapping` right, you should be able to close the handle after calling `MapViewOfFile`; the internal data will persist until you unmap the memory region. IIRC no you can't. I'd need to double check that though. -- Dmitry Olshansky
Re: Phobos addition formal review: std.experimental.allocator
On Thursday, 25 June 2015 at 13:36:31 UTC, Dicebot wrote: 1 day remaining FYI: considering http://forum.dlang.org/thread/mmhjqe$2mud$1...@digitalmars.com there will be some delay between finishing the review period and starting voting process. It is ok to use that time for any additional comments :P
Re: Phobos addition formal review: std.experimental.allocator
1 day remaining
Re: Phobos addition formal review: std.experimental.allocator
On Wednesday, 24 June 2015 at 04:00:11 UTC, Andrei Alexandrescu wrote: std.allocator.allocator.IAllocator std.allocator.allocator.theAllocator; Yep, ridiculous is the first thing that comes to mind. I find it difficult to digest (ehm) the fact that the same community that thinks std.allocator is just not going to cut the mustard, simultaneously believes std.allocator.allocator is a good idea. Of course you are right. It was indeed a poor suggestion. But it's the only example I could find where a separate module name was used to declare the high-level API. Perhaps it was even written before package.d existed. The case against std.allocator isn't about the name, but about the fact that it doesn't do what people, apparently, expect: import the entire public API. I count only 4 uses of package.d in official Phobos: https://github.com/D-Programming-Language/phobos/blob/master/std/algorithm/package.d https://github.com/D-Programming-Language/phobos/blob/master/std/container/package.d https://github.com/D-Programming-Language/phobos/blob/master/std/range/package.d https://github.com/D-Programming-Language/phobos/blob/master/std/regex/package.d All of them seem to import the entire public API, but only std.algorithm.package.d is exclusively used for public imports. The others all have some definition of high-level API features. So, I don't see a precedent one way or another. And maybe that's how it should be: defined based on the intended use of the package. Unfortunately, those against using package.d for the high-level API, did not seem to offer a suggestion for where to put your porcelain. So, if you wish to accommodate them and need a name, consider std.allocator.api. Mike
Re: Phobos addition formal review: std.experimental.allocator
On 6/23/15 9:53 AM, extrawurst wrote: On Tuesday, 23 June 2015 at 16:49:45 UTC, Andrei Alexandrescu wrote: On 6/23/15 9:48 AM, extrawurst wrote: I agree with Adam on this: Just a quick concern, I don't think a package.d should ever have anything except imports in it (see http://forum.dlang.org/post/qwatonmpnoyjsvzjp...@forum.dlang.org) What is the rationale? -- Andrei I simply quote his following post in the mentioned thread: The biggest benefit of breaking up the big modules is so you can access some of it without requiring all of it. But when the part you want is in the package.d, you can't get it independently anymore; importing that also imports everything else, negating the reason it was split up in the first place. But that doesn't apply to packages that do NOT originate as big modules, so they have no backward compatibility issue. This is the case for std.allocator. I see a net pessimization for everyone involved to change the current packaging. All I'm saying is we shouldn't take it noncritically that packages should be done one particular way. Andrei
Re: Phobos addition formal review: std.experimental.allocator
On Tuesday, 23 June 2015 at 16:49:45 UTC, Andrei Alexandrescu wrote: On 6/23/15 9:48 AM, extrawurst wrote: I agree with Adam on this: Just a quick concern, I don't think a package.d should ever have anything except imports in it (see http://forum.dlang.org/post/qwatonmpnoyjsvzjp...@forum.dlang.org) What is the rationale? -- Andrei My reasoning is simple : when `package.d` only contains public imports it allows to both use simple `import std.allocator` for those who want to get started quickly and pick only necessary imports for those who try optimizing build times / code clarity. Not doing that does not change anything for those who prefer `import std.allocator` but makes fine tuning of imports impossible. Thus former approach looks either equal or superior for all use cases.
Re: Phobos addition formal review: std.experimental.allocator
On Tuesday, 23 June 2015 at 16:56:55 UTC, Andrei Alexandrescu wrote: But that doesn't apply to packages that do NOT originate as big modules, so they have no backward compatibility issue. My thought isn't really about backward compatibility but about minimizing dependencies with sibling modules. I don't want to repeat my argument too much from the other thread, but imagine you're writing a minimalist library that is meant to interact with other minimalist libraries. To interact, you want a shared interface and basic types. But to be minimalist, you want to pull as little other standard code as possible. The typical user might just import std.whatever and get it all available. But this minimalist user only wants std.whatever.basic_interface. If the basic interface is shoved inside package.d, she can't get get to it without inadvertently pulling in more modules too. This can quickly grow into a web of dependencies where importing just an interface definition ends up grabbing dozens if implementations you don't want too, bloating compile times and binary sizes.
Re: Phobos addition formal review: std.experimental.allocator
On 6/23/15 2:18 AM, Dicebot wrote: On Monday, 22 June 2015 at 22:38:19 UTC, Andrei Alexandrescu wrote: Perhaps I misunderstood the request - currently the imports in allocator/package.d are: public import std.experimental.allocator.common, std.experimental.allocator.typed; import std.algorithm, std.conv, std.exception, std.range, std.traits, std.typecons, std.typetuple; version(unittest) import std.random, std.stdio; Is that okay, and if not what should change? My concern was about the fact that symbols `IAllocator`, `theAllocator`, `processAllocator` and bunch of others are defined within `package.d` itself and not provided via public import. That means that anyone willing to change default allocator or use `make` MUST import all std.allocator modules even if nothing else is really needed (those utilities look very independent). Which means processing bunch of unnecessary imports - and more of those if more modules will get added to the package. I'd prefer to have a small dedicated module, i.e. `std.allocator.api` (not going to discuss names!) and do public import of it from `std.allocator.package.d`. Makes sense? I see. Well this raises the question whether importing std.xyz automatically means everything under std.xyz is transitorily imported. Right now it's not - the more advanced/obscure building blocks are not automatically imported if you just import std.allocator; for those you'd need to import the std.allocator.building_blocks package, or individual modules from it like std.allocator.building_blocks.region. This idea was proposed by Mike on 2015/05/18: Would it be better to move the porcelain code into package.d so the nomenclature simply becomes `import std.allocator;`? I liked the idea that someone who's not an expert goes let me import std.allocator and call it a day whereas someone who wants to tweak, customize, etc. they'd need to be more specific. If we define an entry point such as std.allocator.api, novices will have one more thing to remember, or some will still import std.allocator thus pulling everything without realizing it. So I'd say let's keep simple things simple. If you want to allocate, import std.allocator. Andrei
Re: Phobos addition formal review: std.experimental.allocator
Am 23.06.2015 um 18:53 schrieb extrawurst: On Tuesday, 23 June 2015 at 16:49:45 UTC, Andrei Alexandrescu wrote: On 6/23/15 9:48 AM, extrawurst wrote: I agree with Adam on this: Just a quick concern, I don't think a package.d should ever have anything except imports in it (see http://forum.dlang.org/post/qwatonmpnoyjsvzjp...@forum.dlang.org) What is the rationale? -- Andrei I simply quote his following post in the mentioned thread: The biggest benefit of breaking up the big modules is so you can access some of it without requiring all of it. But when the part you want is in the package.d, you can't get it independently anymore; importing that also imports everything else, negating the reason it was split up in the first place. Not to mention https://issues.dlang.org/show_bug.cgi?id=11847
Re: Phobos addition formal review: std.experimental.allocator
On 6/23/15 9:48 AM, extrawurst wrote: I agree with Adam on this: Just a quick concern, I don't think a package.d should ever have anything except imports in it (see http://forum.dlang.org/post/qwatonmpnoyjsvzjp...@forum.dlang.org) What is the rationale? -- Andrei
Re: Phobos addition formal review: std.experimental.allocator
On Tuesday, 23 June 2015 at 15:56:15 UTC, Andrei Alexandrescu wrote: On 6/23/15 2:18 AM, Dicebot wrote: On Monday, 22 June 2015 at 22:38:19 UTC, Andrei Alexandrescu wrote: Perhaps I misunderstood the request - currently the imports in allocator/package.d are: public import std.experimental.allocator.common, std.experimental.allocator.typed; import std.algorithm, std.conv, std.exception, std.range, std.traits, std.typecons, std.typetuple; version(unittest) import std.random, std.stdio; Is that okay, and if not what should change? My concern was about the fact that symbols `IAllocator`, `theAllocator`, `processAllocator` and bunch of others are defined within `package.d` itself and not provided via public import. That means that anyone willing to change default allocator or use `make` MUST import all std.allocator modules even if nothing else is really needed (those utilities look very independent). Which means processing bunch of unnecessary imports - and more of those if more modules will get added to the package. I'd prefer to have a small dedicated module, i.e. `std.allocator.api` (not going to discuss names!) and do public import of it from `std.allocator.package.d`. Makes sense? I see. Well this raises the question whether importing std.xyz automatically means everything under std.xyz is transitorily imported. Right now it's not - the more advanced/obscure building blocks are not automatically imported if you just import std.allocator; for those you'd need to import the std.allocator.building_blocks package, or individual modules from it like std.allocator.building_blocks.region. This idea was proposed by Mike on 2015/05/18: Would it be better to move the porcelain code into package.d so the nomenclature simply becomes `import std.allocator;`? I liked the idea that someone who's not an expert goes let me import std.allocator and call it a day whereas someone who wants to tweak, customize, etc. they'd need to be more specific. If we define an entry point such as std.allocator.api, novices will have one more thing to remember, or some will still import std.allocator thus pulling everything without realizing it. So I'd say let's keep simple things simple. If you want to allocate, import std.allocator. Andrei I agree with Adam on this: Just a quick concern, I don't think a package.d should ever have anything except imports in it (see http://forum.dlang.org/post/qwatonmpnoyjsvzjp...@forum.dlang.org) for convenience the package.d could still import a bunch of default stuff publicly for the normal user.
Re: Phobos addition formal review: std.experimental.allocator
On Tuesday, 23 June 2015 at 16:49:45 UTC, Andrei Alexandrescu wrote: On 6/23/15 9:48 AM, extrawurst wrote: I agree with Adam on this: Just a quick concern, I don't think a package.d should ever have anything except imports in it (see http://forum.dlang.org/post/qwatonmpnoyjsvzjp...@forum.dlang.org) What is the rationale? -- Andrei I simply quote his following post in the mentioned thread: The biggest benefit of breaking up the big modules is so you can access some of it without requiring all of it. But when the part you want is in the package.d, you can't get it independently anymore; importing that also imports everything else, negating the reason it was split up in the first place.
Re: Phobos addition formal review: std.experimental.allocator
On 6/23/15 8:56 AM, Andrei Alexandrescu wrote: I see. Well this raises the question whether importing std.xyz automatically means everything under std.xyz is transitorily imported. s/transitorily/transitively/
Re: Phobos addition formal review: std.experimental.allocator
On 6/23/15 10:16 AM, Dicebot wrote: On Tuesday, 23 June 2015 at 16:49:45 UTC, Andrei Alexandrescu wrote: On 6/23/15 9:48 AM, extrawurst wrote: I agree with Adam on this: Just a quick concern, I don't think a package.d should ever have anything except imports in it (see http://forum.dlang.org/post/qwatonmpnoyjsvzjp...@forum.dlang.org) What is the rationale? -- Andrei My reasoning is simple : when `package.d` only contains public imports it allows to both use simple `import std.allocator` for those who want to get started quickly and pick only necessary imports for those who try optimizing build times / code clarity. Not doing that does not change anything for those who prefer `import std.allocator` but makes fine tuning of imports impossible. Thus former approach looks either equal or superior for all use cases. The import std.allocator is already minimal - only contains the high level stuff. -- Andrei
Re: Phobos addition formal review: std.experimental.allocator
On 6/23/15 10:15 AM, Adam D. Ruppe wrote: On Tuesday, 23 June 2015 at 16:56:55 UTC, Andrei Alexandrescu wrote: But that doesn't apply to packages that do NOT originate as big modules, so they have no backward compatibility issue. My thought isn't really about backward compatibility but about minimizing dependencies with sibling modules. I don't want to repeat my argument too much from the other thread, but imagine you're writing a minimalist library that is meant to interact with other minimalist libraries. To interact, you want a shared interface and basic types. But to be minimalist, you want to pull as little other standard code as possible. The typical user might just import std.whatever and get it all available. But this minimalist user only wants std.whatever.basic_interface. If the basic interface is shoved inside package.d, she can't get get to it without inadvertently pulling in more modules too. This can quickly grow into a web of dependencies where importing just an interface definition ends up grabbing dozens if implementations you don't want too, bloating compile times and binary sizes. The case with std.allocator is not everything is imported in it, so no bloating no nothing. -- Andrei
Re: Phobos addition formal review: std.experimental.allocator
On 24-Jun-2015 00:12, Dicebot wrote: On Tuesday, 23 June 2015 at 19:17:21 UTC, Andrei Alexandrescu wrote: On 6/23/15 10:16 AM, Dicebot wrote: On Tuesday, 23 June 2015 at 16:49:45 UTC, Andrei Alexandrescu wrote: On 6/23/15 9:48 AM, extrawurst wrote: I agree with Adam on this: Just a quick concern, I don't think a package.d should ever have anything except imports in it (see http://forum.dlang.org/post/qwatonmpnoyjsvzjp...@forum.dlang.org) What is the rationale? -- Andrei My reasoning is simple : when `package.d` only contains public imports it allows to both use simple `import std.allocator` for those who want to get started quickly and pick only necessary imports for those who try optimizing build times / code clarity. Not doing that does not change anything for those who prefer `import std.allocator` but makes fine tuning of imports impossible. Thus former approach looks either equal or superior for all use cases. The import std.allocator is already minimal - only contains the high level stuff. -- Andrei So you have chosen worst of both worlds - neither give power users ability to fine tune imports nor allow casual users always go with `import std.allocator` and be happy? :) If anything, that will be the first package.d in Phobos (AFAIK) which won't feature public import of _all_ package modules. I'm not sure if that's the case with std.allocator but importing package IMHO should import _typical_ set of submodules. Things that are more niche and rare (power user oriented) shouldn't really be in package.d -- Dmitry Olshansky
Re: Phobos addition formal review: std.experimental.allocator
On Tuesday, 23 June 2015 at 21:12:10 UTC, Dicebot wrote: So you have chosen worst of both worlds - neither give power users ability to fine tune imports nor allow casual users always go with `import std.allocator` and be happy? :) If anything, that will be the first package.d in Phobos (AFAIK) which won't feature public import of _all_ package modules. I agree, a minimalist package.d seems utterly pointless to me.
Re: Phobos addition formal review: std.experimental.allocator
On Tuesday, 23 June 2015 at 19:17:21 UTC, Andrei Alexandrescu wrote: On 6/23/15 10:16 AM, Dicebot wrote: On Tuesday, 23 June 2015 at 16:49:45 UTC, Andrei Alexandrescu wrote: On 6/23/15 9:48 AM, extrawurst wrote: I agree with Adam on this: Just a quick concern, I don't think a package.d should ever have anything except imports in it (see http://forum.dlang.org/post/qwatonmpnoyjsvzjp...@forum.dlang.org) What is the rationale? -- Andrei My reasoning is simple : when `package.d` only contains public imports it allows to both use simple `import std.allocator` for those who want to get started quickly and pick only necessary imports for those who try optimizing build times / code clarity. Not doing that does not change anything for those who prefer `import std.allocator` but makes fine tuning of imports impossible. Thus former approach looks either equal or superior for all use cases. The import std.allocator is already minimal - only contains the high level stuff. -- Andrei So you have chosen worst of both worlds - neither give power users ability to fine tune imports nor allow casual users always go with `import std.allocator` and be happy? :) If anything, that will be the first package.d in Phobos (AFAIK) which won't feature public import of _all_ package modules.
Re: Phobos addition formal review: std.experimental.allocator
On Monday, 22 June 2015 at 22:38:19 UTC, Andrei Alexandrescu wrote: Perhaps I misunderstood the request - currently the imports in allocator/package.d are: public import std.experimental.allocator.common, std.experimental.allocator.typed; import std.algorithm, std.conv, std.exception, std.range, std.traits, std.typecons, std.typetuple; version(unittest) import std.random, std.stdio; Is that okay, and if not what should change? My concern was about the fact that symbols `IAllocator`, `theAllocator`, `processAllocator` and bunch of others are defined within `package.d` itself and not provided via public import. That means that anyone willing to change default allocator or use `make` MUST import all std.allocator modules even if nothing else is really needed (those utilities look very independent). Which means processing bunch of unnecessary imports - and more of those if more modules will get added to the package. I'd prefer to have a small dedicated module, i.e. `std.allocator.api` (not going to discuss names!) and do public import of it from `std.allocator.package.d`. Makes sense?
Re: Phobos addition formal review: std.experimental.allocator
On 6/23/15 6:11 PM, Mike wrote: It seems there is already precedent with std.digest.digest (http://dlang.org/phobos/std_digest_digest.html). I'm sorry I wasn't aware of this at the time. std.allocator.porcelain couldn't stand, and I suggested what seemed most natural to me. I've spend a number of years in C#-land, and that of course has an affect on my biases. But D is not C# (thankfully). Well it's about time we engineers stop suspending our sense of ridiculousness when looking at programmer names. Once some minimal sense and sensibility is in action, it's clear that something like std.digest.digest.digest is just ridiculous. That shouldn't have passed review, and today it shouldn't be used as a precedent. Following the std.digest.digest precedent, there should probably be std.allocator.allocator. It stinks IMO (std.digiest.api and std.allocator.api would have been much better), but the naming discussions are starting to take their toll. std.allocator.allocator.IAllocator std.allocator.allocator.theAllocator; Yep, ridiculous is the first thing that comes to mind. I find it difficult to digest (ehm) the fact that the same community that thinks std.allocator is just not going to cut the mustard, simultaneously believes std.allocator.allocator is a good idea. Anyway, I suggest std.allocator.allocator as a compromise and a precedent to follow in the future. It's tolerable. Also, I can't seem to navigate std.allocator tree in the left nav at http://erdani.com/d/phobos-prerelease/std_experimental_allocator.html. Please advise or fix. Yah, noticed that too. Will look into it tomorrow. Andrei
Re: Phobos addition formal review: std.experimental.allocator
On Tuesday, 23 June 2015 at 19:16:33 UTC, Andrei Alexandrescu wrote: The case with std.allocator is not everything is imported in it, so no bloating no nothing. -- Andrei My assumption with D libraries is that import std.thing; imports everything in the the whole package. Not that I've a strong opinion on it either.
Re: Phobos addition formal review: std.experimental.allocator
On Tuesday, 23 June 2015 at 21:12:10 UTC, Dicebot wrote: On Tuesday, 23 June 2015 at 19:17:21 UTC, Andrei Alexandrescu The import std.allocator is already minimal - only contains the high level stuff. -- Andrei So you have chosen worst of both worlds - neither give power users ability to fine tune imports nor allow casual users always go with `import std.allocator` and be happy? :) If anything, that will be the first package.d in Phobos (AFAIK) which won't feature public import of _all_ package modules. Well, since it was apparently my lousy suggestion that led to this, I guess I'm obligated to comment. It seems there is already precedent with std.digest.digest (http://dlang.org/phobos/std_digest_digest.html). I'm sorry I wasn't aware of this at the time. std.allocator.porcelain couldn't stand, and I suggested what seemed most natural to me. I've spend a number of years in C#-land, and that of course has an affect on my biases. But D is not C# (thankfully). Following the std.digest.digest precedent, there should probably be std.allocator.allocator. It stinks IMO (std.digiest.api and std.allocator.api would have been much better), but the naming discussions are starting to take their toll. Anyway, I suggest std.allocator.allocator as a compromise and a precedent to follow in the future. It's tolerable. Also, I can't seem to navigate std.allocator tree in the left nav at http://erdani.com/d/phobos-prerelease/std_experimental_allocator.html. Please advise or fix. Mike
Re: Phobos addition formal review: std.experimental.allocator
On 6/23/15 6:18 PM, Andrei Alexandrescu wrote: On 6/23/15 3:17 PM, Steven Schveighoffer wrote: we need to redo no Yeah, so then it becomes import std.somepackage means import all the modules in that package, except for std.allocator. But that's fine, if we want that type of inconsistency, we can just RTFM the newbies. -Steve
Re: Phobos addition formal review: std.experimental.allocator
Ok, if you feel so strongly about it, let's not waste time in arguing. You are not obliged to agree to any of reviewer comments so as long as everyone is clear about their opinion, the eventual voting will decide. I am simply pointing out things that catch my attention.
Re: Phobos addition formal review: std.experimental.allocator
On 6/23/15 2:12 PM, Dicebot wrote: So you have chosen worst of both worlds - neither give power users ability to fine tune imports nor allow casual users always go with `import std.allocator` and be happy? :) There is functionality in std.allocator to get anyone started who doesn't want to sit down and define their own allocator. People who want to do that can import std.allocator.building_blocks which is a package fully including all that's needed. If anything, that will be the first package.d in Phobos (AFAIK) which won't feature public import of _all_ package modules. I'm not afraid of creating precedent if that's the best way to go. Again: I have the impression we're blocked into an assumption we didn't take critically until now. Andrei
Re: Phobos addition formal review: std.experimental.allocator
On 6/23/15 2:29 PM, Dmitry Olshansky wrote: I'm not sure if that's the case with std.allocator but importing package IMHO should import _typical_ set of submodules. Things that are more niche and rare (power user oriented) shouldn't really be in package.d Yah, that attitude describes std.allocator well. -- Andrei
Re: Phobos addition formal review: std.experimental.allocator
On 6/23/15 2:14 PM, Tofu Ninja wrote: On Tuesday, 23 June 2015 at 21:12:10 UTC, Dicebot wrote: So you have chosen worst of both worlds - neither give power users ability to fine tune imports nor allow casual users always go with `import std.allocator` and be happy? :) If anything, that will be the first package.d in Phobos (AFAIK) which won't feature public import of _all_ package modules. I agree, a minimalist package.d seems utterly pointless to me. It's not minimalist. -- Andrei
Re: Phobos addition formal review: std.experimental.allocator
On 6/23/15 3:17 PM, Steven Schveighoffer wrote: we need to redo no
Re: Phobos addition formal review: std.experimental.allocator
On 6/23/15 6:07 PM, Andrei Alexandrescu wrote: On 6/23/15 2:12 PM, Dicebot wrote: So you have chosen worst of both worlds - neither give power users ability to fine tune imports nor allow casual users always go with `import std.allocator` and be happy? :) There is functionality in std.allocator to get anyone started who doesn't want to sit down and define their own allocator. People who want to do that can import std.allocator.building_blocks which is a package fully including all that's needed. If anything, that will be the first package.d in Phobos (AFAIK) which won't feature public import of _all_ package modules. I'm not afraid of creating precedent if that's the best way to go. Again: I have the impression we're blocked into an assumption we didn't take critically until now. What happened basically was: 1. There's too much crap in some modules, it's hard to maintain, hard to process. 2. Let's split it up! Oh, but some people want to be able to import everything a la java's import java.io.*, and existing code!!! 3. package.d is the solution, just import that and it's the same thing! I personally think that if we want to define package.d as importing all the submodules, then we should stick to that. If we want to go another direction, we need to redo all the other splitting of modules we have done so far, as that is the pattern. I'd recommend std.allocator.basic instead of std.allocator for a basic direction. If it doesn't make sense to import ALL of std.allocator ever, then drop package.d. But you don't have to listen to me :) -Steve
Re: Phobos addition formal review: std.experimental.allocator
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
Re: Phobos addition formal review: std.experimental.allocator
On Monday, 22 June 2015 at 19:51:50 UTC, Andrei Alexandrescu wrote: 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. Is it? I still see those symbols defined in package.d , 14ccc3a02f3dd332cb435abaf3c35cb8847797c0 does not seem to have affected that.
Re: Phobos addition formal review: std.experimental.allocator
On 6/21/15 4:47 AM, Dicebot wrote: 5. http://erdani.com/d/phobos-prerelease/std_experimental_allocator_stats_collector.html has no overview documentation at all https://github.com/D-Programming-Language/phobos/commit/ef6de891197db6d497df11c9350781eef38df196 Andrei
Re: Phobos addition formal review: std.experimental.allocator
On 6/22/15 2:40 PM, Dicebot wrote: On Monday, 22 June 2015 at 19:51:50 UTC, Andrei Alexandrescu wrote: 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. Is it? I still see those symbols defined in package.d , 14ccc3a02f3dd332cb435abaf3c35cb8847797c0 does not seem to have affected that. Perhaps I misunderstood the request - currently the imports in allocator/package.d are: public import std.experimental.allocator.common, std.experimental.allocator.typed; import std.algorithm, std.conv, std.exception, std.range, std.traits, std.typecons, std.typetuple; version(unittest) import std.random, std.stdio; Is that okay, and if not what should change? Andrei
Re: Phobos addition formal review: std.experimental.allocator
On 6/21/15 4:47 AM, Dicebot wrote: 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. Founds a bunch of issues with the docs indeed; switching the static interface from bool to Ternary (for the sake of unification with IAllocator) was a last-minute thing. https://github.com/D-Programming-Language/phobos/commit/16f5a5631cf1f2eeea3779785c3ea17682b8ff45 Andrei
Re: Phobos addition formal review: std.experimental.allocator
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
Re: Phobos addition formal review: std.experimental.allocator
Can I use these allocators in @nogc code too ?
Re: Phobos addition formal review: std.experimental.allocator
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
Re: Phobos addition formal review: std.experimental.allocator
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.
Re: Phobos addition formal review: std.experimental.allocator
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
Re: Phobos addition formal review: std.experimental.allocator
On Tue, 16 Jun 2015 16:29:08 -0400, Andrei Alexandrescu seewebsiteforem...@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
Re: Phobos addition formal review: std.experimental.allocator
Bringing back to first page.
Re: Phobos addition formal review: std.experimental.allocator
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
Re: Phobos addition formal review: std.experimental.allocator
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
Re: Phobos addition formal review: std.experimental.allocator
On Sunday, 14 June 2015 at 22:47:59 UTC, Brian Schott wrote: Not yet, but while looking for one I found that this code triggers an assertion in Region.expand() Running the above code with GCAllocator substituted for NullAllocator causes an InvalidMemoryOperationError after all 100 iterations of the loop have completed.
Re: Phobos addition formal review: std.experimental.allocator
On Sunday, 14 June 2015 at 00:24:51 UTC, Andrei Alexandrescu wrote: So we have: * 1 request to change names; * 3 requests to wank around the directory structure; * 0 of everything else. Sigh. Andrei How about this: AllocatorList.allocate() calls AllocatorList.owns(), but only if you don't compile with -release. Check the assert statement. AllocatorList.owns() is non-const, so the for loops of these two functions start rearranging the root pointers and end up in an infinite loop in debug builds.
Re: Phobos addition formal review: std.experimental.allocator
On 6/14/15 4:00 AM, Brian Schott wrote: On Sunday, 14 June 2015 at 00:24:51 UTC, Andrei Alexandrescu wrote: So we have: * 1 request to change names; * 3 requests to wank around the directory structure; * 0 of everything else. Sigh. Andrei How about this: AllocatorList.allocate() calls AllocatorList.owns(), but only if you don't compile with -release. Check the assert statement. AllocatorList.owns() is non-const, so the for loops of these two functions start rearranging the root pointers and end up in an infinite loop in debug builds. Interesting. Do you have a repro for this? Thanks! -- Andrei
Re: Phobos addition formal review: std.experimental.allocator
On 2015-06-14 02:24, Andrei Alexandrescu wrote: * 1 request to change names; * 3 requests to wank around the directory structure; * 0 of everything else. Sigh. These are things that are easy to see right away, without looking deep inside the code. You can interpret this as: 1. Either your implementation is already good enough 2. Or people haven't started to look at the implementation yet and are commenting on the things that they is right away Count me on the request to change the directory structure :) -- /Jacob Carlborg
Re: Phobos addition formal review: std.experimental.allocator
On Sunday, 14 June 2015 at 00:24:51 UTC, Andrei Alexandrescu wrote: So we have: * 1 request to change names; * 3 requests to wank around the directory structure; * 0 of everything else. Sigh. That is to be expected and intended for formal Phobos review. Implementation is not of much interest - it can be fixed at any point. Most important thing is to ensure that API feels right, documentation feels clear and people are in general comfortable with using proposed modules as they are. I will do more in-depth review but it will _all_ be about API and docs and naming.
Re: Phobos addition formal review: std.experimental.allocator
So we have: * 1 request to change names; * 3 requests to wank around the directory structure; * 0 of everything else. Sigh. Andrei Yesterday I have found an error in documentation and have left a comment about it on github PR. And I only start to read the documentation, so I think there will be plenty small things to polish.
Re: Phobos addition formal review: std.experimental.allocator
On 6/13/15 11:49 PM, Dicebot wrote: On Sunday, 14 June 2015 at 00:24:51 UTC, Andrei Alexandrescu wrote: So we have: * 1 request to change names; * 3 requests to wank around the directory structure; * 0 of everything else. Sigh. That is to be expected and intended for formal Phobos review. Implementation is not of much interest - it can be fixed at any point. Most important thing is to ensure that API feels right, documentation feels clear and people are in general comfortable with using proposed modules as they are. I will do more in-depth review but it will _all_ be about API and docs and naming. Suggestions for better names are welcome as addenda, and I will act on some, but they're no substitute for competent reviews. We need as a community to learn how to do good reviews. Anyone can put a finger on a name of a thing and say they like another name better. ANYONE. Real review of a library is figuring out how well the proposed library's abstractions fulfill its charter and intended use cases. Andrei
Re: Phobos addition formal review: std.experimental.allocator
On Sunday, 14 June 2015 at 14:30:00 UTC, Andrei Alexandrescu
wrote:
Interesting. Do you have a repro for this? Thanks! -- Andrei
Not yet, but while looking for one I found that this code
triggers an assertion in Region.expand()
```
module test;
import std.experimental.allocator;
import std.stdio;
private alias AllocatorType = CAllocatorImpl!(AllocatorList!(
n = Region!Mallocator(1024 * 4), NullAllocator));
struct Big {
ubyte[1024] bytes;
}
void main(string[] args){
auto a = new AllocatorType;
foreach (i; 0 .. 100) {
a.make!Big();
writeln(i);
}
}
```
Re: Phobos addition formal review: std.experimental.allocator
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. So we have: * 1 request to change names; * 3 requests to wank around the directory structure; * 0 of everything else. Sigh. Andrei
Re: Phobos addition formal review: std.experimental.allocator
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.
Re: Phobos addition formal review: std.experimental.allocator
On Sunday, 14 June 2015 at 00:24:51 UTC, Andrei Alexandrescu wrote: 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. So we have: * 1 request to change names; * 3 requests to wank around the directory structure; * 0 of everything else. Sigh. Andrei can I make a request to just get it the hell in std.experimental for 2.068?
Re: Phobos addition formal review: std.experimental.allocator
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
Re: Phobos addition formal review: std.experimental.allocator
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 haven't seen overview in documentation either
Re: Phobos addition formal review: std.experimental.allocator
On 6/13/15 10:24 AM, Dicebot 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 haven't seen overview in documentation either Yah, that should go in package.d. It's reasonable to mark that as an acceptance requirement even for experimental. Will get into it soon. -- Andrei
Re: Phobos addition formal review: std.experimental.allocator
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.
Re: Phobos addition formal review: std.experimental.allocator
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. -- /Jacob Carlborg
Re: Phobos addition formal review: std.experimental.allocator
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 Personally, I disagree, the flat hierarchy was the first thing I noticed and I thought the same thing as Dicebot. And its 23 now, but what about when new allocators get added. I don't think there is really a cost to moving it to a new folder, just makes things clearer. Any ways isn't the flat hierarchy something people have been complaining about Phobos in general?
Re: Phobos addition formal review: std.experimental.allocator
Also wanted to apologize for authors of any other Phobos proposals - I did intend to step down from review manager role but this specific package is of huge personal interest to me, thus the exception has been made.
Re: Phobos addition formal review: std.experimental.allocator
On Friday, 12 June 2015 at 11:06:43 UTC, 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 This proposal will undergo the review/inclusion process similar to std.experimental.logger : 1. 2 week review/discussion period 2. if proposal author feels confident, 2 week voting period for accepting into sts.experimental 3. once enough field trial feedback has been accumulated, vote for moving into std mainline during the upcoming release beta period If reviewing find some fundamental issues or voting will fail, the whole process will repeats again per proposal authors request. Please start destruction, review period ends on 26.06 (23.59 +0 Greenwich) Great work :) It would be nice if someone could write some tests/benchmarks in order to show for each allocator where it works well over others.
Re: Phobos addition formal review: std.experimental.allocator
On Friday, 12 June 2015 at 19:19:55 UTC, Andrei Alexandrescu wrote: On 6/12/15 11:23 AM, Baz wrote: On Friday, 12 June 2015 at 11:09:01 UTC, Dicebot wrote: Small tip for reviewers: there are quite many modules in proposed package but majority is actual allocator implementation. I'd suggest to start investigating sources/documentation starting from http://erdani.com/d/phobos-prerelease/std_experimental_allocator.html and http://erdani.com/d/phobos-prerelease/std_experimental_allocator_building_blocks.html while using http://erdani.com/d/phobos-prerelease/std_experimental_allocator_showcase.html as reference for highlight examples. while building the package as a static lic library, i've found that _showcase.d_ is not usable under Windows because MmapAllocator is Posix only: https://github.com/andralex/phobos/blob/allocator/std/experimental/allocator/showcase.d#L66 https://github.com/andralex/phobos/blob/allocator/std/experimental/allocator/mmap_allocator.d#L14 I've asked this a while ago as well - could anyone with a Windows rig contribute a MmapAllocator for Windows? Thanks! -- Andrei I have done it (https://gist.github.com/BBasile/e382be91dcc18e4d2358), but if i run the tests of the showcase i get: --- ...\region.d(84): Error: shared method std.experimental.allocator.mmap_allocator.MmapAllocator.allocate is not callable using a non-shared object ...\showcase.d(73): Error: template instance std.experimental.allocator.region.Region!(MmapAllocator, 8u, cast(Flag)false) error instantiating ...\allocator_list.d(76): Error: struct Factory does not overload () ...\showcase.d(79): Error: template instance std.experimental.allocator.allocator_list.AllocatorList!(Factory, NullAllocator) error instantiating ...\region.d(84,29): Error: shared method std.experimental.allocator.mmap_allocator.MmapAllocator.allocate is not callable using a non-shared object ...\showcase.d(73,20): Error: template instance std.experimental.allocator.region.Region!(MmapAllocator, 8u, cast(Flag)false) error instantiating ...\allocator_list.d(76,42): Error: struct Factory does not overload () ...\showcase.d(79,12): Error: template instance std.experimental.allocator.allocator_list.AllocatorList!(Factory, NullAllocator) error instantiating C:\...\allocator\showcase.d has not been compiled --- Should i continue and propose a PR ?
Re: Phobos addition formal review: std.experimental.allocator
On 6/12/15 11:23 AM, Baz wrote: On Friday, 12 June 2015 at 11:09:01 UTC, Dicebot wrote: Small tip for reviewers: there are quite many modules in proposed package but majority is actual allocator implementation. I'd suggest to start investigating sources/documentation starting from http://erdani.com/d/phobos-prerelease/std_experimental_allocator.html and http://erdani.com/d/phobos-prerelease/std_experimental_allocator_building_blocks.html while using http://erdani.com/d/phobos-prerelease/std_experimental_allocator_showcase.html as reference for highlight examples. while building the package as a static lic library, i've found that _showcase.d_ is not usable under Windows because MmapAllocator is Posix only: https://github.com/andralex/phobos/blob/allocator/std/experimental/allocator/showcase.d#L66 https://github.com/andralex/phobos/blob/allocator/std/experimental/allocator/mmap_allocator.d#L14 I've asked this a while ago as well - could anyone with a Windows rig contribute a MmapAllocator for Windows? Thanks! -- Andrei
Re: Phobos addition formal review: std.experimental.allocator
On 6/12/15 4:07 PM, Tofu Ninja wrote: Are there any plans to have new and delete call out to theAllocator or processAllocator? In the future we may rig things that way. -- Andrei
Re: Phobos addition formal review: std.experimental.allocator
On Friday, 12 June 2015 at 15:54:27 UTC, Andrei Alexandrescu wrote: On 6/12/15 8:54 AM, Andrei Alexandrescu wrote: * A dynamicaly-typed layer that allows the user to swap allocators at runtime. This is embodied in IAllocator, CAllocatorImpl, theAllocator, and processAllocator. Forgot to mention - the entry point for this layer is also http://erdani.com/d/phobos-prerelease/std_experimental_allocator.html. Andrei Are there any plans to have new and delete call out to theAllocator or processAllocator?
Re: Phobos addition formal review: std.experimental.allocator
Small tip for reviewers: there are quite many modules in proposed package but majority is actual allocator implementation. I'd suggest to start investigating sources/documentation starting from http://erdani.com/d/phobos-prerelease/std_experimental_allocator.html and http://erdani.com/d/phobos-prerelease/std_experimental_allocator_building_blocks.html while using http://erdani.com/d/phobos-prerelease/std_experimental_allocator_showcase.html as reference for highlight examples.
Phobos addition formal review: std.experimental.allocator
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 This proposal will undergo the review/inclusion process similar to std.experimental.logger : 1. 2 week review/discussion period 2. if proposal author feels confident, 2 week voting period for accepting into sts.experimental 3. once enough field trial feedback has been accumulated, vote for moving into std mainline during the upcoming release beta period If reviewing find some fundamental issues or voting will fail, the whole process will repeats again per proposal authors request. Please start destruction, review period ends on 26.06 (23.59 +0 Greenwich)
Re: Phobos addition formal review: std.experimental.allocator
On 6/12/15 4:08 AM, Dicebot wrote: Small tip for reviewers: there are quite many modules in proposed package but majority is actual allocator implementation. I'd suggest to start investigating sources/documentation starting from http://erdani.com/d/phobos-prerelease/std_experimental_allocator.html and http://erdani.com/d/phobos-prerelease/std_experimental_allocator_building_blocks.html while using http://erdani.com/d/phobos-prerelease/std_experimental_allocator_showcase.html as reference for highlight examples. A few more words on that: Think of std.experimental.allocator as a three-layered cake: * An untyped layer, dealing exclusively in void[], and where all action is happening. That's where work is getting done, and also where assembly of various custom allocators happens. Best entry point for that is http://erdani.com/d/phobos-prerelease/std_experimental_allocator_building_blocks.html, which describes this layer's design and links to all untyped components. * A statically-typed layer, which takes types from the user and gives back typed memory. There are two good entry points here: http://erdani.com/d/phobos-prerelease/std_experimental_allocator.html contains generic routines for creating and destroying typed objects using any untyped allocator: make, dispose, etc. There is also assembly at this level, i.e. deciding to use different heaps for shared vs. unshared data: http://erdani.com/d/phobos-prerelease/std_experimental_allocator_typed.html (this is particularly beautiful or awful, depending). * A dynamicaly-typed layer that allows the user to swap allocators at runtime. This is embodied in IAllocator, CAllocatorImpl, theAllocator, and processAllocator. Thanks in advance for reviewing this! Andrei
Re: Phobos addition formal review: std.experimental.allocator
On 6/12/15 8:54 AM, Andrei Alexandrescu wrote: * A dynamicaly-typed layer that allows the user to swap allocators at runtime. This is embodied in IAllocator, CAllocatorImpl, theAllocator, and processAllocator. Forgot to mention - the entry point for this layer is also http://erdani.com/d/phobos-prerelease/std_experimental_allocator.html. Andrei
Re: Phobos addition formal review: std.experimental.allocator
On 6/12/15 7:31 AM, 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 As I said in the other thread: * Andrei's ODBC code seems to be in the same branch * Some code is commented in std.math and std.traits * What is std.typed_allocator? It doesn't seem to have anything related to allocators, some leftover code? Sorry about that. I now created a pull request with that stuff taken care of: https://github.com/D-Programming-Language/phobos/pull/3405 Andrei
Re: Phobos addition formal review: std.experimental.allocator
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 As I said in the other thread: * Andrei's ODBC code seems to be in the same branch * Some code is commented in std.math and std.traits * What is std.typed_allocator? It doesn't seem to have anything related to allocators, some leftover code? -- /Jacob Carlborg
Re: Phobos addition formal review: std.experimental.allocator
On Friday, 12 June 2015 at 11:09:01 UTC, Dicebot wrote: Small tip for reviewers: there are quite many modules in proposed package but majority is actual allocator implementation. I'd suggest to start investigating sources/documentation starting from http://erdani.com/d/phobos-prerelease/std_experimental_allocator.html and http://erdani.com/d/phobos-prerelease/std_experimental_allocator_building_blocks.html while using http://erdani.com/d/phobos-prerelease/std_experimental_allocator_showcase.html as reference for highlight examples. while building the package as a static lic library, i've found that _showcase.d_ is not usable under Windows because MmapAllocator is Posix only: https://github.com/andralex/phobos/blob/allocator/std/experimental/allocator/showcase.d#L66 https://github.com/andralex/phobos/blob/allocator/std/experimental/allocator/mmap_allocator.d#L14
