Hi, I vote for "No, the library should not be accepted". I do not vote this way because the library is bad. I do so because I have no way of telling. a) There is so much going on about personal hardships (e.g. hundreds of hours of family time and 12-4am sessions and fried disks and no electricity and suspected vendettas and supposedly useless reviews). None of this has anything to do with a library review. Why mention it, even if you feel it? All of this is bound to make it hard to be impartial. b) AFIO depends on other libraries which are probably going to be proposed for boost, but are not in yet. Regardless of whether there is a precedent or not, this is far from being ideal. Judging by the discussion about "Monad", getting that library into boost is not just a mere formality. Thus, Monad implicitly IS part of the review, because if it is not accepted (which we don't know today), it will become part of AFIO. Thus, the scope of the review is not defined correctly. c) There are older versions in there. There is 1.3 and 1.4 at least (are there more?). Please choose one. Go for it. Of course, boost libraries evolve over time. There will be new versions of the interface. But starting with that kind of baggage? And asking for a review that necessarily has to cover everything that is currently available? d) The implementation of 1.4 will be replaced completely. Thus, the implementation (which I consider an important part of the review), cannot be reviewed in any way that is worth anyone's time. e) Documentation * Please drop the "using namespace XYZ" from all examples. Maybe use namespace aliases. * The "cheat-sheet" for the impatient is beyond me. I have no idea what it is trying to convey. * I would certainly prefer ONE page with planned features, not sprinkled info here and there (especially not on the cheat sheet). * Redundancy: For example, all the dir() functions share the same example, don't they? I haven't done a diff and I won't. But it makes me nervous to see the same example over and over again. Why not make one page explaining all the dir() functions and then use ONE example? Also the "Note that ..." text seems to be the same on all pages. This piles up to wonderfully large amounts of documentation, but I have no intention to reading all of that ever repeating stuff trying to find the needle in the haystack that actually contains different information. This has to be condensed. * There are deprecated functions? Again, I strongly recommend to start without the extra baggage. * The mix of history and rationale is hard to read, IMO. I'd be interested in the rationale, but not in the many steps it took to get there. * I do not get the world's simplest blob store in the STL form. It is most definitely not the simplest blob store. Not even the simplest persistent blob store. Why would I need a monad there? Why is there a writable flag (why isn't constness enough to prevent writing)? Why isn't the directory created at construction once and for all? BTW: The name validation fails to check for empty string. * https://boostgsoc13.github.io/boost.afio/doc/html/afio/quickstart/workshop/n... mentions AFIO v1.40 (thats 36 numbers in the future). If the pictures are not loaded, the performance numbers in the table do not say what they are (the per sec is missing). * "The /final/ named blob store design" starts with "Caution: This section was not finished". * "The performance of and problems with this third generation design" consists of "This section was not completed in time[...]" * So basically the crucial parts of the blob store sequence are incomplete at best. f) Code (since the implementation is going to change anyway, this might all be useless): * /attic: Seriously? * I am not a big fan of #if 0 in release code * I doubt that std::cerr should be in this library's code. * I wonder why there really is a need to have your own path stuff on top of boost::filesystem. If boost::filesystem is insufficient, wouldn't it be better to contribute to that one? Haven't looked too closely, though. * I looked briefly into detail/. I would not want to look for a bug there. I have not done any detailed analysis to base this on, but: I would assume that with some refactoring (moving the platform specific parts into platform specific headers), the readability could be increased dramatically. Again: This is just a guess. In the current situation, I cannot do a formal review that would result in acceptance. I recommend to treat the current review period as a pre-review and * Do a review of Monad (or whatever it will be named) first, maybe API Bind, too, depending on whether that is going to be used in AFIO * Remove all old versions, everything deprecated and do the implementation change that is planned * Cleanup the documentation. Get it read by people with different knowledge of the problem domain. I am willing to read it as somebody who is interested but certainly not very knowledgeable. * Let the dust settle a bit * Start a new review * Even if you believe in vendettas, stay calm and answer in a matter-of-fact way. My experience with the boost list is, that there are strong voices here which have a very finely tuned sense of fairness. I did invest several hours in this review, reading the mail threads, reading the documentation, looking through some code, writing this mail. As stated above, I do consider myself interested but not very knowledgeable in the problem domain. I would like to see a library for asynchronous file IO in boost, but I don't think AFIO is ready yet. Regards, Roland
On 27 Aug 2015 at 12:54, Roland Bock wrote:
a) There is so much going on about personal hardships (e.g. hundreds of hours of family time and 12-4am sessions and fried disks and no electricity and suspected vendettas and supposedly useless reviews).
None of this has anything to do with a library review. Why mention it, even if you feel it? All of this is bound to make it hard to be impartial.
Well, I don't know what to say to this, so I'll say nothing and leave your words speak for themselves.
c) There are older versions in there. There is 1.3 and 1.4 at least (are there more?). Please choose one. Go for it.
AFIO provides strong ABI and API evolution guarantees. It guarantees that if you compile and link to v2 of the ABI, your code will continue to compile and link no matter what changes next in AFIO. Some would call that a *huge* feature very atypical of Boost libraries and one of exceptional value. You appear to consider it a fault.
e) Documentation
* Please drop the "using namespace XYZ" from all examples. Maybe use namespace aliases.
I am unaware of any case where I imported a namespace except into a local context. You may have been confused by the very commonly used: using BOOST_AFIO_V2_NAMESPACE::something; ... which is binding "something" from AFIO into the local or global namespace. This pattern is safe.
Why would I need a monad there?
The original idea was to show how monad abstracts away whether an interface is synchronous or asynchronous. It confused people. It was a mistake.
Why is there a writable flag (why isn't constness enough to prevent writing)?
I have no idea what you're going on about here, but constness at the language level has nothing to do with whether you open an on-disk store for reading or writing.
Why isn't the directory created at construction once and for all?
You appear to have not realised that this step enables invariance to the store directory being relocated by third party processes during operation. This is despite the tutorial text and the comments saying this.
BTW: The name validation fails to check for empty string.
Logged to https://github.com/BoostGSoC13/boost.afio/issues/99.
* https://boostgsoc13.github.io/boost.afio/doc/html/afio/quickstart/workshop/n... mentions AFIO v1.40 (thats 36 numbers in the future). If the pictures are not loaded, the performance numbers in the table do not say what they are (the per sec is missing).
Logged to https://github.com/BoostGSoC13/boost.afio/issues/100.
* "The /final/ named blob store design" starts with "Caution: This section was not finished". * "The performance of and problems with this third generation design" consists of "This section was not completed in time[...]" * So basically the crucial parts of the blob store sequence are incomplete at best.
The final example is over 1000 lines of code. I didn't think it mattered hugely to the review.
f) Code (since the implementation is going to change anyway, this might all be useless):
* /attic: Seriously?
AFIO will be three years old soon. It had a life before being ported to Boost in 2013. There are still bits and pieces in the attic directory of use to me.
* I am not a big fan of #if 0 in release code
For the examples, logged at https://github.com/BoostGSoC13/boost.afio/issues/93. In the implementation, if it's #if 0 it's almost always debug assist code which I turn on from time to time to help fix some reported bug. Or it's code which isn't fully baked yet and end users shouldn't use it e.g. byte range locking.
* I doubt that std::cerr should be in this library's code.
Use of std::cerr is only when something truly unusual occurs, like we are about to fatal exit the process and I would assume the user would like to know why we just called std::terminate. Or we are about to deadlock, and again the user probably wants to know why their application has just hanged itself.
* I wonder why there really is a need to have your own path stuff on top of boost::filesystem. If boost::filesystem is insufficient, wouldn't it be better to contribute to that one? Haven't looked too closely, though.
afio::path subclasses boost::filesystem::path or std::filesystem::path. It is not a separate filesystem path implementation, and in fact it is a very thin wrapper. Its purpose is purely to add type safety for NT kernel path conversions from Win32 paths so the compiler will complain if you try to do something stupid like feed a NT kernel path to a filesystem path consuming function. I did take the opportunity to fix up the deviations in boost::filesystem::path from the Filesystem TS however.
* I looked briefly into detail/. I would not want to look for a bug there. I have not done any detailed analysis to base this on, but: I would assume that with some refactoring (moving the platform specific parts into platform specific headers), the readability could be increased dramatically. Again: This is just a guess.
The platform specific parts are already in platform specific files. afio.ipp is for POSIX which includes MSVCRT. afio_iocp.ipp is for Windows IOCP.
I recommend to treat the current review period as a pre-review and
* Do a review of Monad (or whatever it will be named) first, maybe API Bind, too, depending on whether that is going to be used in AFIO * Remove all old versions, everything deprecated and do the implementation change that is planned * Cleanup the documentation. Get it read by people with different knowledge of the problem domain. I am willing to read it as somebody who is interested but certainly not very knowledgeable. * Let the dust settle a bit * Start a new review * Even if you believe in vendettas, stay calm and answer in a matter-of-fact way. My experience with the boost list is, that there are strong voices here which have a very finely tuned sense of fairness.
I do not believe in vendettas, and you will never have seen me pursue one anywhere on the internet anywhere in the past decade. Feel free to search google if you want to prove this in the nearly 30,000 public posts I believe I have made by now. I do however respond when people are attacking me behind the scenes and the leadership have specifically disclaimed doing anything about it (they claim to be unaware of anything happening). If I did not defend myself, nobody would. The fact you are aware of any of this is because of my efforts to make public the fact this stuff was going on behind the scenes. If I had done nothing, nobody here would be the wiser. It is what it is.
I did invest several hours in this review, reading the mail threads, reading the documentation, looking through some code, writing this mail. As stated above, I do consider myself interested but not very knowledgeable in the problem domain.
I would like to see a library for asynchronous file IO in boost, but I don't think AFIO is ready yet.
Thank you for your review. You caught some points nobody else had, including myself. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 2015-08-28 04:28, Niall Douglas wrote:
On 27 Aug 2015 at 12:54, Roland Bock wrote:
c) There are older versions in there. There is 1.3 and 1.4 at least (are there more?). Please choose one. Go for it. AFIO provides strong ABI and API evolution guarantees. It guarantees that if you compile and link to v2 of the ABI, your code will continue to compile and link no matter what changes next in AFIO.
Some would call that a *huge* feature very atypical of Boost libraries and one of exceptional value. You appear to consider it a fault. ABI/API evolution guarantees are cool, great, wonderful, nice, huge. Yes.
Still, it would make it easier to review the library if the scope was limited to 1.4. Having two API versions to start with doubles the effort for analyzing the API. It also increases the amount of code that has to be maintained in your boost library. That is an issue. You make harder for yourself and every reviewer this way.
e) Documentation
* Please drop the "using namespace XYZ" from all examples. Maybe use namespace aliases. I am unaware of any case where I imported a namespace except into a local context.
Copied from the introduction page: using namespace BOOST_AFIO_V2_NAMESPACE;
You may have been confused by the very commonly used:
using BOOST_AFIO_V2_NAMESPACE::something;
... which is binding "something" from AFIO into the local or global namespace. This pattern is safe. I am not a huge fan of that either if it happens in the first few lines of a long example.
It is not about safety, it is about comprehensibility. On the introduction page, you have: using namespace BOOST_AFIO_V2_NAMESPACE; using BOOST_AFIO_V2_NAMESPACE::rmdir; ... auto rmdir(async_rmdir(depends(barrier.front(), mkdir))); // Check ops for errors boost::afio::when_all_p(mkdir, mkfile, mklink, rmlink, rmfile, rmdir).wait(); The first line is bad, the second is confusing at best (IMHO).
Why is there a writable flag (why isn't constness enough to prevent writing)? I have no idea what you're going on about here, but constness at the language level has nothing to do with whether you open an on-disk store for reading or writing. First of all, please reread your claim: "World's simplest named blob store in STL iostreams"
What you present is definitely not what you promise. The simplest store would * create the folder to store stuff in at the beginning (you could use a flag here, but we are talking about the /simplest/) * you can always attempt to read (const operation) o For reading, you would never open a file for writing on the disk * your store has to be non-const if you want to write (non-const operation) o For writing you would always open a file for writing on the disk Turns out that language level constness expresses exactly the same as the read/write access on disk after construction. It is also much more friendly towards the user of the store, since the compiler will catch invalid write attempts.
Why isn't the directory created at construction once and for all? You appear to have not realised that this step enables invariance to the store directory being relocated by third party processes during operation. This is despite the tutorial text and the comments saying this.
No, you appear to be under the impression that the simplest thing has to be pretty complex already and react in ways that might or might not be a good strategy for my use case. If somebody moves the store away, while I am working on it, do I really want it to start over (as if nothing happened)? The simplest store will merely say "did not find" if you want to read and "could not write" if you want to write. If I use the simplest store in the world, I have to take care of third parties doing nonsense to my store. You even mention that in the conditions: No third party must mess with the store. So why do you treat that case then?
BTW: The name validation fails to check for empty string.
Logged to https://github.com/BoostGSoC13/boost.afio/issues/99.
* https://boostgsoc13.github.io/boost.afio/doc/html/afio/quickstart/workshop/n... mentions AFIO v1.40 (thats 36 numbers in the future). If the pictures are not loaded, the performance numbers in the table do not say what they are (the per sec is missing). Logged to https://github.com/BoostGSoC13/boost.afio/issues/100.
* "The /final/ named blob store design" starts with "Caution: This section was not finished". * "The performance of and problems with this third generation design" consists of "This section was not completed in time[...]" * So basically the crucial parts of the blob store sequence are incomplete at best. The final example is over 1000 lines of code. I didn't think it mattered hugely to the review.
Well, it is the culmination of the previous chapters. The place where you bring it all together and present the results of those efforts. How can that not matter?
f) Code (since the implementation is going to change anyway, this might all be useless):
* /attic: Seriously? AFIO will be three years old soon. It had a life before being ported to Boost in 2013. There are still bits and pieces in the attic directory of use to me.
Since the boost port is under review, I suggest getting rid of such stuff. It distracts from the code you want to be reviewed. Seeing an obvious attic, I wonder which other parts of the code might be out of use? Would I be looking at the right stuff when I look into the code? You are using git. git can store the attic. It won't get lost. But reviewers should not see it. That's all I am saying.
* I am not a big fan of #if 0 in release code For the examples, logged at https://github.com/BoostGSoC13/boost.afio/issues/93.
In the implementation, if it's #if 0 it's almost always debug assist code which I turn on from time to time to help fix some reported bug. Or it's code which isn't fully baked yet and end users shouldn't use it e.g. byte range locking.
Why don't you replace #if 0 with #if BOOST_AFIO_USE_HALF_BAKED or similar? Also, git has branches. Why don't you work on the half-baked stuff in a feature branch? #if 0 is code smell.
* I doubt that std::cerr should be in this library's code. Use of std::cerr is only when something truly unusual occurs, like we are about to fatal exit the process and I would assume the user would like to know why we just called std::terminate. Or we are about to deadlock, and again the user probably wants to know why their application has just hanged itself.
Quite a few applications turn off or ignore std::cerr. At least given users the option to provide their own reporting function for emergencies.
* I looked briefly into detail/. I would not want to look for a bug there. I have not done any detailed analysis to base this on, but: I would assume that with some refactoring (moving the platform specific parts into platform specific headers), the readability could be increased dramatically. Again: This is just a guess. The platform specific parts are already in platform specific files. afio.ipp is for POSIX which includes MSVCRT. afio_iocp.ipp is for Windows IOCP. Hmm. The code I looked at was still riddled with #if THIS_SYSTEM do that #else do something else.
I recommend to treat the current review period as a pre-review and
* Do a review of Monad (or whatever it will be named) first, maybe API Bind, too, depending on whether that is going to be used in AFIO * Remove all old versions, everything deprecated and do the implementation change that is planned * Cleanup the documentation. Get it read by people with different knowledge of the problem domain. I am willing to read it as somebody who is interested but certainly not very knowledgeable. * Let the dust settle a bit * Start a new review * Even if you believe in vendettas, stay calm and answer in a matter-of-fact way. My experience with the boost list is, that there are strong voices here which have a very finely tuned sense of fairness. I do not believe in vendettas,
If I did not defend myself, nobody would. Based on what I see on this list, I doubt that. But there is no way of
You claimed that you were victim of one more than once in the context of the AFIO review. That's why I mentioned it. proving it one way or another.
I did invest several hours in this review, reading the mail threads, reading the documentation, looking through some code, writing this mail. As stated above, I do consider myself interested but not very knowledgeable in the problem domain.
I would like to see a library for asynchronous file IO in boost, but I don't think AFIO is ready yet. Thank you for your review. You caught some points nobody else had, including myself.
Glad to help. And with the points where we don't see eye to eye: I am happy to explain if we meet at CppCon. Best, Roland
Am 28.08.2015 8:30 vorm. schrieb "Roland Bock" <rbock@eudoxos.de>:
On 2015-08-28 04:28, Niall Douglas wrote:
On 27 Aug 2015 at 12:54, Roland Bock wrote:
* I doubt that std::cerr should be in this library's code. Use of std::cerr is only when something truly unusual occurs, like we are about to fatal exit the process and I would assume the user would like to know why we just called std::terminate. Or we are about to deadlock, and again the user probably wants to know why their application has just hanged itself. Quite a few applications turn off or ignore std::cerr. At least given users the option to provide their own reporting function for emergencies.
Isn't that exactly what exceptions are for?
On 28 Aug 2015 at 8:29, Roland Bock wrote:
I am unaware of any case where I imported a namespace except into a local context. Copied from the introduction page:
using namespace BOOST_AFIO_V2_NAMESPACE;
That isn't a global using namespace. It could be a local using namespace, and in its original source file it is indeed a local using namespace. But fair enough it could be interpreted differently, so logged to https://github.com/BoostGSoC13/boost.afio/issues/102.
Why is there a writable flag (why isn't constness enough to prevent writing)? I have no idea what you're going on about here, but constness at the language level has nothing to do with whether you open an on-disk store for reading or writing. First of all, please reread your claim: "World's simplest named blob store in STL iostreams"
What you present is definitely not what you promise.
By simplest it was meant the public interface API. Not the implementation. I thought that obvious, but logged anyway to https://github.com/BoostGSoC13/boost.afio/issues/103.
Also, git has branches. Why don't you work on the half-baked stuff in a feature branch?
#if 0 is code smell.
These are entirely personal opinion based on suppositions and assumptions with zero technical relevance to the quality of a library, as was a lot of the stuff I snipped. I don't work in pristine code environments, I like to keep around bits of code in #if 0 blocks to remind me of things, or for the occasional use, or for any other reason. I personally would find such segments educational when reviewing a library rather than the unfounded claim of "code smell" which may be true in other code bases in your experience, but I can assure you is not the case in my code bases. Instead of assuming code smells or other non-factually based suppositions, consider examining the coverage of the unit test suite and the rigorous testing regime run each and every commit and taking some hours to complete.
* I doubt that std::cerr should be in this library's code. Use of std::cerr is only when something truly unusual occurs, like we are about to fatal exit the process and I would assume the user would like to know why we just called std::terminate. Or we are about to deadlock, and again the user probably wants to know why their application has just hanged itself. Quite a few applications turn off or ignore std::cerr. At least given users the option to provide their own reporting function for emergencies.
It is trivial to redirect std::cerr elsewhere. I am happy for std::cerr to become a macro, and this is logged at https://github.com/BoostGSoC13/boost.afio/issues/104.
* I looked briefly into detail/. I would not want to look for a bug there. I have not done any detailed analysis to base this on, but: I would assume that with some refactoring (moving the platform specific parts into platform specific headers), the readability could be increased dramatically. Again: This is just a guess. The platform specific parts are already in platform specific files. afio.ipp is for POSIX which includes MSVCRT. afio_iocp.ipp is for Windows IOCP. Hmm. The code I looked at was still riddled with #if THIS_SYSTEM do that #else do something else.
Sure. There are substantial deviations between POSIX implementations, but not enough to warrant separate files. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 2015-08-29 01:07, Niall Douglas wrote:
On 28 Aug 2015 at 8:29, Roland Bock wrote:
I am unaware of any case where I imported a namespace except into a local context. Copied from the introduction page:
using namespace BOOST_AFIO_V2_NAMESPACE; That isn't a global using namespace. It could be a local using namespace, and in its original source file it is indeed a local using namespace.
And global or not is irrelevant: it makes example code hard to read if you are not familiar with the library.
But fair enough it could be interpreted differently, so logged to https://github.com/BoostGSoC13/boost.afio/issues/102.
Why is there a writable flag (why isn't constness enough to prevent writing)? I have no idea what you're going on about here, but constness at the language level has nothing to do with whether you open an on-disk store for reading or writing. First of all, please reread your claim: "World's simplest named blob store in STL iostreams"
What you present is definitely not what you promise. By simplest it was meant the public interface API. Not the implementation.
I would argue with Scott Meyers: http://programmer.97things.oreilly.com/wiki/index.php/Make_Interfaces_Easy_t... and others. I explained why write should be const for example. I explained why the writable flag is even working against the conditions you list yourself in that example. It is not the simplest API either.
I thought that obvious, but logged anyway to https://github.com/BoostGSoC13/boost.afio/issues/103.
Also, git has branches. Why don't you work on the half-baked stuff in a feature branch?
#if 0 is code smell. These are entirely personal opinion based on suppositions and assumptions with zero technical relevance to the quality of a library, as was a lot of the stuff I snipped. Of course it is personal. But it also happens to be shared by others. And since the library is meant to be used by others, why not listen to them?
Anyway. Up to you.
I don't work in pristine code environments, I like to keep around bits of code in #if 0 blocks to remind me of things, or for the occasional use, or for any other reason. I personally would find such segments educational when reviewing a library rather than the unfounded claim of "code smell" which may be true in other code bases in your experience, but I can assure you is not the case in my code bases. Wow. Speaking of unfounded...
Instead of assuming code smells or other non-factually based suppositions, consider examining the coverage of the unit test suite and the rigorous testing regime run each and every commit and taking some hours to complete. I would do that, once I understand, what needs to be tested. And btw, I know you are fully aware, that the quality of tests does not depend on coverage or the amount of time spent on running them. So why bring that up all the time?
I am trying to tell you where I have trouble comprehending the docs and the code. Since comprehension is a personal thing, sure, OK, you can call all this personal preferences. But they are neither unfounded, nor non-factual, nor do they have zero technical relevance (except maybe for you, which would speak volumes).
On 2015-08-29 08:46, Roland Bock wrote:
On 2015-08-29 08:27, Roland Bock wrote:
I explained why write should be const Sorry, read should be const, of course
And, just to show you that your claim about unfounded claims is unfounded (come on, seriously!), here is what the simplest API of an STL-based named Blob store could look like IMO. #include <vector> #include <string> class data_store { std::string _store_path; public: data_store(std::string store_path); auto read(std::string name) const -> std::vector<uint8_t>; auto write(std::string name, const std::vector<uint8_t>& blob) -> void; }; Please note that this one actually /is/ STL based. Please remember to read your own conditions before making unfounded claims about how this is much, much worse in oh so many ways than the one you claim to be the simplest one. Still trying to help, although you might believe or see it that way.
On 29 Aug 2015 at 9:11, Roland Bock wrote:
And, just to show you that your claim about unfounded claims is unfounded (come on, seriously!), here is what the simplest API of an STL-based named Blob store could look like IMO.
#include <vector> #include <string>
class data_store { std::string _store_path;
public: data_store(std::string store_path);
auto read(std::string name) const -> std::vector<uint8_t>;
auto write(std::string name, const std::vector<uint8_t>& blob) -> void; };
Please note that this one actually /is/ STL based.
The above is a poor interface design. Imagine 1Gb sized blobs. The interface in the tutorial scales very well to 1Gb sized blobs. Anyway, it's been made very clear by many reviewers that nobody likes the workshop tutorial, so I should imagine that the interface will be completely refactored into something less controversial.
Please remember to read your own conditions before making unfounded claims about how this is much, much worse in oh so many ways than the one you claim to be the simplest one.
Still trying to help, although you might believe or see it that way.
You might find me far more receptive to your help if you had (a) not publicly questioned my sanity (b) used my mention of the hours I have invested into the development of AFIO as a basis for rejecting it in your review (c) accused me of trying to "shape" the review or whatever you called it, and then also used that as a basis for rejecting my library. Your review did contain useful points, for which I am grateful and I said so. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 2015-08-29 16:07, Niall Douglas wrote:
On 29 Aug 2015 at 9:11, Roland Bock wrote:
And, just to show you that your claim about unfounded claims is unfounded (come on, seriously!), here is what the simplest API of an STL-based named Blob store could look like IMO.
#include <vector> #include <string>
class data_store { std::string _store_path;
public: data_store(std::string store_path);
auto read(std::string name) const -> std::vector<uint8_t>;
auto write(std::string name, const std::vector<uint8_t>& blob) -> void; };
Please note that this one actually /is/ STL based. The above is a poor interface design. Imagine 1Gb sized blobs. The interface in the tutorial scales very well to 1Gb sized blobs. Huh? I thought the purpose of the example was to serve as an introduction to doing it better with AFIO?
But anyway, that complaint is unfounded. Citing from from that page of yours, one of the conditions of that "simplest blob store": https://boostgsoc13.github.io/boost.afio/doc/html/afio/quickstart/workshop/n... Conditions: ... "The size of the values being read and written is small." ... That's why I asked you to reread your own conditions before answering. Of course, I could also do something super simple with streams, too, if you insist on them: #include <iostream> #include <fstream> #include <vector> #include <string> class data_store { std::string _store_path; std::ifstream _ifs; std::ofstream _ofs; public: data_store(std::string store_path); data_store(const data_store&); auto getIstream(std::string name) const -> std::istream&; auto getOstream(std::string name) -> std::ostream&; }; (I would not call this the world's simplest named blob store anymore. You might feel tempted to call that bad design, too. But please remember that there is another of your conditions "Only one thread or process will ever interact with the key-value store at a time." But honestly, I would rather forget about the streams in the "simplest" blob store. I'd rather extend my original code with methods to append data or to read chunks.
Anyway, it's been made very clear by many reviewers that nobody likes the workshop tutorial, so I should imagine that the interface will be completely refactored into something less controversial.
But it will stay a named blob store? I am actually interested in that.
Please remember to read your own conditions before making unfounded claims about how this is much, much worse in oh so many ways than the one you claim to be the simplest one.
Still trying to help, although you might believe or see it that way. You might find me far more receptive to your help if you had (a) not publicly questioned my sanity (b) used my mention of the hours I have invested into the development of AFIO as a basis for rejecting it in your review (c) accused me of trying to "shape" the review or whatever you called it, and then also used that as a basis for rejecting my library.
a) I did no such thing! If you read it that way, I am very sorry for the misunderstanding. b) It was a combination of things, and I stand by that c) I stand by that I offered to explain face to face at CppCon, because mail won't suffice to work this out.
Your review did contain useful points, for which I am grateful and I said so.
I am aware of that. Otherwise I would not continue to communicate :-) Roland
On 29 Aug 2015 at 19:37, Roland Bock wrote:
Huh? I thought the purpose of the example was to serve as an introduction to doing it better with AFIO?
I've posted a much better simple transactional key-value store API to https://gist.github.com/ned14/163ac57c937bda61e5c9. It'll probably become the standard API for all that tutorial's stages.
But anyway, that complaint is unfounded. Citing from from that page of yours, one of the conditions of that "simplest blob store":
https://boostgsoc13.github.io/boost.afio/doc/html/afio/quickstart/workshop/n...
Conditions: ... "The size of the values being read and written is small." ...
That's why I asked you to reread your own conditions before answering.
That's a condition caused by the quality of implementation, not quality of interface design. I would have thought that very obvious from the implementation.
Of course, I could also do something super simple with streams, too, if you insist on them:
#include <iostream> #include <fstream> #include <vector> #include <string>
class data_store { std::string _store_path; std::ifstream _ifs; std::ofstream _ofs;
public: data_store(std::string store_path); data_store(const data_store&);
auto getIstream(std::string name) const -> std::istream&;
auto getOstream(std::string name) -> std::ostream&; };
(I would not call this the world's simplest named blob store anymore. You might feel tempted to call that bad design, too. But please remember that there is another of your conditions
"Only one thread or process will ever interact with the key-value store at a time."
But honestly, I would rather forget about the streams in the "simplest" blob store. I'd rather extend my original code with methods to append data or to read chunks.
My replacement API exclusively uses scatter gather buffers. I agree the STL iostreams interop is probably unnecessary. I only added it because I could see complaints here that AFIO isn't STL iostreams friendly, as it happened nobody actually complained about that.
Anyway, it's been made very clear by many reviewers that nobody likes the workshop tutorial, so I should imagine that the interface will be completely refactored into something less controversial. But it will stay a named blob store? I am actually interested in that.
Of course.
Please remember to read your own conditions before making unfounded claims about how this is much, much worse in oh so many ways than the one you claim to be the simplest one.
Still trying to help, although you might believe or see it that way. You might find me far more receptive to your help if you had (a) not publicly questioned my sanity (b) used my mention of the hours I have invested into the development of AFIO as a basis for rejecting it in your review (c) accused me of trying to "shape" the review or whatever you called it, and then also used that as a basis for rejecting my library. a) I did no such thing! If you read it that way, I am very sorry for the misunderstanding.
I'll chalk that down to a misunderstanding. Thanks for the apology.
b) It was a combination of things, and I stand by that c) I stand by that
I offered to explain face to face at CppCon, because mail won't suffice to work this out.
I'm happy to talk with anyone anywhere about anything. I assume you'll be at the Boost dinner? Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 2015-08-30 16:55, Niall Douglas wrote:
On 29 Aug 2015 at 19:37, Roland Bock wrote:
b) It was a combination of things, and I stand by that c) I stand by that
I offered to explain face to face at CppCon, because mail won't suffice to work this out. I'm happy to talk with anyone anywhere about anything. I assume you'll be at the Boost dinner?
Niall Probably yes. Looking forward to it!
Roland
On 8/29/15 12:11 AM, Roland Bock wrote:
On 2015-08-29 08:46, Roland Bock wrote:
On 2015-08-29 08:27, Roland Bock wrote:
I explained why write should be const Sorry, read should be const, of course
And, just to show you that your claim about unfounded claims is unfounded (come on, seriously!), here is what the simplest API of an STL-based named Blob store could look like IMO.
#include <vector> #include <string>
class data_store { std::string _store_path;
public: data_store(std::string store_path);
auto read(std::string name) const -> std::vector<uint8_t>;
auto write(std::string name, const std::vector<uint8_t>& blob) -> void; };
Please note that this one actually /is/ STL based.
I've been looking at the comments in the review threads. Sometime ago I reviewed the documentation for the AFIO library. I sat in on two presentations on the subject at C++Now. That is all I know about the library. Sadly, I won't have time to make a review. The above comment is the only thing in all of this which has made any sense at all to me. I don't see why something like the above shouldn't be possible and I don't see why it would be a bad idea. A library which did this would be highly useful. Robert Ramey
On 29 Aug 2015 at 9:18, Robert Ramey wrote:
And, just to show you that your claim about unfounded claims is unfounded (come on, seriously!), here is what the simplest API of an STL-based named Blob store could look like IMO.
#include <vector> #include <string>
class data_store { std::string _store_path;
public: data_store(std::string store_path);
auto read(std::string name) const -> std::vector<uint8_t>;
auto write(std::string name, const std::vector<uint8_t>& blob) -> void; };
Please note that this one actually /is/ STL based.
I've been looking at the comments in the review threads. Sometime ago I reviewed the documentation for the AFIO library. I sat in on two presentations on the subject at C++Now. That is all I know about the library. Sadly, I won't have time to make a review.
The above comment is the only thing in all of this which has made any sense at all to me. I don't see why something like the above shouldn't be possible and I don't see why it would be a bad idea. A library which did this would be highly useful.
A transactional key-value store *is* coming. That's my end goal in this: a standardised transactional key-value store built into the C++ language with bindings for C, Python, .NET and so on. I'm currently hoping to bring that for Boost review start of 2017 (anyone who wants to bring it to Boost by next spring instead can sponsor me for a very reasonable hourly rate, drop me a line if you're interested). I did a bit of mocking up of what the refactored data_store interface for that AFIO tutorial will look like: https://gist.github.com/ned14/163ac57c937bda61e5c9 Ok, so it's no longer the world's simplest key-value store API and I deliberately dropped the STL iostreams interop, but the big difference is that this one is real-world ready and probably quite close to a final API design for such a thing. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
I did a bit of mocking up of what the refactored data_store interface for that AFIO tutorial will look like:
https://gist.github.com/ned14/163ac57c937bda61e5c9
Ok, so it's no longer the world's simplest key-value store API and I deliberately dropped the STL iostreams interop, but the big difference is that this one is real-world ready and probably quite close to a final API design for such a thing.
*Looks* much better, you seem to listen after all. I wouldn't call that to be real-world-ready without a thorough specification of the semantics, though. For instance: - What happens if commit() was called on your transaction, the returned future is not ready yet, but the ~transaction() is called? - Why is your transaction object not RAII? - Why do you need begin_transaction? - Why do you need two load_blob() functions? - Why is your buffertype so over-specified, shouldn't any range do? - Why can't the user specify/provide the type of the hash? std::hash<> anybody? - Why does one of the load_blob returns a future<std::shared_ptr<buffers_type>>? Handle to handle to handle to data this time? Shared ownership again, really? - Can this blob-store be distributed over many machines? Can it be in memory only (with transparent disk backup if needed)? - Why do you specify undefined behavior into your API (see your union). Why do you need this in the first place? - What's the concurrency/thread-safety guarantees of each of the functions? etc. many more open questions remain. Please, don't get me wrong, I don't need/want answers to those questions right away, I'm just listing those for you to think about. I however suggest that you redesign AFIO first (including the proposed AFIO API), before you even start thinking about submitting this to Boost review. Regards Hartmut --------------- http://boost-spirit.com http://stellar.cct.lsu.edu
On 30 Aug 2015 at 10:19, Hartmut Kaiser wrote:
I did a bit of mocking up of what the refactored data_store interface for that AFIO tutorial will look like:
https://gist.github.com/ned14/163ac57c937bda61e5c9
Ok, so it's no longer the world's simplest key-value store API and I deliberately dropped the STL iostreams interop, but the big difference is that this one is real-world ready and probably quite close to a final API design for such a thing.
*Looks* much better, you seem to listen after all. I wouldn't call that to be real-world-ready without a thorough specification of the semantics, though.
This is the first post you have written which contained anything useful. You also wrote civilly in a productive tone. Thank you.
- What happens if commit() was called on your transaction, the returned future is not ready yet, but the ~transaction() is called?
Nothing happens. commit() transfers the state held by the transaction object into an asynchronous operation bound by the returned future. The transaction object's destructor becomes do-nothing. You are probably now about to ask if the returned future's destructor is blocking. It is not. The transferred state is kept around by the AFIO engine until success or failure and disposed of internally.
- Why is your transaction object not RAII?
You may have a different definition to me. The idea is when you begin a transaction, you pin a copy of the internally held key-value index to that transaction. You then say what you are going to modify to the transaction object via add(). If an exception throw occurs before the commit(), the transaction destructor will throw away any modifications you have added and it unpins the internally held key-value index.
- Why do you need begin_transaction?
It pins a copy of the internally held key-value index to the transaction object. Later on, during commit, it should be able to match off key lookups with key updates to decide if two concurrent commits conflict or not. This interface is purely for the AFIO tutorial only. Its concurrent transaction handling policy is to always abort if any concurrent update happened since the index used for the transaction was pinned. This is deliberately simple for the tutorial.
- Why do you need two load_blob() functions?
One loads a blob into a set of user supplied scatter buffers. The other memory maps the blob and returns it as a set of potentially discontinuous buffers. This saves you having to allocate your own buffers. The reason there are two forms is that there is limited free address space on 32 bit architectures.
- Why is your buffertype so over-specified, shouldn't any range do?
std::vector is ABI stable and friendly to C and Microsoft COM wrappers.
- Why can't the user specify/provide the type of the hash? std::hash<> anybody?
For a content addressible data store, std::hash<> collides too frequently. The choice between Spooky (non-crypto) and Blake2b (crypto) is all anyone needs. There is a good argument for renaming them to "fast" and "quality" respectively which I would do if this were not a tutorial example.
- Why does one of the load_blob returns a future<std::shared_ptr<buffers_type>>? Handle to handle to handle to data this time? Shared ownership again, really?
shared_ptr can opaquely call an internally supplied destruction callback to de-memory map the blob. You could return some custom smart pointer type which does the same. I felt it was much of a muchness. I know you'll disagree with that decision, but it's a tutorial, and an additional custom smart pointer type is extra screen verbiage.
- Can this blob-store be distributed over many machines?
It can be concurrently used from many machines, including via CIFS network shares. It would be trivial to extend to be distributed across multiple machines, but that is an implementation detail.
Can it be in memory only (with transparent disk backup if needed)?
The kernel file page cache and extents based design makes it effectively memory only with very lazy but write ordered flushing to disk. If used with scatter gather buffers allocated via afio::page_allocator, it also is zero memory copy on Windows and BSD.
- Why do you specify undefined behavior into your API (see your union). Why do you need this in the first place?
The compiler will correctly align a __m128i on all the three major compilers which allows for the hash implementations to be more efficient. I left out the #ifdef SSE2 for brevity, but the final edition would have it.
- What's the concurrency/thread-safety guarantees of each of the functions?
Undecided. I currently expect thread safety for everything will naturally emerge without even needing a mutex, but my local implementation here isn't finished yet.
etc. many more open questions remain. Please, don't get me wrong, I don't need/want answers to those questions right away, I'm just listing those for you to think about.
I however suggest that you redesign AFIO first (including the proposed AFIO API), before you even start thinking about submitting this to Boost review.
This is purely for the front AFIO tutorial only and what I would call an unguaranteed solution. It has an over engineered interface for a tutorial, but I also want to demonstrate how AFIO's features deliver the solution in just 1000 lines of code. A production ready solution with well tested power loss behaviour I doubt I could bring for Boost review until early 2017 unless someone sponsors me to work on this full time, in which case Spring 2016 would a maximum. I have had enough of working 60-80 hour weeks which has been the case most weeks since February and has contributed in large part to my recent grumpiness. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
I did a bit of mocking up of what the refactored data_store interface for that AFIO tutorial will look like:
https://gist.github.com/ned14/163ac57c937bda61e5c9
Ok, so it's no longer the world's simplest key-value store API and I deliberately dropped the STL iostreams interop, but the big difference is that this one is real-world ready and probably quite close to a final API design for such a thing.
*Looks* much better, you seem to listen after all. I wouldn't call that to be real-world-ready without a thorough specification of the semantics, though.
This is the first post you have written which contained anything useful. You also wrote civilly in a productive tone. Thank you.
Most welcome, however note: this is just my way of (to quote you): "... draw a series of conclusions from a set of incorrect assumptions, as per (my) usual style of trying to instil uncertainty and doubt" (I left the spelling unchanged).
- What happens if commit() was called on your transaction, the returned future is not ready yet, but the ~transaction() is called?
Nothing happens. commit() transfers the state held by the transaction object into an asynchronous operation bound by the returned future. The transaction object's destructor becomes do-nothing.
And if the commit fails? Will the future roll back the transaction, even if the transaction was already destroyed? To avoid this you'd have for the future to keep the transaction alive, which contradicts to it being move-only (unique vs. shared ownership problems again, I believe).
You are probably now about to ask if the returned future's destructor is blocking. It is not. The transferred state is kept around by the AFIO engine until success or failure and disposed of internally.
If I wanted to ask that I would have.
- Why is your transaction object not RAII?
You may have a different definition to me. The idea is when you begin a transaction, you pin a copy of the internally held key-value index to that transaction. You then say what you are going to modify to the transaction object via add(). If an exception throw occurs before the commit(), the transaction destructor will throw away any modifications you have added and it unpins the internally held key-value index.
In case you didn't know: RAII is a silly acronym which stands for 'Resource Acquisition Is Initialization'. IOW, a resource is acquired by the constructor and released by the destructor. Your transaction does not acquire anything while being constructed, it does not even have an initializing non-default constructor. However your begin_transaction() does acquire that resource and IIUC somehow magically transfers it into the transaction. That's the reason why I asked this question.
- Why do you need begin_transaction?
It pins a copy of the internally held key-value index to the transaction object. Later on, during commit, it should be able to match off key lookups with key updates to decide if two concurrent commits conflict or not.
I should have asked, why doesn't `transaction` provide a constructor which takes a reference to the outer blob-store and that boolean value (even if I still believe booleans in APIs is something to avoid, but you would probably disregard it as being too compsci-like, and not sufficiently 'haphazardly' as you prefer doing your design - note: your own words).
This interface is purely for the AFIO tutorial only. Its concurrent transaction handling policy is to always abort if any concurrent update happened since the index used for the transaction was pinned. This is deliberately simple for the tutorial.
Ahh, now it's for a tutorial only, previously you claimed this to be 'ready for real-world' and you already were planning to propose it for Boost review in 2017 if not for standardization.
- Why do you need two load_blob() functions?
One loads a blob into a set of user supplied scatter buffers.
The other memory maps the blob and returns it as a set of potentially discontinuous buffers. This saves you having to allocate your own buffers.
How can a blob-store which hands out memory mapped ranges pointing into a file be thread-safe? What about if two threads ask for the same memory mapped region for the same file? Can you handle that properly, even if both threads write to that same region?
The reason there are two forms is that there is limited free address space on 32 bit architectures.
- Why is your buffertype so over-specified, shouldn't any range do?
std::vector is ABI stable and friendly to C and Microsoft COM wrappers.
You'd surprised to hear that there are no guarantees for std::vector to be ABI stable. Especially in the MS world where every compiler version is allowed to break C++ ABI compatibility. Granted, every sane implementation will make it ABI stable, but that's beside the point of my question. Wouldn't a std::vector<std::pair<char const*, char const*>> be as fine? Shouldn't be anything usable for which std::begin() and std::end() are well defined? Wouldn't a std::list<Range> do the same thing? Etc.
- Why can't the user specify/provide the type of the hash? std::hash<> anybody?
For a content addressible data store, std::hash<> collides too frequently.
std::hash<> is just an example and for some people it might be good enough. What I should have suggested was using the Hash concept (http://en.cppreference.com/w/cpp/concept/Hash), though.
The choice between Spooky (non-crypto) and Blake2b (crypto) is all anyone needs. There is a good argument for renaming them to "fast" and "quality" respectively which I would do if this were not a tutorial example.
Again that assumption that you know what your user needs, this time not even giving a chance to overrule your ahh-so-sensible-defaults. BTW, I disagree violently with your statement that 'good design is about choosing good defaults'. Good design is about being simple to use (which might encompass sensible defaults) while allowing the user to still use your library for things you haven't even dreamt of.
- Why does one of the load_blob returns a future<std::shared_ptr<buffers_type>>? Handle to handle to handle to data this time? Shared ownership again, really?
shared_ptr can opaquely call an internally supplied destruction callback to de-memory map the blob.
You're exposing your library internals to the user. Just because you believe shared_ptr is the only way to properly destruct things doesn't mean that you should use it.
You could return some custom smart pointer type which does the same.
Or you return a custom type with value semantics which exposes unique ownership through its API and still does the correct thing. I strongly believe that future<some_custom_type> could do the right thing - if `some_custom_type` was designed properly. Also note, instead of 3 levels of indirection in your design (all bets are off if you try to tell who owns which of the indirections), this would give you just one with well define ownership semantics.
I felt it was much of a muchness. I know you'll disagree with that decision, but it's a tutorial, and an additional custom smart pointer type is extra screen verbiage.
Again, I thought it was a real-world-ready-for-boost-review-design. Please make up your mind.
- Can this blob-store be distributed over many machines?
It can be concurrently used from many machines, including via CIFS network shares. It would be trivial to extend to be distributed across multiple machines, but that is an implementation detail.
Ok.
Can it be in memory only (with transparent disk backup if needed)?
The kernel file page cache and extents based design makes it effectively memory only with very lazy but write ordered flushing to disk. If used with scatter gather buffers allocated via afio::page_allocator, it also is zero memory copy on Windows and BSD.
How is such a design making the access to the data thread-safe?
- Why do you specify undefined behavior into your API (see your union). Why do you need this in the first place?
The compiler will correctly align a __m128i on all the three major compilers which allows for the hash implementations to be more efficient. I left out the #ifdef SSE2 for brevity, but the final edition would have it.
I'm able to read code, so I understood your intention. That does not make it more conforming, though. It's still undefined behavior. I thought we were over this kind of stuff.
- What's the concurrency/thread-safety guarantees of each of the functions?
Undecided. I currently expect thread safety for everything will naturally emerge without even needing a mutex, but my local implementation here isn't finished yet.
If you expose shared ownership (currently you do) you will have to have some kind of synchronization.
etc. many more open questions remain. Please, don't get me wrong, I don't need/want answers to those questions right away, I'm just listing those for you to think about.
I however suggest that you redesign AFIO first (including the proposed AFIO API), before you even start thinking about submitting this to Boost review.
This is purely for the front AFIO tutorial only and what I would call an unguaranteed solution. It has an over engineered interface for a tutorial, but I also want to demonstrate how AFIO's features deliver the solution in just 1000 lines of code.
That does not mean anything. With the appropriate abstractions you can implement it with as many lines of code as you expose API functions (multiplied by 3 if you count the '{' and '}').
A production ready solution with well tested power loss behaviour I doubt I could bring for Boost review until early 2017 unless someone sponsors me to work on this full time, in which case Spring 2016 would a maximum.
I have neither the means to sponsor you, nor would I contemplate doing so even if I had.
I have had enough of working 60-80 hour weeks which has been the case most weeks since February and has contributed in large part to my recent grumpiness.
None of my questions was asking for this information. Thanks for providing it anyways. Regards Hartmut --------------- http://boost-spirit.com http://stellar.cct.lsu.edu
On 2015-08-29 08:27, Roland Bock wrote:
Instead of assuming code smells or other non-factually based
suppositions, consider examining the coverage of the unit test suite and the rigorous testing regime run each and every commit and taking some hours to complete. I would do that, once I understand, what needs to be tested. And btw, I know you are fully aware, that the quality of tests does not depend on coverage or the amount of time spent on running them. So why bring that up all the time? Another personal opinion, but please search the Internet before calling it unfounded or non-factual: Long running per-commit tests actually are a bad thing since they slow down development considerably.
I suggest trying to re-design your tests in such a way that they run much faster.
On Fri, Aug 28, 2015 at 8:07 PM, Niall Douglas <s_sourceforge@nedprod.com> wrote:
Also, git has branches. Why don't you work on the half-baked stuff in a feature branch?
#if 0 is code smell.
These are entirely personal opinion based on suppositions and assumptions with zero technical relevance to the quality of a library, as was a lot of the stuff I snipped. I don't work in pristine code environments, I like to keep around bits of code in #if 0 blocks to remind me of things, or for the occasional use, or for any other reason. I personally would find such segments educational when reviewing a library rather than the unfounded claim of "code smell" which may be true in other code bases in your experience, but I can assure you is not the case in my code bases.
Instead of assuming code smells or other non-factually based suppositions, consider examining the coverage of the unit test suite and the rigorous testing regime run each and every commit and taking some hours to complete.
It's a code smell. AFIO is suposedly a finished library candidate for inclusion in Boost. Dead blocks shouldn't be there. Just store the code in your computer and get it out of a library that will continuously be processed in user code (even if with negligible costs). If you need special Intellisense (or alike) information just configure your local environment to read those files elsewhere.
Quite a few applications turn off or ignore std::cerr. At least given users the option to provide their own reporting function for emergencies.
It is trivial to redirect std::cerr elsewhere.
I am happy for std::cerr to become a macro, and this is logged at https://github.com/BoostGSoC13/boost.afio/issues/104.
Why on earth would you want to do this? Exceptions are there for these cases. Don't use a macro. Just don't log. It's not your job to do so. Specially using std::cerr. If you create such macro, and since I use Boost Log, that means I have to take care of configuring AFIO so it doesn't "spill" information. This is just bad. Regards, Rodrigo Madera
On 8/29/2015 1:30 AM, Rodrigo Madera wrote:
Quite a few applications turn off or ignore std::cerr. At least given users the option to provide their own reporting function for emergencies.
It is trivial to redirect std::cerr elsewhere.
I am happy for std::cerr to become a macro, and this is logged at https://github.com/BoostGSoC13/boost.afio/issues/104.
Why on earth would you want to do this? Exceptions are there for these cases.
I don't know in which cases this is used, but if it's in the case of UB/programming error I'd much rather it not use exceptions. Especially if it is for precondition violations. If it is failure to meet post conditions then by all means it should be signaled with an exception.
On 29 Aug 2015 at 3:30, Rodrigo Madera wrote:
It's a code smell. AFIO is suposedly a finished library candidate for inclusion in Boost. Dead blocks shouldn't be there. Just store the code in your computer and get it out of a library that will continuously be processed in user code (even if with negligible costs).
Ok, seeing as you're the second person to have a problem with this, what I can do is add the source code to the special #if 0 filtering tool which strips out the #if 0 blocks into a special pristine boost only branch. I have modified the relevant issue at https://github.com/BoostGSoC13/boost.afio/issues/93. For the record, I strenously disagree with this viewpoint. It is the same as claiming that code formatting differences are a code smell. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On Sat, Aug 29, 2015, Niall Douglas wrote:
Ok, seeing as you're the second person to have a problem with this, what I can do is add the source code to the special #if 0 filtering tool which strips out the #if 0 blocks into a special pristine boost only branch.
For me, long #if 0 blocks or /* commented out code are create a feeling of: - Inconvenience: It may as well not even be code in there; it could be a giant wall of text that starts with "Call me Ishmael." I just want to skip past it. - Doubt: About the quality or state of completeness. As if I was reading a paper with the author's annotations about things that yet need to be changed or fixed. The feelings are stronger still if it's in the documentation examples since they're more visible than the implementation. Glen
On 29 Aug 2015 at 10:31, Glen Fernandes wrote:
For me, long #if 0 blocks or /* commented out code are create a feeling of: - Inconvenience: It may as well not even be code in there; it could be a giant wall of text that starts with "Call me Ishmael." I just want to skip past it. - Doubt: About the quality or state of completeness. As if I was reading a paper with the author's annotations about things that yet need to be changed or fixed.
The feelings are stronger still if it's in the documentation examples since they're more visible than the implementation.
I definitely can see the point in the documentation examples, and there is an issue logged to have a script preprocess out all the #if 0 blocks. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
-----Original Message----- From: Boost [mailto:boost-bounces@lists.boost.org] On Behalf Of Niall Douglas Sent: Friday, August 28, 2015 19:08 To: boost@lists.boost.org Subject: Re: [boost] [AFIO] Review (or lack of it)
It is trivial to redirect std::cerr elsewhere.
This assumes the user is not using std::cerr for something else. Please never use std::cerr, in a library. Every library I've seen that wants to log that "something bad happened" supports a user-supplied log function. To this end, I'd strongly prefer a configurable callback, rather than a macro. If a macro is used, then it places the burden of understanding how it interacts with the library's internal state on the user. As for std::terminate(), if unwinding the call stack would leave the program in a state from where it could not reliably be returned to a known-good state, then I think calling std::terminate() is the best option. I understand that, in the real world, there might be error conditions (similar to assertion failures), where some inconsistency is detected. In such cases, I think it's far better to log and abort, than to try to continue in a bad state. There's no benefit to using exceptions, here - only downsides. Matt ________________________________ This e-mail contains privileged and confidential information intended for the use of the addressees named above. If you are not the intended recipient of this e-mail, you are hereby notified that you must not disseminate, copy or take any action in respect of any information contained in it. If you have received this e-mail in error, please notify the sender immediately by e-mail and immediately destroy this e-mail and its attachments.
-----Original Message----- From: Boost [mailto:boost-bounces@lists.boost.org] On Behalf Of Niall Douglas Sent: Friday, August 28, 2015 19:08 To: boost@lists.boost.org Subject: Re: [boost] [AFIO] Review (or lack of it) On 28 Aug 2015 at 8:29, Roland Bock wrote:
#if 0 is code smell.
These are entirely personal opinion based on suppositions and assumptions with zero technical relevance to the quality of a library, as was a lot of the stuff I snipped. I don't work in pristine code environments, I like to keep around bits of code in #if 0 blocks to remind me of things, or for the occasional use, or for any other reason. I personally would find such segments educational when reviewing a library rather than the unfounded claim of "code smell" which may be true in other code bases in your experience, but I can assure you is not the case in my code bases.
I can think of two good reasons for '#if 0' blocks in production code. The first is where you'd like to have a reminder of something that was tried and didn't work, so that others know why it wasn't done that way and maybe not to try it. The second is for code that will be enabled, once some other condition is satisfied. It's true that the second can be maintained using branches, but that's not always practical for small snippets. In both cases, there should be a clear comment that explains why the code is disabled. While AFIO has such comments (though often quite terse) in some cases, there are several where it seems the reader is left to guess why it has been left in the code, but disabled. In short, there should always be a clear purpose behind leaving an #if 0'd block in the code. As long as that's true, it shouldn't be hard to state that in a brief, clear comment. Matt ________________________________ This e-mail contains privileged and confidential information intended for the use of the addressees named above. If you are not the intended recipient of this e-mail, you are hereby notified that you must not disseminate, copy or take any action in respect of any information contained in it. If you have received this e-mail in error, please notify the sender immediately by e-mail and immediately destroy this e-mail and its attachments.
On 30 Aug 2015 at 22:25, Gruenke,Matt wrote:
In both cases, there should be a clear comment that explains why the code is disabled. While AFIO has such comments (though often quite terse) in some cases, there are several where it seems the reader is left to guess why it has been left in the code, but disabled.
In short, there should always be a clear purpose behind leaving an #if 0'd block in the code. As long as that's true, it shouldn't be hard to state that in a brief, clear comment.
Yes, I agree. Logged to https://github.com/BoostGSoC13/boost.afio/issues/109. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 28-Aug-15 5:28 AM, Niall Douglas wrote:
* I doubt that std::cerr should be in this library's code. Use of std::cerr is only when something truly unusual occurs, like we are about to fatal exit the process and I would assume the user would like to know why we just called std::terminate. Or we are about to deadlock, and again the user probably wants to know why their application has just hanged itself.
It might be helpful for some cases, but then what about Windows UI applications. I, for one, does not know a way to extract std::cerr of an app that does not have a console, so if this were a useful mechanism, it should use debug log there. - Volodya
On 8/28/2015 1:51 AM, Vladimir Prus wrote:
On 28-Aug-15 5:28 AM, Niall Douglas wrote:
* I doubt that std::cerr should be in this library's code. Use of std::cerr is only when something truly unusual occurs, like we are about to fatal exit the process and I would assume the user would like to know why we just called std::terminate. Or we are about to deadlock, and again the user probably wants to know why their application has just hanged itself.
It might be helpful for some cases, but then what about Windows UI applications. I, for one, does not know a way to extract std::cerr of an app that does not have a console, so if this were a useful mechanism, it should use debug log there.
You just use rdbuf to set the stream buffer. You can see an example here for cout. http://www.cplusplus.com/reference/ios/ios/rdbuf/
On 29-Aug-15 3:12 AM, Michael Marcin wrote:
On 8/28/2015 1:51 AM, Vladimir Prus wrote:
On 28-Aug-15 5:28 AM, Niall Douglas wrote:
* I doubt that std::cerr should be in this library's code. Use of std::cerr is only when something truly unusual occurs, like we are about to fatal exit the process and I would assume the user would like to know why we just called std::terminate. Or we are about to deadlock, and again the user probably wants to know why their application has just hanged itself.
It might be helpful for some cases, but then what about Windows UI applications. I, for one, does not know a way to extract std::cerr of an app that does not have a console, so if this were a useful mechanism, it should use debug log there.
You just use rdbuf to set the stream buffer. You can see an example here for cout.
Michael, thanks for the suggestion. However, that seems a general advice; and I'm aware that you can change buffer like so, but I'd rather not write custom code to create a stream buffer that uses OutputDebugString. Other libraries don't force me to do so. - Volodya
Niall Douglas <s_sourceforge <at> nedprod.com> writes:
On 27 Aug 2015 at 12:54, Roland Bock wrote:
c) There are older versions in there. There is 1.3 and 1.4 at least (are there more?). Please choose one. Go for it.
AFIO provides strong ABI and API evolution guarantees. It guarantees that if you compile and link to v2 of the ABI, your code will continue to compile and link no matter what changes next in AFIO.
I think we all very much appreciate backwards compatibility guarantees (and indeed the lack of this in almost all C++ libraries, Boost included, can be quite annoying), but of course they do imply added size and complexity. My understanding is that AFIO, being a new library, doesn't have any existing users (except perhaps some of your own unpublished code, but that doesn't make for a very compelling justification) and therefore there is no benefit in providing backwards compatibility with prior versions of AFIO. It seems what you are trying to do is provide a demonstration of the way backwards compatibility will work for future versions of AFIO, when it would actually be useful, and also perhaps to provide an example of a good design for supporting backwards compatibility in C++ libraries. While this is interesting, perhaps it would be better to do this as an example separate from AFIO, so that AFIO can remain as small and simple as possible. One comment on this design: it seems that different version of AFIO will function as completely independent libraries, that can each be used independently. While this is certainly much better than what most C++ libraries provide, there is still the potential for AFIO version incompatibilities: one piece of code (A) may rely on version X of AFIO but also needs to interoperate (via AFIO types) with another piece of code (B) that formerly relied on AFIO version X, but upgraded to AFIO version X+1. Ideally the backwards/forwards compatibility would be finer-grained, so that this interoperability might be possible, but of course that tends to severely constrain the types of changes you can make.
[snip]
On 28 Aug 2015 at 7:56, Jeremy Maitin-Shepard wrote:
c) There are older versions in there. There is 1.3 and 1.4 at least (are there more?). Please choose one. Go for it.
AFIO provides strong ABI and API evolution guarantees. It guarantees that if you compile and link to v2 of the ABI, your code will continue to compile and link no matter what changes next in AFIO.
It seems what you are trying to do is provide a demonstration of the way backwards compatibility will work for future versions of AFIO, when it would actually be useful, and also perhaps to provide an example of a good design for supporting backwards compatibility in C++ libraries. While this is interesting, perhaps it would be better to do this as an example separate from AFIO, so that AFIO can remain as small and simple as possible.
The documentation and tutorials explain - fairly tersely it is true - how to pin your code to specific ABI versions or to always choose the latest. The examples you ask for are littered throughout the documentation. There is even a FAQ item with extra detail for the curious. It may well be because it's so lacking in obvious impact to end user code you don't realise it all already works and you get it for free.
One comment on this design: it seems that different version of AFIO will function as completely independent libraries, that can each be used independently. While this is certainly much better than what most C++ libraries provide, there is still the potential for AFIO version incompatibilities: one piece of code (A) may rely on version X of AFIO but also needs to interoperate (via AFIO types) with another piece of code (B) that formerly relied on AFIO version X, but upgraded to AFIO version X+1. Ideally the backwards/forwards compatibility would be finer-grained, so that this interoperability might be possible, but of course that tends to severely constrain the types of changes you can make.
You may not be aware you can also build AFIO using any choice of dependencies, so v1.40 has eight different potential configurations, all of which are incompatible and will not work together. This is unavoidable, because the dependencies do not work together e.g. std::future does not work with boost::future. All eight configurations will however coexist in the same translation unit. The exact same thing goes for previous versions of AFIO, so v1 in its eight configurations will coexist with v2 in its eight configurations. I haven't tested 16 copies of differently configured AFIO in the same translation unit, but I do regularly test two copies built with incompatible dependencies and that works very well. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Niall Douglas <s_sourceforge <at> nedprod.com> writes:
On 28 Aug 2015 at 7:56, Jeremy Maitin-Shepard wrote:
c) There are older versions in there. There is 1.3 and 1.4 at least (are there more?). Please choose one. Go for it.
AFIO provides strong ABI and API evolution guarantees. It guarantees that if you compile and link to v2 of the ABI, your code will continue to compile and link no matter what changes next in AFIO.
It seems what you are trying to do is provide a demonstration of the way backwards compatibility will work for future versions of AFIO, when it would actually be useful, and also perhaps to provide an example of a good design for supporting backwards compatibility in C++ libraries. While this is interesting, perhaps it would be better to do this as an example separate from AFIO, so that AFIO can remain as small and simple as possible.
The documentation and tutorials explain - fairly tersely it is true - how to pin your code to specific ABI versions or to always choose the latest. The examples you ask for are littered throughout the documentation. There is even a FAQ item with extra detail for the curious.
It may well be because it's so lacking in obvious impact to end user code you don't realise it all already works and you get it for free.
I understand that it is already supported, and I agree that it is great to be able to pin to the current version (V2) and have assurances that it will work, and this will also be useful once there is a V3. It is just the actual support for V1 that doesn't seem to serve any purpose, since my understanding is that there aren't any users of it. Please correct me if I'm wrong. I see that it is just a submodule reference within the repository, which nicely separates the code, though presumably in the default case this will result in users downloading/storing more data. I do very much appreciate the principle of what you are trying to do, as far as maintaining strict API/ABI versioning and clearly documenting changes made between versions. Indeed, this is something unfortunately lacking in other libraries. However, I think you picked too early a starting point for maintaining this strict versioning, assuming my understanding that there aren't any users of the pre-review versions is correct. Therefore I suggest that the references to pre-review versions of AFIO throughout the documentation should be removed, as they only add complexity to the documentation without providing any value to actual users. You could still keep a history of the development of the library in a separate document linked from the end of the table of the contents or something, but this information should not be integrated prominently throughout the documentation. For changes made after the library has users, doing it the way you have been doing clearly makes sense.
One comment on this design: it seems that different version of AFIO will function as completely independent libraries, that can each be used independently. While this is certainly much better than what most C++ libraries provide, there is still the potential for AFIO version incompatibilities: one piece of code (A) may rely on version X of AFIO but also needs to interoperate (via AFIO types) with another piece of code (B) that formerly relied on AFIO version X, but upgraded to AFIO version X+1. Ideally the backwards/forwards compatibility would be finer-grained, so that this interoperability might be possible, but of course that tends to severely constrain the types of changes you can make.
You may not be aware you can also build AFIO using any choice of dependencies, so v1.40 has eight different potential configurations, all of which are incompatible and will not work together. This is unavoidable, because the dependencies do not work together e.g. std::future does not work with boost::future.
All eight configurations will however coexist in the same translation unit. The exact same thing goes for previous versions of AFIO, so v1 in its eight configurations will coexist with v2 in its eight configurations. I haven't tested 16 copies of differently configured AFIO in the same translation unit, but I do regularly test two copies built with incompatible dependencies and that works very well.
I have been following the development of AFIO and your announcements regarding APIbind with much interest, so I was generally aware of these capabilities, although not the specific number of configurations. I very much like that AFIO can interoperate with either different library variants, but making the API and ABI depend on so many different configuration choices has its own drawbacks. Is there a way, for instance, to make the library simultaneously interoperate with both filesystem TS paths and boost filesystem paths? Similarly for Boost future vs. C++11 std::future.
On 29 Aug 2015 at 5:52, Jeremy Maitin-Shepard wrote:
I understand that it is already supported, and I agree that it is great to be able to pin to the current version (V2) and have assurances that it will work, and this will also be useful once there is a V3. It is just the actual support for V1 that doesn't seem to serve any purpose, since my understanding is that there aren't any users of it. Please correct me if I'm wrong.
If a v1 were not in there, I would be hearing complaints that there is no demonstration of this feature in action and without such a demonstration I cannot claim the feature in the documentation. I am sure you can see I cannot win here: nobody is happy. For the record, I intend to remove v1 when v3 is released. Until then, v1 works very well and is a well tested, reliable implementation.
I very much like that AFIO can interoperate with either different library variants, but making the API and ABI depend on so many different configuration choices has its own drawbacks. Is there a way, for instance, to make the library simultaneously interoperate with both filesystem TS paths and boost filesystem paths? Similarly for Boost future vs. C++11 std::future.
Such interoperation comes down to the willingness of the maintainers of those Boost libraries. I do have some control over the futures interoperation as I control the lightweight futures library. As lightweight futures allow you to mix together any custom future type, you can compose some but not all waits from across multiple incompatible configurations of AFIO. Past that, there is unfortunately very little I can do. Personally speaking I would find it very useful if Boost's STL 11 facilities understood the C++ 11 STL equivalents. For example, std::error_code could convert into boost::error_code, std::filesystem::path could convert into boost::filesystem::path and so on. But all that is a separate issue to AFIO or APIBind. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 2015-08-29 15:44, Niall Douglas wrote:
On 29 Aug 2015 at 5:52, Jeremy Maitin-Shepard wrote:
I understand that it is already supported, and I agree that it is great to be able to pin to the current version (V2) and have assurances that it will work, and this will also be useful once there is a V3. It is just the actual support for V1 that doesn't seem to serve any purpose, since my understanding is that there aren't any users of it. Please correct me if I'm wrong. If a v1 were not in there, I would be hearing complaints that there is no demonstration of this feature in action and without such a demonstration I cannot claim the feature in the documentation. I thought you wanted to propose APIBind separately anyway?
I am sure you can see I cannot win here: nobody is happy.
That way, everybody could be happy.
On 28.08.2015 05:28, Niall Douglas wrote:
On 27 Aug 2015 at 12:54, Roland Bock wrote:
* "The /final/ named blob store design" starts with "Caution: This section was not finished". * "The performance of and problems with this third generation design" consists of "This section was not completed in time[...]" * So basically the crucial parts of the blob store sequence are incomplete at best.
The final example is over 1000 lines of code. I didn't think it mattered hugely to the review.
You mentioned this earlier as well. I keep wondering why an example requires more than 1000 lines of code? Is it because the task is too broad or because the library interface is too verbose? In both cases something should be done about it - either change the docs or the library.
f) Code (since the implementation is going to change anyway, this might all be useless):
* /attic: Seriously?
AFIO will be three years old soon. It had a life before being ported to Boost in 2013. There are still bits and pieces in the attic directory of use to me.
The implementation in Boost should preferably be free from lecacy code. If that code is generally useful then why is it not in the library? If it's only usefult to you then keep it in your own repo separate from the library.
* I am not a big fan of #if 0 in release code
For the examples, logged at https://github.com/BoostGSoC13/boost.afio/issues/93.
In the implementation, if it's #if 0 it's almost always debug assist code which I turn on from time to time to help fix some reported bug. Or it's code which isn't fully baked yet and end users shouldn't use it e.g. byte range locking.
* I doubt that std::cerr should be in this library's code.
Use of std::cerr is only when something truly unusual occurs, like we are about to fatal exit the process and I would assume the user would like to know why we just called std::terminate.
Please, don't do that (both print anything and call std::terminate). In my projects I'm very often required to remove any output to the console and redirect it to a (custom) log library. Third party libraries that have hard-coded console output are always a pain and have to be patched. Calling std::terminate is also something I would call unexpected and harmful. You don't know what the application is doing - one thread may be doing something with Boost.AFIO and another may be doing domething important and unrelated. Throw exceptions on errors, where possible. Document UB when not.
Or we are about to deadlock, and again the user probably wants to know why their application has just hanged itself.
Let it deadlock. Seriously. At least the user will be able to see it blocked and get a backtrace to figure out how did it come to this. Terminating the app is the most useless and harmful reaction.
On 8/28/2015 4:31 AM, Andrey Semashev wrote:
Calling std::terminate is also something I would call unexpected and harmful. You don't know what the application is doing - one thread may be doing something with Boost.AFIO and another may be doing domething important and unrelated. Throw exceptions on errors, where possible. Document UB when not.
If it's documented as UB and implemented as calling std::terminate and prints to std::cerr that seems to satisfy your requirements without making any changes to the code. I assume you want the code to not do this however.
On Friday 28 August 2015 19:15:58 Michael Marcin wrote:
On 8/28/2015 4:31 AM, Andrey Semashev wrote:
Calling std::terminate is also something I would call unexpected and harmful. You don't know what the application is doing - one thread may be doing something with Boost.AFIO and another may be doing domething important and unrelated. Throw exceptions on errors, where possible. Document UB when not.
If it's documented as UB and implemented as calling std::terminate and prints to std::cerr that seems to satisfy your requirements without making any changes to the code.
I assume you want the code to not do this however.
By UB I mean the library doesn't attempt to recover from the error. If the user passed an invalid pointer then let it crash. If the program logic locks mutexes or files in an inconsistent order then let it deadlock. Or whatever the behavior in such cases on the current platform is. When the library attempts to recover from the error then this should be a documented behavior, and very much defined. Documenting a well defined error handling as UB is not useful or fair IMO.
On 28 Aug 2015 at 12:31, Andrey Semashev wrote:
On 28.08.2015 05:28, Niall Douglas wrote:
The final example is over 1000 lines of code. I didn't think it mattered hugely to the review.
You mentioned this earlier as well. I keep wondering why an example requires more than 1000 lines of code? Is it because the task is too broad or because the library interface is too verbose? In both cases something should be done about it - either change the docs or the library.
The earlier steps in the tutorial showed a design probably close to what the average C++ programmer might first think of and how you might implement that in STL iostreams and then AFIO. All those steps are complete, and with benchmarks. The final step shows a design that an expert in the field would write as an example of a real world solution to a real world problem as a comparator to the earlier examples. Similar to lock free memory programming, the final step is entirely file system lock free and uses numerous tricks of the trade to achieve a superb technical solution to the original problem of a simple key value store. It contains memory mapped dense hash maps (alone is about 250 lines of implementation), a concurrent transaction conflict resolution mechanism, resilience to incomplete and/or partial writes and corrupted metadata including power loss, and a complete versioning system letting you replay the history of transactions modifying the store. It achieves late durability without requiring fsync by making use of concurrent atomic appends to enforce extent aging and flushing, so performance is really excellent compared to alternative designs. The fact all that fits into just 750 lines (1000 - 250 for the dense hash map) is a demonstration of just how much utility and time saving AFIO delivers. But for sure it's going to be gobbledy gook to anyone not versed in the field which is precisely why it both should be in there and isn't important for a Boost review in my opinion. Do note that the documentation *does* describe the final stage in detail and how it works. I felt that level of detail sufficient for a review, though the comparative benchmarks would have been nice. As I mentioned, I had a hard drive failure which got in the way. It happens.
Calling std::terminate is also something I would call unexpected and harmful. You don't know what the application is doing - one thread may be doing something with Boost.AFIO and another may be doing domething important and unrelated. Throw exceptions on errors, where possible. Document UB when not.
If AFIO is fatal exiting the process, you are in an unrecoverable cannot continue situation. I mainly have those traps in there for handling memory corruption which is not uncommon when writing file system applications. They are *extremely* useful for catching inadvertent memory corruption. And they should never trip unless memory corruption has occurred or you have a serious error in how you are using AFIO.
Or we are about to deadlock, and again the user probably wants to know why their application has just hanged itself.
Let it deadlock. Seriously. At least the user will be able to see it blocked and get a backtrace to figure out how did it come to this. Terminating the app is the most useless and harmful reaction.
Fatal exit happens when AFIO cannot continue without losing or corrupting other people's data. I make no apology for that - it is the right thing to do. Deadlocks are handled by continuing to deadlock in debug builds, though with plenty of help and backtraces etc printed to std::cerr. This lets you interrupt in a debugger and try and figure out the cause. In release builds, after a reasonably long timeout you get a terse single line message to std::cerr and it breaks the deadlock and mops up the orphaned state as best it can before continuing. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On Saturday 29 August 2015 01:29:26 Niall Douglas wrote:
On 28 Aug 2015 at 12:31, Andrey Semashev wrote:
On 28.08.2015 05:28, Niall Douglas wrote:
The final example is over 1000 lines of code. I didn't think it mattered hugely to the review.
You mentioned this earlier as well. I keep wondering why an example requires more than 1000 lines of code? Is it because the task is too broad or because the library interface is too verbose? In both cases something should be done about it - either change the docs or the library.
The earlier steps in the tutorial showed a design probably close to what the average C++ programmer might first think of and how you might implement that in STL iostreams and then AFIO. All those steps are complete, and with benchmarks.
The final step shows a design that an expert in the field would write as an example of a real world solution to a real world problem as a comparator to the earlier examples. Similar to lock free memory programming, the final step is entirely file system lock free and uses numerous tricks of the trade to achieve a superb technical solution to the original problem of a simple key value store. It contains memory mapped dense hash maps (alone is about 250 lines of implementation), a concurrent transaction conflict resolution mechanism, resilience to incomplete and/or partial writes and corrupted metadata including power loss, and a complete versioning system letting you replay the history of transactions modifying the store. It achieves late durability without requiring fsync by making use of concurrent atomic appends to enforce extent aging and flushing, so performance is really excellent compared to alternative designs.
The fact all that fits into just 750 lines (1000 - 250 for the dense hash map) is a demonstration of just how much utility and time saving AFIO delivers. But for sure it's going to be gobbledy gook to anyone not versed in the field which is precisely why it both should be in there and isn't important for a Boost review in my opinion.
Do note that the documentation *does* describe the final stage in detail and how it works. I felt that level of detail sufficient for a review, though the comparative benchmarks would have been nice. As I mentioned, I had a hard drive failure which got in the way. It happens.
All that sounds great but regardless of its value an example of 1000 lines of code just isn't comprehensible. You mentioned several design points that compose this example (hash maps, transactions and so on), which means the example is decomposable. Perhaps you should document these bits separately along with any specifics and benefits wrt Boost.AFIO. Maybe even include tools for implementing these things into Boost.AFIO. But don't unload a heavy pile of code on the reader.
Calling std::terminate is also something I would call unexpected and harmful. You don't know what the application is doing - one thread may be doing something with Boost.AFIO and another may be doing domething important and unrelated. Throw exceptions on errors, where possible. Document UB when not.
If AFIO is fatal exiting the process, you are in an unrecoverable cannot continue situation. I mainly have those traps in there for handling memory corruption which is not uncommon when writing file system applications. They are *extremely* useful for catching inadvertent memory corruption.
And they should never trip unless memory corruption has occurred or you have a serious error in how you are using AFIO.
What kind of memory corruption do you mean and how do you detect it? Also how terminating the process helps in fixing those errors?
Or we are about to deadlock, and again the user probably wants to know why their application has just hanged itself.
Let it deadlock. Seriously. At least the user will be able to see it blocked and get a backtrace to figure out how did it come to this. Terminating the app is the most useless and harmful reaction.
Fatal exit happens when AFIO cannot continue without losing or corrupting other people's data. I make no apology for that - it is the right thing to do.
Why is this even possible with the library? And why not report an error in a usual way?
Deadlocks are handled by continuing to deadlock in debug builds, though with plenty of help and backtraces etc printed to std::cerr. This lets you interrupt in a debugger and try and figure out the cause. In release builds, after a reasonably long timeout you get a terse single line message to std::cerr and it breaks the deadlock and mops up the orphaned state as best it can before continuing.
In production there are release builds, not debug. We don't even use debug builds while testing because they don't really add anything but the possibility for the error to slip unnoticed in release builds. You may say this is an unusual policy but an least at two jobs I worked at practiced this. Altering the state before exiting is also something that doesn't look right to me. Having the latest state of the crashed process is sometimes helpful in debugging. Along with console output and unexpected program termination this is one of the "never do that" things. I think you're trying to be too smart in your library. Don't. You will never cover everything users want. There will always be someone like me who wants a different behavior than you envisioned.
On 29 Aug 2015 at 15:36, Andrey Semashev wrote:
If AFIO is fatal exiting the process, you are in an unrecoverable cannot continue situation. I mainly have those traps in there for handling memory corruption which is not uncommon when writing file system applications. They are *extremely* useful for catching inadvertent memory corruption.
And they should never trip unless memory corruption has occurred or you have a serious error in how you are using AFIO.
What kind of memory corruption do you mean and how do you detect it?
If an impossible situation has occurred i.e. if one bit of data in location A says one thing and another bit of data in location B says the opposite, it is assumed to be memory corruption. It should never, ever occur in any situation other than memory corruption.
Also how terminating the process helps in fixing those errors?
If memory corruption has occurred, then known program state has been lost and the loss of other people's data is a high likelihood. Terminating the process is the only sane thing to do now.
Fatal exit happens when AFIO cannot continue without losing or corrupting other people's data. I make no apology for that - it is the right thing to do.
Why is this even possible with the library?
I always code in detection of impossible states and fatal exit in any of my code and I have done so for years. It has proven its worth time and time again. I obviously don't go overboard with O(N^2) proof of good state checks in release builds, but if detection of impossible states has a low overhead, it's worth doing as a matter of routine. Think of it being a manually written undefined behaviour sanitiser. It's no different.
And why not report an error in a usual way?
To be honest it's never come up. In use cases without memory corruption you never see this code in action. On the other hand, when someone logs a bug which mentions all this stuff printed to std::cerr, it's an excellent sign they have memory corruption problems.
Deadlocks are handled by continuing to deadlock in debug builds, though with plenty of help and backtraces etc printed to std::cerr. This lets you interrupt in a debugger and try and figure out the cause. In release builds, after a reasonably long timeout you get a terse single line message to std::cerr and it breaks the deadlock and mops up the orphaned state as best it can before continuing.
In production there are release builds, not debug. We don't even use debug builds while testing because they don't really add anything but the possibility for the error to slip unnoticed in release builds. You may say this is an unusual policy but an least at two jobs I worked at practiced this.
Altering the state before exiting is also something that doesn't look right to me. Having the latest state of the crashed process is sometimes helpful in debugging. Along with console output and unexpected program termination this is one of the "never do that" things.
My rationale is that the deadlock detection occurs in a destructor and that is almost always being called in a process shutdown scenario. I figured a default timeout and mop up wasn't a bad choice as I can't throw an exception. The deadlock printing code is already macro enabled but for other reasons. I have logged an issue to add an explicit macro at https://github.com/BoostGSoC13/boost.afio/issues/108.
I think you're trying to be too smart in your library. Don't. You will never cover everything users want. There will always be someone like me who wants a different behavior than you envisioned.
I am not being too smart in my library. I am choosing sensible defaults which are useful in most use cases to most people. All this is really quality of implementation detail anyway. Issues have been logged for controlling the printing to std::cerr and enabling or disabling the deadlock debug printing code. If at a later stage people find they need additional behaviours in their real world use cases I have no problem adding solutions for them as well. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On Saturday 29 August 2015 15:29:32 Niall Douglas wrote:
On 29 Aug 2015 at 15:36, Andrey Semashev wrote:
What kind of memory corruption do you mean and how do you detect it?
If an impossible situation has occurred i.e. if one bit of data in location A says one thing and another bit of data in location B says the opposite, it is assumed to be memory corruption. It should never, ever occur in any situation other than memory corruption.
Also how terminating the process helps in fixing those errors?
If memory corruption has occurred, then known program state has been lost and the loss of other people's data is a high likelihood. Terminating the process is the only sane thing to do now.
Fatal exit happens when AFIO cannot continue without losing or corrupting other people's data. I make no apology for that - it is the right thing to do.
Why is this even possible with the library?
I always code in detection of impossible states and fatal exit in any of my code and I have done so for years. It has proven its worth time and time again. I obviously don't go overboard with O(N^2) proof of good state checks in release builds, but if detection of impossible states has a low overhead, it's worth doing as a matter of routine.
Think of it being a manually written undefined behaviour sanitiser. It's no different.
If you're trying to protect yourself from a memory failure this way then I think your approach is futile. You can't guarantee correctness with your checks, however many you insert in the code. If you want better guarantees buy a better hardware. If you're trying to protect from a program bug that results in memory corruption (e.g. through a dangling pointer) then terminating with std::terminate is useless because it doesn't help to locate and eliminate the bug. It doesn't help the customer either since his production service crashes all the time. You may save the data from corruption but again - there are no guarantees as the corrupted data may have slipped through. I guess this is better than nothing but hardly something I could count on.
And why not report an error in a usual way?
To be honest it's never come up. In use cases without memory corruption you never see this code in action. On the other hand, when someone logs a bug which mentions all this stuff printed to std::cerr, it's an excellent sign they have memory corruption problems.
So the checks never fired so far but you say they have proven their worth many times. In what way?
I think you're trying to be too smart in your library. Don't. You will never cover everything users want. There will always be someone like me who wants a different behavior than you envisioned.
I am not being too smart in my library. I am choosing sensible defaults which are useful in most use cases to most people. All this is really quality of implementation detail anyway.
Trying to protect from memory failures is one example of being too smart. This isn't the library's job to do. Console output also doesn't look like something universally accepted.
Issues have been logged for controlling the printing to std::cerr and enabling or disabling the deadlock debug printing code. If at a later stage people find they need additional behaviours in their real world use cases I have no problem adding solutions for them as well.
In general I'd discourage from adding configuration macros as they increase the possibility of config mismatch between TUs. Please consider macro-free options first.
On 29 Aug 2015 at 18:58, Andrey Semashev wrote:
I always code in detection of impossible states and fatal exit in any of my code and I have done so for years. It has proven its worth time and time again. I obviously don't go overboard with O(N^2) proof of good state checks in release builds, but if detection of impossible states has a low overhead, it's worth doing as a matter of routine.
Think of it being a manually written undefined behaviour sanitiser. It's no different.
If you're trying to protect yourself from a memory failure this way then I think your approach is futile. You can't guarantee correctness with your checks, however many you insert in the code. If you want better guarantees buy a better hardware.
If you're trying to protect from a program bug that results in memory corruption (e.g. through a dangling pointer) then terminating with std::terminate is useless because it doesn't help to locate and eliminate the bug. It doesn't help the customer either since his production service crashes all the time. You may save the data from corruption but again - there are no guarantees as the corrupted data may have slipped through. I guess this is better than nothing but hardly something I could count on.
Remember that AFIO is reading and writing other people's data. If internal state has become corrupt, AFIO no longer knows if the 5Mb of data to be written at offset X in file Y is right or wrong. If it proceeded, you risk corruption and damage to other people's data. This is why I make no apologies for immediate fatal exit of the process in this situation. If AFIO were not reading and writing other people's data, I'd be far happier to do something less extreme.
To be honest it's never come up. In use cases without memory corruption you never see this code in action. On the other hand, when someone logs a bug which mentions all this stuff printed to std::cerr, it's an excellent sign they have memory corruption problems.
So the checks never fired so far but you say they have proven their worth many times. In what way?
Oh they do fire for me during my development, but very rarely for commits pushed to master branch. By that stage they are hopefully debugged.
I think you're trying to be too smart in your library. Don't. You will never cover everything users want. There will always be someone like me who wants a different behavior than you envisioned.
I am not being too smart in my library. I am choosing sensible defaults which are useful in most use cases to most people. All this is really quality of implementation detail anyway.
Trying to protect from memory failures is one example of being too smart. This isn't the library's job to do. Console output also doesn't look like something universally accepted.
AFIO doesn't go to extremes to check for memory corruption. It does some basic checks for internal consistency, ones which are lightweight and cost very little. The console output is already logged with an issue to change that to a macro so it can be changed by anyone requiring that.
Issues have been logged for controlling the printing to std::cerr and enabling or disabling the deadlock debug printing code. If at a later stage people find they need additional behaviours in their real world use cases I have no problem adding solutions for them as well.
In general I'd discourage from adding configuration macros as they increase the possibility of config mismatch between TUs. Please consider macro-free options first.
These are internal code on vs off switches. They have no effect on ABI, and if an end user wants a local customised variant which coexists with other copies of AFIO it's very easy to make one thanks to APIBind. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
participants (12)
-
Andrey Semashev -
Glen Fernandes -
Gruenke,Matt -
Hartmut Kaiser -
Jeremy Maitin-Shepard -
Michael Marcin -
Niall Douglas -
Robert Ramey -
Rodrigo Madera -
Roland Bock -
Thomas Heller -
Vladimir Prus